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

On Fri, 2026-08-21 at 07:43 +0930, Qu Wenruo wrote:
> 
> 在 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.
> 
> 

No. I don't have any evidence of btrfs_orphan_add() failing because of
-ENOMEM. I put those patches in because LLM review flagged it as a
potential issue, and Qu recommended that approach to fix it.

If we're content to leave that problem until we have evidence that it
is one, we can just drop patches 1 and 2. The later patches don't have
any dependencies on them.

Dave, would that resolve your concerns?

-ENOMEM abort analysis from the LLM for anyone interested:
---------------------------8<--------------------------

  btrfs_orphan_add aborts by errno:                                   
                                                                                                                  
  ┌────────────┬────────┬───────┐                                     
  │   errno    │ events │ hosts │                                     
  ├────────────┼────────┼───────┤                                     
  │ -5 EIO     │ 131    │ 127   │                                     
  ├────────────┼────────┼───────┤                                     
  │ -28 ENOSPC │ 1      │ 1     │                                     
  ├────────────┼────────┼───────┤                                     
  │ -12 ENOMEM │ 0      │ 0     │                                     
  └────────────┴────────┴───────┘                                     
  Same result with the cascade filter dropped (293 events / 270 hosts, all -5).                                   
                                                                                                                  
  Three controls make that a real negative rather than a query artifact:                           

  1. ENOMEM aborts exist — 475 events / 471 hosts in the same window, so the query would find them.
  2. The signature is detectable and the code is live — the identical abort site in btrfs_orphan_add: fires for 270 hosts with -5. And btrfs_orphan_add() passes ret straight to btrfs_abort_transaction() with no remapping, so a -12 would print as -12.                     
  3. The path is hot — in the base tree btrfs_orphan_add() is called from btrfs_unlink() (nlink→0), btrfs_rmdir(), btrfs_create_new_inode() (O_TMPFILE), and btrfs_rename() (victim nlink→0). That allocation runs on essentially every file deletion fleet-wide.                 

Where the ENOMEM aborts actually are (hosts/30d): btrfs_rename 147, convert_free_space_to_extents 116, btrfs_add_link 67, convert_free_space_to_bitmaps 56, btrfs_replace_file_extents 42, btrfs_create_new_inode 16.

-- 
Jeff Layton <[email protected]>