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

[David Sterba <dsterba@suse.cz>]

On Mon, Jul 11, 2022 at 04:47:25PM +0800, Qu Wenruo wrote:
> 
> 
> 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.

I would be interested in numbers too, a percentage of skip/total would
be good for some workloads so we have at least some idea.

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

Looking to update_block_group_item, there are other block grup items
updated too:

- used - the one one you check
- flags - can't change on the fly, we'd have to do relocation, ie. a new
  block group
- chunk_objectid - that stays for the whole fileystem lifetime

So I think it's safe to just check the 'used' value but it increases
memory size of block group (that's been increasing in size recently) so
I'd rather have a better idea about the justification.