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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2026/8/21 07:43, 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.
For now I have removed those from for-next, but I really hope you can 
come up with a consistent behavior on on-stack memory usage.