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

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

On 17/08/2021 21:24, David Sterba wrote:
> On Mon, Aug 09, 2021 at 03:26:13PM -0300, 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>
>> ---
>>
>>   This change mimics what the kernel currently does, which is set the stripe_size
>>   regardless of the profile. Any thoughts on it? Thanks!
> 


> Makes sense to unify that,

  Patch [1] also said the same thing 1.5years back.
  Probably in a wrong context and timing? ;-)

  [1]
 
https://patchwork.kernel.org/project/linux-btrfs/patch/1583926429-28485-1-git-send-email-anand.jain@oracle.com/

-Anand

 >  it works well for the large sizes.
> Please
> write tests that verify that the chunk sizes are correct after mkfs on
> various device sizes. Patch added to devel, thanks.
>