Re: [PATCH 3/4] btrfs: handle ENOMEM from btrfs_insert_dir_item() without aborting

[Qu Wenruo <wqu@suse.com>]

在 2026/7/18 09:25, Jeff Layton 写道:
> On Fri, 2026-07-17 at 16:04 -0700, Boris Burkov wrote:
>> On Fri, Jul 17, 2026 at 06:40:42PM -0400, Jeff Layton wrote:
>>> On Fri, 2026-07-17 at 13:18 -0700, Boris Burkov wrote:
>>>> On Fri, Jul 17, 2026 at 12:52:38PM -0400, Jeff Layton wrote:
>>>>> Now that btrfs_insert_dir_item() returns -ENOMEM before modifying the
>>>>> btree (thanks to delayed dir index pre-allocation), callers can handle
>>>>> ENOMEM gracefully instead of aborting the transaction.
>>>>>
>>>>> In btrfs_add_link(), add -ENOMEM to the set of recoverable errors
>>>>> alongside -EEXIST and -EOVERFLOW. The fail_dir_item cleanup path
>>>>> unwinds the inode_ref/root_ref and returns the error to userspace.
>>>>>
>>>>> In btrfs_create_new_inode(), when btrfs_add_link() fails with -ENOMEM,
>>>>> convert the newly-created inode into an orphan instead of aborting.
>>>>> This is done by clearing nlink and adding an orphan item, which ensures
>>>>> btrfs_evict_inode() will delete the INODE_ITEM and INODE_REF, and
>>>>> crash-recovery will clean it up via orphan processing. If
>>>>> btrfs_orphan_add() itself fails, we fall back to aborting.
>>>>>
>>>>> This turns a filesystem-killing transaction abort into a graceful
>>>>> -ENOMEM return to userspace for create(), mkdir(), mknod(), symlink(),
>>>>> and link() operations under memory pressure.
>>>>>
>>>>> Assisted-by: LLM
>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>>> ---
>>>>>   fs/btrfs/inode.c | 17 +++++++++++++++--
>>>>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>>>> index b7b4e6177135..4d9947ae08f7 100644
>>>>> --- a/fs/btrfs/inode.c
>>>>> +++ b/fs/btrfs/inode.c
>>>>> @@ -6676,7 +6676,20 @@ int btrfs_create_new_inode(struct btrfs_trans_handle *trans,
>>>>>   	} else {
>>>>>   		ret = btrfs_add_link(trans, BTRFS_I(dir), BTRFS_I(inode), name,
>>>>>   				     false, BTRFS_I(inode)->dir_index);
>>>>> -		if (unlikely(ret)) {
>>>>> +		if (ret == -ENOMEM) {
>>>>> +			/*
>>>>> +			 * The ENOMEM came before the DIR_ITEM was inserted,
>>>>> +			 * so the btree has our INODE_ITEM + INODE_REF but no
>>>>> +			 * directory entry. Convert this into an orphan so
>>>>> +			 * eviction (or crash-recovery) cleans up the inode.
>>>>> +			 */
>>>>> +			clear_nlink(inode);
>>>>> +			ret = btrfs_orphan_add(trans, BTRFS_I(inode));
>>>>> +			if (unlikely(ret))
>>>>> +				btrfs_abort_transaction(trans, ret);
>>>>
>>>> I feel like the crux of this series to me is whether you have practical
>>>> conditions where the allocation of the delayed_node is failing, but the
>>>> allocations involved in btrfs_orphan_add() succeed. It allocates a
>>>> btrfs_path and has to walk the btree which might have to read the node
>>>> at every level which might need to allocate 16k extent buffers and
>>>> extent buffer objects and xarray storage for each one. For size
>>>> reference, on my build (maybe debug..?) a delayed_node is 552 bytes,
>>>> while a btrfs_path is 112 and an extent_buffer is 432. So they are
>>>> pretty similar in size (not to mention the 16k of node file backed
>>>> memory we are sort of likely to have to allocate if we are under
>>>> reclaim)
>>>>
>>>> Were you able to reproduce this issue and help in practice or is this a
>>>> theoretical / structural improvement?
>>>>
>>>
>>> I didn't really try to reproduce this in earnest. We only see it in our
>>> fleet under heavy memory pressure, and even then at such low frequency,
>>> I doubt our chances of hitting this on anything other than a huge set
>>> of machines.
>>>
>>> So, theoretical / structural, but we have record of filesystem aborts
>>> where the stack indicates that this would have prevented it. Userland
>>> would have gotten an -ENOMEM back but the fs wouldn't have aborted.
>>>
>>
>> My concern is not that we don't hit ENOMEM in btrfs_add_link(), since
>> like you said we can observe that in abort logs. I am worried that even
>> if we try to handle it gracefully, we will just ENOMEM in
>> btrfs_orphan_add() and abort anyway. That is why I was wanting to see
>> some more concrete evidence this actually helps to make it worth the
>> complexity.
>>
> 
> In the case where we handle this gracefully, we won't hit that because
> it will have returned -ENOMEM before that point. But, you do have a
> good point that we could allocate these objects successfully, and then
> hit an error in btrfs_orphan_add() anyway.
> 
> One thought: It looks like the main allocation in that codepath is
> btrfs_alloc_path()? We could consider preallocating that too -- maybe
> stash it in a new pointer in btrfs_trans_handle?

For btrfs_alloc_path(), you can just use on-stack memory for that.

That's the practice we utilize for btrfs-progs, and it's still 
acceptable for kernel, since that structure is only 112 bytes.

We should not switch all btrfs_path to on-stack ones, but for critical 
ones that memory allocation can lead to trans abort, I'd say it's 
definitely worth the extra on-stack memory usage.

Thanks,
Qu