linux-kernel - Re: [RFC PATCH 7/8] btrfs: simplify return path via cleanup.h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPjX3FcAE--WHP78jvpXy-aBUEmX9e3FK=F68v-f8sPJbi+CTw@mail.gmail.com>
Date: Thu, 13 Nov 2025 13:48:10 +0100
From: Daniel Vacek <neelx@...e.com>
To: dsterba@...e.cz
Cc: Qu Wenruo <quwenruo.btrfs@....com>, Gladyshev Ilya <foxido@...ido.dev>, Chris Mason <clm@...com>, 
	David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 7/8] btrfs: simplify return path via cleanup.h

On Thu, 13 Nov 2025 at 09:59, David Sterba <dsterba@...e.cz> wrote:
>
> On Thu, Nov 13, 2025 at 07:20:01AM +1030, Qu Wenruo wrote:
> > > --- a/fs/btrfs/extent-tree.c
> > > +++ b/fs/btrfs/extent-tree.c
> > > @@ -1878,16 +1878,14 @@ static int cleanup_ref_head(struct btrfs_trans_handle *trans,
> > >      * and then re-check to make sure nobody got added.
> > >      */
> > >     spin_unlock(&head->lock);
> > > -   spin_lock(&delayed_refs->lock);
> > > -   spin_lock(&head->lock);
> > > -   if (!RB_EMPTY_ROOT(&head->ref_tree.rb_root) || head->extent_op) {
> > > -           spin_unlock(&head->lock);
> > > -           spin_unlock(&delayed_refs->lock);
> > > -           return 1;
> > > +   {
> >
> > There are some internal discussion about such anonymous code block usage.
> >
> > Although I support such usage, especially when it can reduce the
> > lifespan of local variables, it's not a commonly accepted pattern yet.
>
> And the discussion is going great, I think we wont't find a consensus
> without somebody either missing a coding pattern (you) or suffering to
> look at such code each time (me). Others have similar mixed feelings
> about the guards use.

And yet I can imagine even wilder creativity like:

> +     scoped_guard(spinlock, &delayed_refs->lock)
> +     scoped_guard(spinlock, &head->lock) {
> +             if (!RB_EMPTY_ROOT(&head->ref_tree.rb_root) || head->extent_op)
> +                     return 1;
> +             btrfs_delete_ref_head(fs_info, delayed_refs, head);
>       }

Here the indentation is irregular, but still looks kind of just. Would
we be happy with such exceptions?

Otherwise this could end up rather mixed and that does not look
preferable, at least to me:

> +     scoped_guard(spinlock, &delayed_refs->lock) {
> +             guard(spinlock)(&head->lock)
> +             if (!RB_EMPTY_ROOT(&head->ref_tree.rb_root) || head->extent_op)
> +                     return 1;
> +             btrfs_delete_ref_head(fs_info, delayed_refs, head);
>       }