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 03:48:39PM +0100, Filipe Manana wrote:
> On Mon, Jul 29, 2024 at 3:41 PM Josef Bacik <josef@toxicpanda.com> wrote:
> >
> > 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,
> 
> I see, I figured it was something to avoid repeating unlock calls.
> 
> But rather than fixing a bug and doing the migration in a single
> patch, I'd rather have 1 patch to fix the bug and another to the
> migration afterwards.
> 

Fair, agreed,

Josef