Re: [PATCH] btrfs: round down stripe size and chunk size to pow of 2

[Wang Yugui <wangyugui@e16-tech.com>]

Hi,

> On 2022/8/16 09:46, Wang Yugui wrote:
> > In decide_stripe_size_regular(), when new disk is added to RAID0/RAID10/RAID56,
> > it is better to alloc/reuse the free space if stripe size is kept or
> > changed to 1/2. so stripe size and chunk size of pow of 2 will be more
> > friendly.
> >
> > Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>
> > ---
> >   fs/btrfs/volumes.c | 20 +++++++++-----------
> >   1 file changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 6595755..fab9765 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -5083,9 +5083,9 @@ static void init_alloc_chunk_ctl_policy_regular(
> >   	if (ctl->type & BTRFS_BLOCK_GROUP_SYSTEM)
> >   		ctl->devs_max = min_t(int, ctl->devs_max, BTRFS_MAX_DEVS_SYS_CHUNK);
> >
> > -	/* We don't want a chunk larger than 10% of writable space */
> > -	ctl->max_chunk_size = min(div_factor(fs_devices->total_rw_bytes, 1),
> > -				  ctl->max_chunk_size);
> > +	/* We don't want a chunk larger than 1/8 of writable space */
> 
> For the 1/8 change I'm completely fine.
> 
> > +	ctl->max_chunk_size = min_t(u64, ctl->max_chunk_size,
> > +			rounddown_pow_of_two(fs_devices->total_rw_bytes >> 3));
> 
> But I'm not sure if there is any benefit for the extra
> dounwdown_pow_of_two().
> 
> Our chunk size doesn't really need to be power of 2 at all.
> 
> Any extra explanation on why power of 2 chunk size has any benefit?

Becasue stripe_size is roundup_pow_of_two()/rounddown_pow_of_two() later,
so real chunk size is roundup_pow_of_two() * N /rounddown_pow_of_two()
* N.

Here we get max_chunk_size base on  pow of 2 too, so that in
single/RAID1 profile,  max_chunk_size and chunk size will be same.

> >   	ctl->dev_extent_min = BTRFS_STRIPE_LEN * ctl->dev_stripes;
> >   }
> >
> > @@ -5143,10 +5143,9 @@ static void init_alloc_chunk_ctl_policy_zoned(
> >   		BUG();
> >   	}
> >
> > -	/* We don't want a chunk larger than 10% of writable space */
> > -	limit = max(round_down(div_factor(fs_devices->total_rw_bytes, 1),
> > -			       zone_size),
> > -		    min_chunk_size);
> > +	/* We don't want a chunk larger than 1/8 of writable space */
> > +	limit = max_t(u64, min_chunk_size,
> > +		rounddown_pow_of_two(fs_devices->total_rw_bytes >> 3));
> >   	ctl->max_chunk_size = min(limit, ctl->max_chunk_size);
> >   	ctl->dev_extent_min = zone_size * ctl->dev_stripes;
> >   }
> > @@ -5284,13 +5283,12 @@ static int decide_stripe_size_regular(struct alloc_chunk_ctl *ctl,
> >   	 */
> >   	if (ctl->stripe_size * data_stripes > ctl->max_chunk_size) {
> >   		/*
> > -		 * Reduce stripe_size, round it up to a 16MB boundary again and
> > +		 * Reduce stripe_size, round down to pow of 2 boundary again and
> >   		 * then use it, unless it ends up being even bigger than the
> >   		 * previous value we had already.
> >   		 */
> > -		ctl->stripe_size = min(round_up(div_u64(ctl->max_chunk_size,
> > -							data_stripes), SZ_16M),
> > -				       ctl->stripe_size);
> > +		ctl->stripe_size = min_t(u64, ctl->stripe_size,
> > +			rounddown_pow_of_two(div_u64(ctl->max_chunk_size, data_stripes)));
> 
> I think this can even be problematic since now stripe_size can be much
> smaller than what we want.
> 
> Thanks,
> Qu

rounddown_pow_of_two() is at least > round_up(,SZ_16M) * 0.5,
so it is not too much smaller.

But  roundup_pow_of_two() match with orig round_up(,SZ_16M)
better than rounddown_pow_of_two().

It will be changed to roundup_pow_of_two() in v2.

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2022/08/16