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

[David Sterba <dsterba@suse.cz>]

On Wed, Sep 07, 2022 at 01:29:49PM -0400, 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,

Thanks for tracking it down, I've removed the patch from for-next, it's
an optimization and we haven't seen any numbers yet how useful it is.