Hi, Anand Jain > -----Original Message----- > From: linux-btrfs-ow...@vger.kernel.org > [mailto:linux-btrfs-ow...@vger.kernel.org] On Behalf Of Zhao Lei > Sent: Friday, July 17, 2015 5:39 PM > To: 'Anand Jain'; linux-btrfs@vger.kernel.org > Subject: RE: [PATCH] btrfs: Add raid56 support for updating > num_tolerated_disk_barrier_failures in btrfs_balance() > > Hi, Anand Jain > > Thanks for review it. > > > -----Original Message----- > > From: Anand Jain [mailto:anand.j...@oracle.com] > > Sent: Friday, July 17, 2015 5:12 PM > > To: Zhaolei; linux-btrfs@vger.kernel.org > > Subject: Re: [PATCH] btrfs: Add raid56 support for updating > > num_tolerated_disk_barrier_failures in btrfs_balance() > > > > > > > > nice clean up thanks. but... more below. > > > > On 07/16/2015 08:15 PM, Zhaolei wrote: > > > From: Zhao Lei <zhao...@cn.fujitsu.com> > > > > > > Code for updating fs_info->num_tolerated_disk_barrier_failures in > > > btrfs_balance() lacks raid56 support. > > > > > > Reason: > > > Above code was wroten in 2012-08-01, together with > > > btrfs_calc_num_tolerated_disk_barrier_failures()'s first version. > > > > > > Then, btrfs_calc_num_tolerated_disk_barrier_failures() was updated > > > later to support raid56, but code in btrfs_balance() was not > > > updated together. > > > > > > Fix: > > > Merge these similar code by adding a argument to > > > btrfs_calc_num_tolerated_disk_barrier_failures() to make it > > > support both case. > > > > > > It can fix this bug with a bonus of cleanup, and make these code > > > never in current no-sync state from now on. > > > > > > Signed-off-by: Zhao Lei <zhao...@cn.fujitsu.com> > > > --- > > > fs/btrfs/disk-io.c | 9 +++++---- > > > fs/btrfs/disk-io.h | 2 +- > > > fs/btrfs/volumes.c | 28 +++++++++------------------- > > > 3 files changed, 15 insertions(+), 24 deletions(-) > > > > > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index > > > b6600c7..ac26111 100644 > > > --- a/fs/btrfs/disk-io.c > > > +++ b/fs/btrfs/disk-io.c > > > @@ -2946,7 +2946,7 @@ retry_root_backup: > > > goto fail_sysfs; > > > } > > > fs_info->num_tolerated_disk_barrier_failures = > > > - btrfs_calc_num_tolerated_disk_barrier_failures(fs_info); > > > + btrfs_calc_num_tolerated_disk_barrier_failures(fs_info, 0); > > > if (fs_info->fs_devices->missing_devices > > > > fs_info->num_tolerated_disk_barrier_failures && > > > !(sb->s_flags & MS_RDONLY)) { @@ -3441,7 +3441,7 @@ static > > > int barrier_all_devices(struct btrfs_fs_info > > *info) > > > } > > > > > > int btrfs_calc_num_tolerated_disk_barrier_failures( > > > - struct btrfs_fs_info *fs_info) > > > + struct btrfs_fs_info *fs_info, u64 extra_flags) > > > { > > > > extra_flags not required. since .. more below. > > > > > struct btrfs_ioctl_space_info space; > > > struct btrfs_space_info *sinfo; > > > @@ -3481,7 +3481,7 @@ int > > btrfs_calc_num_tolerated_disk_barrier_failures( > > > &space); > > > if (space.total_bytes == 0 || space.used_bytes > > > == 0) > > > continue; > > > - flags = space.flags; > > > + flags = space.flags | extra_flags; > > > /* > > > * return > > > * 0: if dup, single or RAID0 is configured for > > > @@ -3493,7 > > > +3493,8 @@ int btrfs_calc_num_tolerated_disk_barrier_failures( > > > */ > > > if (num_tolerated_disk_barrier_failures > 0 && > > > ((flags & (BTRFS_BLOCK_GROUP_DUP | > > > - BTRFS_BLOCK_GROUP_RAID0)) || > > > + BTRFS_BLOCK_GROUP_RAID0 | > > > + BTRFS_AVAIL_ALLOC_BIT_SINGLE)) || > > > ((flags & BTRFS_BLOCK_GROUP_PROFILE_MASK) > > > == > > 0))) > > > num_tolerated_disk_barrier_failures = 0; > > > else if (num_tolerated_disk_barrier_failures > > > > 1 && diff > > --git > > > a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h index d4cbfee..aceaa8d > > > 100644 > > > --- a/fs/btrfs/disk-io.h > > > +++ b/fs/btrfs/disk-io.h > > > @@ -140,7 +140,7 @@ struct btrfs_root *btrfs_create_tree(struct > > btrfs_trans_handle *trans, > > > int btree_lock_page_hook(struct page *page, void *data, > > > void (*flush_fn)(void *)); > > > int btrfs_calc_num_tolerated_disk_barrier_failures( > > > - struct btrfs_fs_info *fs_info); > > > + struct btrfs_fs_info *fs_info, u64 extra_flags); > > > int __init btrfs_end_io_wq_init(void); > > > void btrfs_end_io_wq_exit(void); > > > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index > > > fbe7c10..d739915 100644 > > > --- a/fs/btrfs/volumes.c > > > +++ b/fs/btrfs/volumes.c > > > @@ -1812,7 +1812,8 @@ int btrfs_rm_device(struct btrfs_root *root, > > > char > > *device_path) > > > } > > > > > > root->fs_info->num_tolerated_disk_barrier_failures = > > > - btrfs_calc_num_tolerated_disk_barrier_failures(root->fs_info); > > > + btrfs_calc_num_tolerated_disk_barrier_failures(root->fs_info, > > > + 0); > > > > > > /* > > > * at this point, the device is zero sized. We want to @@ > > > -2342,7 > > > +2343,8 @@ int btrfs_init_new_device(struct btrfs_root *root, char > > *device_path) > > > } > > > > > > root->fs_info->num_tolerated_disk_barrier_failures = > > > - btrfs_calc_num_tolerated_disk_barrier_failures(root->fs_info); > > > + btrfs_calc_num_tolerated_disk_barrier_failures(root->fs_info, > > > + 0); > > > ret = btrfs_commit_transaction(trans, root); > > > > > > if (seeding_dev) { > > > @@ -3573,23 +3575,10 @@ int btrfs_balance(struct > > > btrfs_balance_control > > *bctl, > > > } while (read_seqretry(&fs_info->profiles_lock, seq)); > > > > > > if (bctl->sys.flags & BTRFS_BALANCE_ARGS_CONVERT) { > > > - int num_tolerated_disk_barrier_failures; > > > - u64 target = bctl->sys.target; > > > - > > > - num_tolerated_disk_barrier_failures = > > > - btrfs_calc_num_tolerated_disk_barrier_failures(fs_info); > > > - if (num_tolerated_disk_barrier_failures > 0 && > > > - (target & > > > - (BTRFS_BLOCK_GROUP_DUP | BTRFS_BLOCK_GROUP_RAID0 | > > > - BTRFS_AVAIL_ALLOC_BIT_SINGLE))) > > > - num_tolerated_disk_barrier_failures = 0; > > > - else if (num_tolerated_disk_barrier_failures > 1 && > > > - (target & > > > - (BTRFS_BLOCK_GROUP_RAID1 | > > BTRFS_BLOCK_GROUP_RAID10))) > > > - num_tolerated_disk_barrier_failures = 1; > > > - > > > fs_info->num_tolerated_disk_barrier_failures = > > > - num_tolerated_disk_barrier_failures; > > > + btrfs_calc_num_tolerated_disk_barrier_failures( > > > + fs_info, > > > + bctl->sys.target); > > > } > > > > > > target is part of the user-end set item. please don't propagate > > that to the function btrfs_calc_num_tolerated_disk_barrier_failures() > > which is quite usefully used by many more functions. target must be > > handled in here. > > > > Also, while you are here it looks like this and > > btrfs_chunk_max_errors() can be merged as well. > > > > Do you means use btrfs_chunk_max_errors() here to calculate > s_info->num_tolerated_disk_barrier_failures here, instead of adding a extea > argument to btrfs_calc_num_tolerated_disk_barrier_failures(), > like: > > info->num_tolerated_disk_barrier_failures = > min( > btrfs_calc_num_tolerated_disk_barrier_failures(fs_info), > btrfs_chunk_max_errors(bctl->sys.target) > ); >
I'll send v2 based on your comment of: Don't propagate extra argument to btrfs_calc_num_tolerated_disk_barrier_failures() which is quite usefully used by many more functions. btrfs_chunk_max_errors() is similar but have little different with our request, so I merged and move these common code into new function: btrfs_calc_num_tolerated_disk_barrier_failures() different of these functions are: btrfs_calc_num_tolerated_disk_barrier_failures(): max wrong disks btrfs_chunk_max_errors(): max wrong mirrors For dup, max wrong disks is 0, and max wrong mirrors is 1. Thanks Zhaolei > Thanks > Zhaolei > > > Thanks. Anand > > > > > > > ret = insert_balance_item(fs_info->tree_root, bctl); @@ -3616,7 > > > +3605,8 @@ int btrfs_balance(struct btrfs_balance_control *bctl, > > > > > > if (bctl->sys.flags & BTRFS_BALANCE_ARGS_CONVERT) { > > > fs_info->num_tolerated_disk_barrier_failures = > > > - btrfs_calc_num_tolerated_disk_barrier_failures(fs_info); > > > + btrfs_calc_num_tolerated_disk_barrier_failures(fs_info, > > > + 0); > > > } > > > > > > if (bargs) { > > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the > body > of a message to majord...@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html