Message-ID: <20251113112514.GN13846@suse.cz>
Date: Thu, 13 Nov 2025 12:25:14 +0100
From: David Sterba <dsterba@...e.cz>
To: Gladyshev Ilya <foxido@...ido.dev>
Cc: David Sterba <dsterba@...e.com>, neelx@...e.com,
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 4/8] btrfs: simplify function protections with guards

On Thu, Nov 13, 2025 at 01:06:42PM +0300, Gladyshev Ilya wrote:
> On 11/13/25 11:43, David Sterba wrote:
> > On Wed, Nov 12, 2025 at 09:49:40PM +0300, Gladyshev Ilya wrote:
> >> Replaces cases like
> >>
> >> void foo() {
> >>      spin_lock(&lock);
> >>      ... some code ...
> >>      spin_unlock(&lock)
> >> }
> >>
> >> with
> >>
> >> void foo() {
> >>      guard(spinlock)(&lock);
> >>      ... some code ...
> >> }
> >>
> >> While it doesn't has any measurable impact,
> > 
> > There is impact, as Daniel mentioned elsewhere, this also adds the
> > variable on stack. It's measuable on the stack meter, one variable is
> > "nothing" but combined wherever the guards are used can add up. We don't
> > mind adding variables where needed, occasional cleanups and stack
> > reduction is done. Here it's a systematic stack use increase, not a
> > reason to reject the guards but still something I cannot just brush off
> I thought it would be optimized out by the compiler in the end, I will 
> probably recheck this

There are cases where compiler will optimize it out, IIRC when the lock
is embedded in a structure, and not when the pointer is dereferenced.

> >> it makes clear that whole
> >> function body is protected under lock and removes future errors with
> >> additional cleanup paths.
> > 
> > The pattern above is the one I find problematic the most, namely in
> > longer functions or evolved code. Using your example as starting point
> > a followup change adds code outside of the locked section:
> > 
> > void foo() {
> >      spin_lock(&lock);
> >      ... some code ...
> >      spin_unlock(&lock)
> > 
> >      new code;
> > }
> > 
> > with
> > 
> > void foo() {
> >      guard(spinlock)(&lock);
> >      ... some code ...
> > 
> >      new code;
> > }
> > 
> > This will lead to longer critical sections or even incorrect code
> > regarding locking when new code must not run under lock.
> > 
> > The fix is to convert it to scoped locking, with indentation and code
> > churn to unrelated code to the new one.
> > 
> > Suggestions like refactoring to make minimal helpers and functions is
> > another unecessary churn and breaks code reading flow.
> 
> What if something like C++ unique_lock existed? So code like this would 
> be possible:
> 
> void foo() {
>    GUARD = unique_lock(&spin);
> 
>    if (a)
>      // No unlocking required -> it will be done automatically
>      return;
> 
>    unlock(GUARD);
> 
>    ... unlocked code ...
> 
>    // Guard undestands that it's unlocked and does nothing
>    return;
> }
> 
> It has similar semantics to scoped_guard [however it has weaker 
> protection -- goto from locked section can bypass `unlock` and hold lock 
> until return], but it doesn't introduce diff noise correlated with 
> indentations.
> 
> Another approach is allowing scoped_guards to have different indentation 
> codestyle to avoid indentation of internal block [like goto labels for 
> example].
> 
> However both of this approaches has their own downsides and are pure 
> proposals

Thanks, it's good to have concrete examples to discuss. It feels like
we'd switch away from C and force patterns that are maybe common in
other languages like C++ that do things under the hood and it's fine
there as it's the mental programming model one has. This feels alien to
kernel C programming, namely for locking where the "hide things" is IMO
worse than "make things explicit".

There are cases where switching to guards would be reasonable because
the functions are short and simple but then it does not utilize the
semantics of automatic cleanup. In more complex functions it would mean
to make more structural changes to the code at the cost of churn and
merge conflicts of backported patches.

Powered by blists - more mailing lists