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
