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

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

On 2022/9/8 06:20, Qu Wenruo wrote:
>
>
> On 2022/9/8 01:29, Josef Bacik wrote:
>> On Wed, Sep 07, 2022 at 10:31:58AM -0400, Josef Bacik wrote:
>>> On Mon, Jul 11, 2022 at 02:37:52PM +0800, 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.
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>
>>> I've been seeing random btrfs check failures on our overnight testing
>>> since this
>>> patch was merged.  I can't blame it directly yet, I've mostly seen it on
>>> TEST_DEV, and once while running generic/648.  I'm running it in a
>>> loop now to
>>> reproduce and then fix it.
>>>
>>> We can start updating block groups before we're in the critical
>>> section, so we
>>> can update block_group->bytes_used while we're updating the block
>>> group item in
>>> a different thread.  So if we set the block_group item to some value of
>>> bytes_used, then update it in another thread, and then set
>>> ->commit_used to the
>>> new value we'll fail to update the block group item with the correct
>>> value
>>> later.
>>>
>>> We need to wrap this bit in the block_group->lock to avoid this
>>> particular
>>> problem.  Once I reproduce and validate the fix I'll send that, but I
>>> wanted to
>>> reply in case that takes longer than I expect.  Thanks,
>>
>> Ok this is in fact the problem, this fixup made the problem go away.
>> Thanks,
>
> This fix means, a bg members can change even we are at
> update_block_group_item().
>
> The old code is completely relying on the one time access on cache->used.
>
> Anyway thanks for the fix.

So this is only happening if we execute
btrfs_start_dirty_block_groups(), which unlike
btrfs_write_dirty_block_groups(), is not yet protected by transaction
critical path.

Thus we can have bg members changing halfway and caused the race.


To David, do I need to send a updated version with extra comments on this?

Thanks,
Qu

>
> Thanks,
> Qu
>>
>> Josef
>>
>> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
>> index 6e7bb1c0352d..1e2773b120d4 100644
>> --- a/fs/btrfs/block-group.c
>> +++ b/fs/btrfs/block-group.c
>> @@ -2694,10 +2694,16 @@ static int update_block_group_item(struct
>> btrfs_trans_handle *trans,
>>       struct extent_buffer *leaf;
>>       struct btrfs_block_group_item bgi;
>>       struct btrfs_key key;
>> +    u64 used;
>>
>>       /* No change in used bytes, can safely skip it. */
>> -    if (cache->commit_used == cache->used)
>> +    spin_lock(&cache->lock);
>> +    used = cache->used;
>> +    if (cache->commit_used == used) {
>> +        spin_unlock(&cache->lock);
>>           return 0;
>> +    }
>> +    spin_unlock(&cache->lock);
>>
>>       key.objectid = cache->start;
>>       key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
>> @@ -2712,13 +2718,14 @@ static int update_block_group_item(struct
>> btrfs_trans_handle *trans,
>>
>>       leaf = path->nodes[0];
>>       bi = btrfs_item_ptr_offset(leaf, path->slots[0]);
>> -    btrfs_set_stack_block_group_used(&bgi, cache->used);
>> +
>> +    btrfs_set_stack_block_group_used(&bgi, used);
>>       btrfs_set_stack_block_group_chunk_objectid(&bgi,
>>                              cache->global_root_id);
>>       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;
>> +    cache->commit_used = used;
>>   fail:
>>       btrfs_release_path(path);
>>       return ret;