Re: [PATCH] btrfs: zoned: properly take lock to read/update BG's zoned variables

[Josef Bacik <josef@toxicpanda.com>]

On Mon, Jul 29, 2024 at 12:46:48PM +0100, Filipe Manana wrote:
> On Mon, Jul 29, 2024 at 9:33 AM Naohiro Aota <naohiro.aota@wdc.com> wrote:
> >
> > __btrfs_add_free_space_zoned() references and modifies BG's alloc_offset,
> > ro, and zone_unusable, but without taking the lock. It is mostly safe
> > because they monotonically increase (at least for now) and this function is
> > mostly called by a transaction commit, which is serialized by itself.
> >
> > Still, taking the lock is a safer and correct option and I'm going to add a
> > change to reset zone_unusable while a block group is still alive. So, add
> > locking around the operations.
> >
> > Fixes: 169e0da91a21 ("btrfs: zoned: track unusable bytes for zones")
> > CC: stable@vger.kernel.org # 5.15+
> > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> > ---
> >  fs/btrfs/free-space-cache.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> > index f5996a43db24..51263d6dac36 100644
> > --- a/fs/btrfs/free-space-cache.c
> > +++ b/fs/btrfs/free-space-cache.c
> > @@ -2697,15 +2697,16 @@ static int __btrfs_add_free_space_zoned(struct btrfs_block_group *block_group,
> >         u64 offset = bytenr - block_group->start;
> >         u64 to_free, to_unusable;
> >         int bg_reclaim_threshold = 0;
> > -       bool initial = ((size == block_group->length) && (block_group->alloc_offset == 0));
> > +       bool initial;
> >         u64 reclaimable_unusable;
> >
> > -       WARN_ON(!initial && offset + size > block_group->zone_capacity);
> > +       guard(spinlock)(&block_group->lock);
> 
> What's this guard thing and why do we use it only here? We don't use
> it anywhere else in btrfs' code base.
> A quick search in the Documentation directory of the kernel and I
> can't find anything there.
> In the fs/ directory there's only two users of it.
> 

It's relatively new, it's like the C++ guards.  If you look in the VFS we've
started using it pretty heavily there.

But it does need to be documented, if you look at include/linux/cleanup.h it has
documentation about it.

> Why not the usual spin_lock(&block_group->lock) call?

Because this is nice for error handling.  Here it doesn't look as helpful, but
look at d842379313a2 ("fs: use guard for namespace_sem in statmount()") for an
example of how much it cleans up the error paths.

FWIW one of the tasks I have for one of our new people is to come through and
utilize some of this new infrastructure to cleanup our error paths, so while it
doesn't exist yet inside btrfs, I hope it gets used pretty liberally.  Thanks,

Josef