Re: [PATCH v3 1/6] btrfs: use an on-stack path in btrfs_insert_orphan_item()

[Qu Wenruo]

在 2026/8/20 21:34, David Sterba 写道:
> On Tue, Aug 11, 2026 at 02:14:54PM -0400, Jeff Layton wrote:
> > btrfs_insert_orphan_item() allocated a btrfs_path with btrfs_alloc_path()
> > which returns -ENOMEM on failure. It is called from btrfs_orphan_add(),
> > so a path allocation failure there turns a recoverable error into a
> > transaction abort.
> >
> > btrfs_path is only ~112 bytes, so allocate it on the stack instead.
>
> 112 is too much for on-stack, we've avoided that for btrfs_path in
> particular, except some justified cases. This means in general the
> beginning of call stack like ioctl, syscall handler and such. Otherwise
> we assume there are other layers in the IO stack, like block device
> drivers (DM), NFS, encoding layers or networking (iscsi), and obviously
> the lowest level device drivers.

I think you're very inconsistent on on-stack memory usage at least.

You were fine when I was adding 128bytes for several call sites for the 
support of huge pages, and I'd argue all those call sites have a deeper 
stack, because it's on the writeback path.

Furthermore, that huge page support is not widely used, but everyone 
will need to pay that on-stack price.

On the other hand, you are also very hesitant on my recent patches 
removing those 128 bytes usages.

So your behavior doesn't seem to match what you said here.


Secondly, your deep-in-the-stack argument doesn't sound solid either.

Every block file system can be built upon layer of storage stacks, not 
only btrfs, but *every* block fs as long as there is a chance to do IO.
This means you're just saying, there can be almost-infinite lower layers 
under us, so we should not use any extra on-stack memory.

I do not think this is the sane nor really validated.

If you want to argue if the extra 112 bytes is good or not, give me some 
data about the on-stack memory usage.
With the proof that with enough stacked dm layer, that extra 112 bytes 
are going to cause problem.

Not to mention I believe some dm drivers are queuing the real submission 
handling into a workqueue, avoiding further increasing the on-stack 
memory usage.

> The trade off with possible allocation failure vs stack consumption
> needs to be argued in the changelog, "is just 112" is not sufficient.

Although I agree that more changelog will help, especially if Jeff has a 
good call trace showing that this is causing a flip RO in meta's fleet.

> Getting back the consumed stack space is painful, we've been reducing
> unneeded or redundant parameters of functions for years. The gains are
> like -8 bytes here and -8 bytes there, allocation of +112 wipes that out.
>
> If the place of allocation is critical we can consider that but we have
> too many of them, anywhere during the transaction commit path or
> irreversible metadata changes. Possibly using __GFP_HIGH could work, but
> I haven't explored that.
>
> Qu added the patches to for-next but I had no chance to look closely at
> this patchset yet and am hesitant to leave it like that.