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 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.

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;