[Ocfs2-tools-devel] Re: New patch for backup superblock against r1273 of ocfs2-tools

[tao.ma]

Sunil Mushran wrote:
> 1. ocfs2_set_backup_sb():
> +       ret = ocfs2_malloc_block(fs->fs_io, &buf);
> +       if (ret)
> +               goto bail;
> +
> +       memset(buf, 0, fs->fs_blocksize);
> +
> +       for (i = 0; i < len; i++, blkno++) {
> +               if (!*blkno)
> +                       break;
> +
> +               /* zero the whole cluster first */
> +               for (j = 0; j < bpc; j++) {
> +                       ret = io_write_block(fs->fs_io, *blkno + j, 1, buf);
> +                       if (ret)
> +                               goto bail;
> +               }
> How about allocating fs->fs_clustersize and initializing in one chunk.
>
> The other bug here is that it assumes the cluster start matches the block start.
> Are you sure about that? 
>   
I am sure about this. Since the block is calculated from backup byte 
offset and the least byte offset is 1G, with all the cluster size(from 
4K to 1M), they are all cluster-aligned. So these blocks num should be 
the cluster start. Your option about fs_clustersize is right. I will 
change it.
> 2. ocfs2_set_backup_sb():
> +       if (fs->fs_cluster_alloc) {
> +               ocfs2_free_cached_inode(fs, fs->fs_cluster_alloc);
> +               fs->fs_cluster_alloc = NULL;
> +       }
> Is this necessary?
>   
Yes, it is necessary. There is no place for releasing it. ocfs2_close 
doesn't do the work for us.
> 10. debugfs:
> +       /* iterate all the blocksize and get the right one. */
> +       for (blksize = OCFS2_MIN_BLOCKSIZE;
> +               blksize <= OCFS2_MAX_BLOCKSIZE; blksize <<= 1) {
> +               sb = byte_off[s-1] / blksize;
> +               ret = ocfs2_open(args[1], OCFS2_FLAG_RO, sb, blksize, &fs);
> +               if (!ret)
> +                       break;
> +       }
> Why? Why not just io_read_block(4k) at the offset and read the blocksize off the sb.
>   
You are right. The following ocfs2_open will do the work. I copy this 
code from fsck.ocfs2, in that scenario this ocfs2_open check is needed,  
here it is no use.
> Index: ocfs2-tools/tunefs.ocfs2/tunefs.c
> ===================================================================
> --- ocfs2-tools.orig/tunefs.ocfs2/tunefs.c
> +++ ocfs2-tools/tunefs.ocfs2/tunefs.c
> @@ -1224,15 +1224,36 @@ bail:
>  	return ret;
>  }
>  
> -static errcode_t update_backup_sb(ocfs2_filesys *fs)
> +
> +static errcode_t update_backup_sb(ocfs2_filesys *fs, uint64_t newblocks)
>  {
>  	errcode_t ret;
> -	int len;
> +	int num;
>  	uint64_t blocks[OCFS2_MAX_BACKUP_SUPERBLOCKS];
> +	uint64_t startblk = fs->fs_blocks - newblocks;
> +
> +	num = ocfs2_get_backup_sb_offset(fs, blocks, ARRAY_SIZE(blocks));
> +
> +	if (!num)
> +		return 0;
> +
> +	if (newblocks) {
> +		for (i = 0; i < num; ++i) {
> +			if (blocks[i] > startblk)
> +				break;
> +		}
> +
> +		if (!(num - i))
> +			return 0;
>  
> -	len = ocfs2_get_backup_sb_offset(fs, blocks, ARRAY_SIZE(blocks));
> +		if (i) {
> +			memmove(blocks, &blocks[i],
> +				sizeof(uint64_t) * (num - i));
> +			num -= i;
> +		}
> +	}
>  
> -	ret = ocfs2_set_backup_sb(fs, blocks, len);
> +	ret = ocfs2_set_backup_sb(fs, blocks, num);
>  	if (ret) {
>  		com_err(opts.progname, ret, "while backuping superblock.");
>  		goto bail;
> @@ -1261,6 +1282,7 @@ int main(int argc, char **argv)
>  	int dirty = 0;
>  	char old_uuid[OCFS2_VOL_UUID_LEN * 2 + 1];
>  	char new_uuid[OCFS2_VOL_UUID_LEN * 2 + 1];
> +	int refresh_backup_supers = 0;
>  
>  	initialize_ocfs_error_table();
>  	initialize_o2dl_error_table();
> @@ -1454,6 +1476,9 @@ int main(int argc, char **argv)
>  		update_volume_label(fs, &upd_label);
>  		if (upd_label)
>  			printf("Changed volume label\n");
> +		if (OCFS2_HAS_COMPAT_FEATURE(fs->fs_super,
> +					     OCFS2_FEATURE_COMPAT_BACKUP_SB))
> +			refresh_backup_supers = 1;
>  	}
>  
>  	/* update volume uuid */
> @@ -1509,6 +1534,17 @@ int main(int argc, char **argv)
>  			printf("Resized volume\n");
>  	}
>  
> +	/* update backup super */
> +	if (opts.backup_sb ||
> +	    (opts.num_blocks &&
> +	     OCFS2_HAS_COMPAT_FEATURE(fs->fs_super,
> +				      OCFS2_FEATURE_COMPAT_BACKUP_SB))) {
> +		ret = update_backup_sb(fs, opts.num_blocks);
> +		if (ret)
> +			handle error;
> +		OCFS2_SET_COMPAT_FEATURE(fs->fs_super, OCFS2_FEATURE_COMPAT_BACKUP_SB);
> +	}
> +
>  	/* write superblock */
>  	if (upd_label || upd_uuid || upd_slots || upd_blocks || upd_incompat ||
>  	    upd_mount) {
> @@ -1521,6 +1557,8 @@ int main(int argc, char **argv)
>  		block_signals(SIG_UNBLOCK);
>  		printf("Wrote Superblock\n");
>  
> +		ret = refresh_backup_sb(fs);
> +#if 0
>  		/* superblock's information has changed.
>  		 * we need to synchronize the backup blocks if needed.
>  		 */
> @@ -1537,8 +1575,9 @@ int main(int argc, char **argv)
>  			else
>  				printf("Updated backup superblock.\n");
>  		}
> +#endif
>  	}
> -
> +#if 0
>  	if (opts.backup_sb) {
>  		block_signals(SIG_BLOCK);
>  		ret = update_backup_sb(fs);
> @@ -1560,7 +1599,7 @@ int main(int argc, char **argv)
>  
>  		printf("Backuped Superblock.\n");
>  	}
> -
> +#endif
>  unlock:
>  	block_signals(SIG_BLOCK);
>  	if (cluster_locked && fs->fs_dlm_ctxt)
>   
I am not quite sure of your modification of tunefs. Why you want to take 
resize as a specific activity? I think it is a condition like the change 
of volume and slot add. Just want to make the whole mechanism clear?  It 
seems that the new backup offset is written twice in update_backup_sb 
and refresh_backup_sb. And actually these two functions almost do the 
same thing except that your function of update_backup_sb will update the 
backup sb according to the "newblocks" while refresh_backup_sb doesn't 
take it into consideration. I have to admitted my old mechanism is a 
little complex. So I will revise it and I think only one 
update_backup_sb is needed here.