Re: [PATCH 6/9] btrfs-progs: add the block group item in make_btrfs()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/8/24 上午7:47, Josef Bacik wrote:
> On 8/23/21 7:37 PM, Qu Wenruo wrote:
>>
>>
>> On 2021/8/24 上午4:04, Josef Bacik wrote:
>>> On 8/23/21 5:00 AM, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2021/8/21 上午3:11, Josef Bacik wrote:
>>>>> Currently we build a bare-bones file system in make_btrfs(), and
>>>>> then we
>>>>> load it up and fill in the rest of the file system after the fact.
>>>>> One
>>>>> thing we omit in make_btrfs() is the block group item for the
>>>>> temporary
>>>>> system chunk we allocate, because we just add it after we've opened
>>>>> the
>>>>> file system.
>>>>>
>>>>> However I want to be able to generate the free space tree at
>>>>> make_btrfs() time, because extent tree v2 will not have an extent tree
>>>>> that has every block allocated in the system.  In order to do this I
>>>>> need to make sure that the free space tree entries are added on block
>>>>> group creation, which is annoying if we have to add this chunk after
>>>>> I've created a free space tree.
>>>>>
>>>>> So make future work simpler by simply adding our block group item at
>>>>> make_btrfs() time, this way I can do the right things with the free
>>>>> space tree in the generic make block group code without needing a
>>>>> special case for our temporary system chunk.
>>>>>
>>>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>>>> ---
>>>>>   mkfs/common.c | 31 +++++++++++++++++++++++++++++++
>>>>>   mkfs/main.c   |  9 ++-------
>>>>>   2 files changed, 33 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/mkfs/common.c b/mkfs/common.c
>>>>> index 9263965e..cba97687 100644
>>>>> --- a/mkfs/common.c
>>>>> +++ b/mkfs/common.c
>>>>> @@ -190,6 +190,7 @@ int make_btrfs(int fd, struct btrfs_mkfs_config
>>>>> *cfg)
>>>>>       u64 num_bytes;
>>>>>       u64 system_group_offset = BTRFS_BLOCK_RESERVED_1M_FOR_SUPER;
>>>>>       u64 system_group_size = BTRFS_MKFS_SYSTEM_GROUP_SIZE;
>>>>> +    bool add_block_group = true;
>>>>>
>>>>>       if ((cfg->features & BTRFS_FEATURE_INCOMPAT_ZONED)) {
>>>>>           system_group_offset = cfg->zone_size *
>>>>> BTRFS_NR_SB_LOG_ZONES;
>>>>> @@ -283,6 +284,36 @@ int make_btrfs(int fd, struct btrfs_mkfs_config
>>>>> *cfg)
>>>>>           if (blk == MKFS_SUPER_BLOCK)
>>>>>               continue;
>>>>>
>>>>> +        /* Add the block group item for our temporary chunk. */
>>>>> +        if (cfg->blocks[blk] > system_group_offset &&
>>>>> +            add_block_group) {
>>>>
>>>> This makes the block group item always be the first item.
>>>>
>>>> But for skinny metadata, METADATA_ITEM is smaller than
>>>> BLOCK_GROUP_ITEM,
>>>> meaning it should be before BLOCK_GROUP_ITEM.
>>>>
>>>> Won't this cause the extent tree has a bad key order?
>>>>
>>>
>>> No because it's based on the actual bytenr.  We'll insert the extent
>>> item entry first, and then move to the next block and if it's past the
>>> first block we add the block group item, and then the actual extent
>>> item.  So it goes
>>>
>>> first block X gets extent item inserted
>>> X+1 > X, insert block group item
>>> insert X+1 extent item.
>>>
>>
>> But then what if we go non-skinny metadata?
>>
>> Then block group item is always before any extent item.
>>
>
>          item 0 key (1048576 METADATA_ITEM 0) itemoff 16259 itemsize 24
>                  refs 1 gen 1 flags TREE_BLOCK
>                  tree block skinny level 0
>          item 1 key (1048576 TREE_BLOCK_REF 1) itemoff 16259 itemsize 0
>                  tree block backref
>          item 2 key (1048576 BLOCK_GROUP_ITEM 4194304) itemoff 16235
> itemsize 24
>                  block group used 98304 chunk_objectid 256 flags SYSTEM
>          item 3 key (1064960 METADATA_ITEM 0) itemoff 16211 itemsize 24
>                  refs 1 gen 1 flags TREE_BLOCK
>                  tree block skinny level 0
>
>
> This is what it looks like.  We're basing it off of the key.objectid. If
> the key.objectid's match, which they will, the block group will always
> be after it all.  It's doing the right thing.  Thanks,

Oh my bad memory.

Both EXTENT_ITEM and METADATA_ITEM are smaller than BLOCK_GROUP_ITEM.

So these extent items should always be before BLOCK_GROUP_ITEM, no
matter if the skinny metadata is spcififed.

Then the code is completely fine.

Thanks,
Qu
>
> Josef