Re: [PATCH] btrfs-progs: Drop the type check in init_alloc_chunk_ctl_policy_regular

[Anand Jain <anand.jain@oracle.com>]

On 10/08/2021 02:26, Marcos Paulo de Souza wrote:
> [PROBLEM]
> Our documentation says that a DATA block group can have up to 1G of
> size, or the at most 10% of the filesystem size. Currently, by default,
> mkfs.btrfs is creating an initial DATA block group of 8M of size,
> regardless of the filesystem size. It happens because we check for the
> ctl->type in init_alloc_chunk_ctl_policy_regular to be one of the
> BTRFS_BLOCK_GROUP_PROFILE_MASK bits, which is not the case for SINGLE
> (default) DATA block group:


> $ mkfs.btrfs -f /storage/btrfs.disk
> btrfs-progs v4.19.1
> See http://btrfs.wiki.kernel.org for more information.
> 
> Label:              (null)
> UUID:               39e3492f-41f2-4bd7-8c25-93032606b9fe
> Node size:          16384
> Sector size:        4096
> Filesystem size:    55.00GiB
> Block group profiles:
>    Data:             single            8.00MiB
>    Metadata:         DUP               1.00GiB
>    System:           DUP               8.00MiB
> SSD detected:       no
> Incompat features:  extref, skinny-metadata
> Number of devices:  1
> Devices:
>     ID        SIZE  PATH
>      1    55.00GiB  /storage/btrfs.disk
> 
> In this case, for single data profile, it should create a data block
> group of 1G, since the filesystem if bigger than 50G.
> 
> [FIX]
> Remove the check for BTRFS_BLOCK_GROUP_PROFILE_MASK in
> init_alloc_chunk_ctl_policy_regular function. The ctl->stripe_size is
> later on assigned to ctl.num_bytes, which is used by
> btrfs_make_block_group to create the block group.
> 
> By removing the check we allow the code to always configure the correct
> stripe_size regardless of the PROFILE, looking on the block group TYPE.
> 
> With the fix applied, it now created the BG correctly:
> 
> $ ./mkfs.btrfs -f /storage/btrfs.disk
> btrfs-progs v5.10.1
> See http://btrfs.wiki.kernel.org for more information.
> 
> Label:              (null)
> UUID:               5145e343-5639-462d-82ee-3eb75dc41c31
> Node size:          16384
> Sector size:        4096
> Filesystem size:    55.00GiB
> Block group profiles:
>    Data:             single            1.00GiB
>    Metadata:         DUP               1.00GiB
>    System:           DUP               8.00MiB
> SSD detected:       no
> Zoned device:       no
> Incompat features:  extref, skinny-metadata
> Runtime features:
> Checksum:           crc32c
> Number of devices:  1
> Devices:
>     ID        SIZE  PATH
>      1    55.00GiB  /storage/btrfs.disk
> 
> Using a disk >50G it creates a 1G single data block group. Another
> example:
> 
> $ ./mkfs.btrfs -f /storage/btrfs2.disk
> btrfs-progs v5.10.1
> See http://btrfs.wiki.kernel.org for more information.
> 
> Label:              (null)
> UUID:               c0910857-e512-4e76-9efa-50a475aafc87
> Node size:          16384
> Sector size:        4096
> Filesystem size:    5.00GiB
> Block group profiles:
>    Data:             single          512.00MiB
>    Metadata:         DUP             256.00MiB
>    System:           DUP               8.00MiB
> SSD detected:       no
> Zoned device:       no
> Incompat features:  extref, skinny-metadata
> Runtime features:
> Checksum:           crc32c
> Number of devices:  1
> Devices:
>     ID        SIZE  PATH
>      1     5.00GiB  /storage/btrfs2.disk
> 
> The code now created a single data block group of 512M, which is exactly
> 10% of the size of the filesystem, which is 5G in this case.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>

Looks good to me.

I hope there isn't any xfstests/tests/btrfs test case that is hardcoded
to the older block group alloc (like swap file test with relocation?).
But then the test case will need a fix, not btrfs-progs. So

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

> ---
> 
>   This change mimics what the kernel currently does, which is set the stripe_size
>   regardless of the profile. Any thoughts on it? Thanks!
> 
>   kernel-shared/volumes.c | 40 +++++++++++++++++++---------------------
>   1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
> index aeeb25fe..3677dd7c 100644
> --- a/kernel-shared/volumes.c
> +++ b/kernel-shared/volumes.c
> @@ -1105,27 +1105,25 @@ static void init_alloc_chunk_ctl_policy_regular(struct btrfs_fs_info *info,
>   	u64 type = ctl->type;
>   	u64 percent_max;
>   
> -	if (type & BTRFS_BLOCK_GROUP_PROFILE_MASK) {
> -		if (type & BTRFS_BLOCK_GROUP_SYSTEM) {
> -			ctl->stripe_size = SZ_8M;
> -			ctl->max_chunk_size = ctl->stripe_size * 2;
> -			ctl->min_stripe_size = SZ_1M;
> -			ctl->max_stripes = BTRFS_MAX_DEVS_SYS_CHUNK;
> -		} else if (type & BTRFS_BLOCK_GROUP_DATA) {
> -			ctl->stripe_size = SZ_1G;
> -			ctl->max_chunk_size = 10 * ctl->stripe_size;
> -			ctl->min_stripe_size = SZ_64M;
> -			ctl->max_stripes = BTRFS_MAX_DEVS(info);
> -		} else if (type & BTRFS_BLOCK_GROUP_METADATA) {
> -			/* For larger filesystems, use larger metadata chunks */
> -			if (info->fs_devices->total_rw_bytes > 50ULL * SZ_1G)
> -				ctl->max_chunk_size = SZ_1G;
> -			else
> -				ctl->max_chunk_size = SZ_256M;
> -			ctl->stripe_size = ctl->max_chunk_size;
> -			ctl->min_stripe_size = SZ_32M;
> -			ctl->max_stripes = BTRFS_MAX_DEVS(info);
> -		}
> +	if (type & BTRFS_BLOCK_GROUP_SYSTEM) {
> +		ctl->stripe_size = SZ_8M;
> +		ctl->max_chunk_size = ctl->stripe_size * 2;
> +		ctl->min_stripe_size = SZ_1M;
> +		ctl->max_stripes = BTRFS_MAX_DEVS_SYS_CHUNK;
> +	} else if (type & BTRFS_BLOCK_GROUP_DATA) {
> +		ctl->stripe_size = SZ_1G;
> +		ctl->max_chunk_size = 10 * ctl->stripe_size;
> +		ctl->min_stripe_size = SZ_64M;
> +		ctl->max_stripes = BTRFS_MAX_DEVS(info);
> +	} else if (type & BTRFS_BLOCK_GROUP_METADATA) {
> +		/* For larger filesystems, use larger metadata chunks */
> +		if (info->fs_devices->total_rw_bytes > 50ULL * SZ_1G)
> +			ctl->max_chunk_size = SZ_1G;
> +		else
> +			ctl->max_chunk_size = SZ_256M;
> +		ctl->stripe_size = ctl->max_chunk_size;
> +		ctl->min_stripe_size = SZ_32M;
> +		ctl->max_stripes = BTRFS_MAX_DEVS(info);
>   	}
>   
>   	/* We don't want a chunk larger than 10% of the FS */
>