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

[David Sterba <dsterba@suse.cz>]

On Mon, Jul 29, 2024 at 10:41:50AM -0400, Josef Bacik 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:
> > 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.

I don't find this commit as a good example where guard improves things.
The code is factored to a helper do_statmount() and the guard is used as

	scoped_guard(rwsem_read, &lock)
		do_statmount();

With the rwsem this would be

	down_read(&lock);
	do_statmount();
	up_read(&lock);

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

In some cases the guard() can improve the error handling, namely when
there are several branches and each of them ends with an unlock. But I
think this is rare.

The scoped_guard() can be useful more often but adds an indentation and
obscures that it's locking.

It can also make things worse when the scope is unnecessarily long,
covering function calls that don't need the protection but can take time
and keep the lock held. Like kfree(), "put structure" or similar cleanup
work.