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

[Josef Bacik <josef@toxicpanda.com>]

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,

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;