Re: [PATCH 10/21] btrfs: return bool from lock_extent_buffer_for_io

[David Sterba <dsterba@suse.cz>]

On Wed, May 24, 2023 at 07:44:49AM +0200, Christoph Hellwig wrote:
> On Tue, May 23, 2023 at 08:43:17PM +0200, David Sterba wrote:
> > On Wed, May 03, 2023 at 05:24:30PM +0200, Christoph Hellwig wrote:
> > > lock_extent_buffer_for_io never returns a negative error value, so switch
> > > the return value to a simple bool.  Also remove the noinline_for_stack
> > > annotation given that nothing in lock_extent_buffer_for_io or its callers
> > > is particularly stack hungry.
> > 
> > AFAIK the reason for noinline_for_stack is not because of a function is
> > stack hungry but because we want to prevent inlining it so we can see it
> > on stack in case there's an implied waiting. This makes it easier to
> > debug when IO is stuck or there's some deadlock.
> > 
> > This is not consistent in btrfs code though, quick search shows lots of
> > historical noinline_for_stack everywhere without an obvious reason.
> 
> Hmm.  noinline_for_stack is explicitly documented to only exist as an
> annotation that noinline is used for stack usage.  So this is very odd.

Yes that's the documented way, I found one commit fixing the stack
problem, 8ddc319706e5 ("btrfs: reduce stack usage for
btrfsic_process_written_block").

> If you want a normal noinline here I can add one, but to be honest
> I don't really see the point even for stack traces.

What I had in mind was based on 6939f667247e ("Btrfs: fix confusing
worker helper info in stacktrace"), but digging in the mail archives the
patch was sent with noinline
(https://lore.kernel.org/linux-btrfs/20170908213445.1601-1-bo.li.liu@oracle.com/).
I don't remember where I read about the noinline_for_stack use for the
stack trace.

We can audit and remove some of the attributes but this tends to break
only on non-x86 builds so verification would need to go via linux-next
and let build bots report.