Re: [PATCH 3/3] btrfs: handle active zone accounting properly

[Josef Bacik <josef@toxicpanda.com>]

On Thu, Mar 02, 2023 at 02:45:13PM +0000, Naohiro Aota wrote:
> On Thu, Mar 02, 2023 at 04:01:07PM +0900, Naohiro Aota wrote:
> > On Wed, Mar 01, 2023 at 04:14:44PM -0500, Josef Bacik wrote:
> > > Running xfstests on my ZNS drives uncovered a problem where I was
> > > allocating the entire device with metadata block groups.  The root cause
> > > of this was we would get ENOSPC on mount, and while trying to satisfy
> > > tickets we'd allocate more block groups.
> > > 
> > > The reason we were getting ENOSPC was because ->bytes_zone_unusable was
> > > set to 40gib, but ->active_total_bytes was set to 8gib, which was the
> > > maximum number of active block groups we're allowed to have at one time.
> > > This was because we always update ->bytes_zone_unusable with the
> > > unusable amount from every block group, but we only update
> > > ->active_total_bytes with the active block groups.
> > >
> > > This is actually much worse however, because we could potentially have
> > > other counters potentially well above the ->active_total_bytes, which
> > > would lead to this early enospc situation.
> > > 
> > > Fix this by mirroring the counters for active block groups into their
> > > own counters.  This allows us to keep the full space_info counters
> > > consistent, which is needed for things like sysfs and the space info
> > > ioctl, and then track the actual usage for ENOSPC reasons for only the
> > > active block groups.
> > 
> > I think the mirroring the counters approach duplicates the code and
> > variables and makes them complex. Instead, we can fix the
> > "active_total_bytes" accounting maybe like this:
> > 
> > diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> > index d4dd73c9a701..bf4d96d74efe 100644
> > --- a/fs/btrfs/space-info.c
> > +++ b/fs/btrfs/space-info.c
> > @@ -319,7 +319,8 @@ void btrfs_add_bg_to_space_info(struct btrfs_fs_info *info,
> >  	ASSERT(found);
> >  	spin_lock(&found->lock);
> >  	found->total_bytes += block_group->length;
> > -	if (test_bit(BLOCK_GROUP_FLAG_ZONE_IS_ACTIVE, &block_group->runtime_flags))
> > +	if (test_bit(BLOCK_GROUP_FLAG_ZONE_IS_ACTIVE, &block_group->runtime_flags) ||
> > +	    btrfs_zoned_bg_is_full(block_group))
> >  		found->active_total_bytes += block_group->length;
> >  	found->disk_total += block_group->length * factor;
> >  	found->bytes_used += block_group->used;
> > 
> > Or, we can remove "active_total_bytes" and introduce something like
> > "preactivated_bytes" to count the bytes of BGs never get activated (BGs #1 below).
> 
> I got a new idea. How about counting all the region in a new block group as
> zone_unusable? Then, region [0 .. zone_capacity] will be freed for use once
> it gets activated. With this, we can drop "active_total_bytes" so the code
> will become similar between the regular and the zoned mode.
> 
> We also need to care not to reclaim the fresh BGs but it's trivial
> (alloc_offset == 0).
> 

I like this idea better than mine for sure.  Will you write it up and send it
and I'll run it through my test-rig?  Thanks,

Josef