Re: [PATCH] btrfs: don't update the block group item if used bytes are the same

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

On 2022/7/11 16:30, Nikolay Borisov wrote:
>
>
> On 11.07.22 г. 9:37 ч., Qu Wenruo wrote:
>> When committing a transaction, we will update block group items for all
>> dirty block groups.
>>
>> But in fact, dirty block groups don't always need to update their block
>> group items.
>> It's pretty common to have a metadata block group which experienced
>> several CoW operations, but still have the same amount of used bytes.
>
> This could happen if for example the allocated/freed extents in a single
> transaction cancel each other out, right? Are there other cases where it
> could matter?

No need to completely cancel each other.

In fact, just COWing a path without adding/deleting new cousins would be
enough, and that would be very common for a lot of tree block operations.

>
>>
>> In that case, we may unnecessarily CoW a tree block doing nothing.
>>
>> This patch will introduce btrfs_block_group::commit_used member to
>> remember the last used bytes, and use that new member to skip
>> unnecessary block group item update.
>>
>> This would be more common for large fs, which metadata block group can
>> be as large as 1GiB, containing at most 64K metadata items.
>>
>> In that case, if CoW added and the deleted one metadata item near the end
>> of the block group, then it's completely possible we don't need to touch
>> the block group item at all.
>>
>> I don't have any benchmark to prove this, but this should not cause any
>> hurt either.
>
> It should not but adds more state and is overall a maintenance burden.
> One way to test this would be to rig up the fs to count how many times
> the optimization has been hit over the course of, say, a full xfstest
> run or at least demonstrate a particular workload where this makes
> tangible difference.

But in this particular case, there is really not that much status to bother.

In fact, we don't care about if there is any status, we only care about
the block_group::used is different from committed one.
Even no change to lock schemes.

Thanks,
Qu
>
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/block-group.c | 6 ++++++
>>   fs/btrfs/block-group.h | 6 ++++++
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
>> index 0148a6d719a4..5b08ac282ace 100644
>> --- a/fs/btrfs/block-group.c
>> +++ b/fs/btrfs/block-group.c
>> @@ -2024,6 +2024,7 @@ static int read_one_block_group(struct
>> btrfs_fs_info *info,
>>       cache->length = key->offset;
>>       cache->used = btrfs_stack_block_group_used(bgi);
>> +    cache->commit_used = cache->used;
>>       cache->flags = btrfs_stack_block_group_flags(bgi);
>>       cache->global_root_id =
>> btrfs_stack_block_group_chunk_objectid(bgi);
>> @@ -2724,6 +2725,10 @@ static int update_block_group_item(struct
>> btrfs_trans_handle *trans,
>>       struct btrfs_block_group_item bgi;
>>       struct btrfs_key key;
>> +    /* No change in used bytes, can safely skip it. */
>> +    if (cache->commit_used == cache->used)
>> +        return 0;
>> +
>>       key.objectid = cache->start;
>>       key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
>>       key.offset = cache->length;
>> @@ -2743,6 +2748,7 @@ static int update_block_group_item(struct
>> btrfs_trans_handle *trans,
>>       btrfs_set_stack_block_group_flags(&bgi, cache->flags);
>>       write_extent_buffer(leaf, &bgi, bi, sizeof(bgi));
>>       btrfs_mark_buffer_dirty(leaf);
>> +    cache->commit_used = cache->used;
>>   fail:
>>       btrfs_release_path(path);
>>       return ret;
>> diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
>> index 35e0e860cc0b..3f92b8eb9a05 100644
>> --- a/fs/btrfs/block-group.h
>> +++ b/fs/btrfs/block-group.h
>> @@ -74,6 +74,12 @@ struct btrfs_block_group {
>>       u64 cache_generation;
>>       u64 global_root_id;
>> +    /*
>> +     * The last committed used bytes of this block group, if above @used
>> +     * is still the same as @commit_used, we don't need to update block
>> +     * group item of this block group.
>> +     */
>> +    u64 commit_used;
>>       /*
>>        * If the free space extent count exceeds this number, convert
>> the block
>>        * group to bitmaps.