Re: [PATCH v2 3/5] btrfs: pre-allocate delayed dir index before btree modification

[Qu Wenruo <wqu@suse.com>]

在 2026/8/5 01:14, Jeff Layton 写道:
> Move the delayed dir index allocation in btrfs_insert_dir_item() before
> the insert_with_overflow() call that modifies the btree. Previously, the
> allocations happened after the DIR_ITEM was already inserted, meaning an
> ENOMEM failure left the btree in a partially-modified state that could
> only be resolved by aborting the transaction.
> 
> Add an optional caller-provided btrfs_dir_index_prealloc parameter to
> btrfs_insert_dir_item(). When non-NULL, ownership of the prealloc
> transfers to btrfs_insert_dir_item(). When NULL, it allocates internally.
> All existing callers pass NULL to preserve the current behavior.
> 
> Remove the btrfs_insert_delayed_dir_index() wrapper, as there are no
> more callers.
> 
> Assisted-by: LLM
> Suggested-by: Qu Wenruo <wqu@suse.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/btrfs/delayed-inode.c | 21 ---------------------
>   fs/btrfs/delayed-inode.h |  5 -----
>   fs/btrfs/dir-item.c      | 30 ++++++++++++++++++++++++------
>   fs/btrfs/dir-item.h      |  5 +++--
>   fs/btrfs/inode.c         |  2 +-
>   fs/btrfs/transaction.c   |  2 +-
>   6 files changed, 29 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index 95d2dca80444..d9de7f269874 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -1608,27 +1608,6 @@ int btrfs_insert_delayed_dir_index_prealloc(struct btrfs_trans_handle *trans,
>   	return ret;
>   }
>   
> -/* Will return 0, -ENOMEM or -EEXIST (index number collision, unexpected). */
> -int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
> -				   const char *name, int name_len,
> -				   struct btrfs_inode *dir,
> -				   const struct btrfs_disk_key *disk_key, u8 flags,
> -				   u64 index)
> -{
> -	struct btrfs_dir_index_prealloc prealloc;
> -	int ret;
> -
> -	ret = btrfs_prealloc_delayed_dir_index(dir, name, name_len, &prealloc);
> -	if (ret)
> -		return ret;
> -
> -	memcpy(prealloc.item->data + sizeof(struct btrfs_dir_item), name,
> -	       name_len);
> -
> -	return btrfs_insert_delayed_dir_index_prealloc(trans, dir, &prealloc,
> -						       disk_key, flags, index);
> -}
> -
>   static bool btrfs_delete_delayed_insertion_item(struct btrfs_delayed_node *node,
>   						u64 index)
>   {
> diff --git a/fs/btrfs/delayed-inode.h b/fs/btrfs/delayed-inode.h
> index e310a257c9a6..878d70aee2f9 100644
> --- a/fs/btrfs/delayed-inode.h
> +++ b/fs/btrfs/delayed-inode.h
> @@ -115,11 +115,6 @@ struct btrfs_delayed_item {
>   };
>   
>   void btrfs_init_delayed_root(struct btrfs_delayed_root *delayed_root);
> -int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
> -				   const char *name, int name_len,
> -				   struct btrfs_inode *dir,
> -				   const struct btrfs_disk_key *disk_key, u8 flags,
> -				   u64 index);
>   
>   struct btrfs_dir_index_prealloc {
>   	struct btrfs_delayed_node *node;
> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
> index 84f1c64423d3..1b956df2c571 100644
> --- a/fs/btrfs/dir-item.c
> +++ b/fs/btrfs/dir-item.c
> @@ -106,8 +106,11 @@ int btrfs_insert_xattr_item(struct btrfs_trans_handle *trans,
>    * Will return 0 or -ENOMEM
>    */
>   int btrfs_insert_dir_item(struct btrfs_trans_handle *trans,
> -			  const struct fscrypt_str *name, struct btrfs_inode *dir,
> -			  const struct btrfs_key *location, u8 type, u64 index)
> +			  const struct fscrypt_str *name,
> +			  struct btrfs_inode *dir,
> +			  const struct btrfs_key *location, u8 type,
> +			  u64 index,
> +			  struct btrfs_dir_index_prealloc *prealloc)
>   {
>   	int ret = 0;
>   	int ret2 = 0;
> @@ -119,6 +122,8 @@ int btrfs_insert_dir_item(struct btrfs_trans_handle *trans,
>   	struct btrfs_key key;
>   	struct btrfs_disk_key disk_key;
>   	u32 data_size;
> +	const bool need_delayed_index = (root != root->fs_info->tree_root);
> +	struct btrfs_dir_index_prealloc local_prealloc;
>   
>   	key.objectid = btrfs_ino(dir);
>   	key.type = BTRFS_DIR_ITEM_KEY;
> @@ -130,6 +135,18 @@ int btrfs_insert_dir_item(struct btrfs_trans_handle *trans,
>   
>   	btrfs_cpu_key_to_disk(&disk_key, location);
>   
> +	/* Pre-allocate the delayed dir index before modifying the btree. */
> +	if (need_delayed_index && !prealloc) {

This is exposed by sashiko.

If we have @prealloc passed in, and before we even hit 
insert_with_overflow(), the previous btrfs_alloc_path() failed, we 
return -ENOMEM directly, leaking the @prealloc.


> +		ret = btrfs_prealloc_delayed_dir_index(dir, name->name,
> +						       name->len,
> +						       &local_prealloc);

Sashiko also pointed out that, the preallocation itself doesn't really 
utilize name->name.

Thus it may be a good idea to merge the later memcpy() into the 
preallocation function.

Thanks,
Qu

> +		if (ret)
> +			return ret;
> +		memcpy(local_prealloc.item->data + sizeof(struct btrfs_dir_item),
> +		       name->name, name->len);
> +		prealloc = &local_prealloc;
> +	}
> +
>   	data_size = sizeof(*dir_item) + name->len;
>   	dir_item = insert_with_overflow(trans, root, path, &key, data_size,
>   					name->name, name->len);
> @@ -137,6 +154,8 @@ int btrfs_insert_dir_item(struct btrfs_trans_handle *trans,
>   		ret = PTR_ERR(dir_item);
>   		if (ret == -EEXIST)
>   			goto second_insert;
> +		if (need_delayed_index)
> +			btrfs_free_delayed_dir_index_prealloc(trans, prealloc);
>   		goto out_free;
>   	}
>   
> @@ -154,15 +173,14 @@ int btrfs_insert_dir_item(struct btrfs_trans_handle *trans,
>   	write_extent_buffer(leaf, name->name, name_ptr, name->len);
>   
>   second_insert:
> -	/* FIXME, use some real flag for selecting the extra index */
> -	if (root == root->fs_info->tree_root) {
> +	if (!need_delayed_index) {
>   		ret = 0;
>   		goto out_free;
>   	}
>   	btrfs_release_path(path);
>   
> -	ret2 = btrfs_insert_delayed_dir_index(trans, name->name, name->len, dir,
> -					      &disk_key, type, index);
> +	ret2 = btrfs_insert_delayed_dir_index_prealloc(trans, dir, prealloc,
> +						       &disk_key, type, index);
>   out_free:
>   	if (ret)
>   		return ret;
> diff --git a/fs/btrfs/dir-item.h b/fs/btrfs/dir-item.h
> index e52174a8baf9..d7a7d0b66f37 100644
> --- a/fs/btrfs/dir-item.h
> +++ b/fs/btrfs/dir-item.h
> @@ -16,9 +16,11 @@ struct btrfs_trans_handle;
>   
>   int btrfs_check_dir_item_collision(struct btrfs_root *root, u64 dir_ino,
>   			  const struct fscrypt_str *name);
> +struct btrfs_dir_index_prealloc;
>   int btrfs_insert_dir_item(struct btrfs_trans_handle *trans,
>   			  const struct fscrypt_str *name, struct btrfs_inode *dir,
> -			  const struct btrfs_key *location, u8 type, u64 index);
> +			  const struct btrfs_key *location, u8 type, u64 index,
> +			  struct btrfs_dir_index_prealloc *prealloc);
>   struct btrfs_dir_item *btrfs_lookup_dir_item(struct btrfs_trans_handle *trans,
>   					     struct btrfs_root *root,
>   					     struct btrfs_path *path, u64 dir,
> @@ -53,5 +55,4 @@ static inline u64 btrfs_name_hash(const char *name, int len)
>   {
>          return crc32c((u32)~1, name, len);
>   }
> -
>   #endif
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 3c10a0ef0002..3a2dca093c7d 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6924,7 +6924,7 @@ int btrfs_add_link(struct btrfs_trans_handle *trans,
>   		return ret;
>   
>   	ret = btrfs_insert_dir_item(trans, name, parent_inode, &key,
> -				    btrfs_inode_type(inode), index);
> +				    btrfs_inode_type(inode), index, NULL);
>   	if (ret == -EEXIST || ret == -EOVERFLOW)
>   		goto fail_dir_item;
>   	else if (unlikely(ret)) {
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index c641099d66e2..6fdfea5d35af 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1882,7 +1882,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
>   
>   	ret = btrfs_insert_dir_item(trans, &fname.disk_name,
>   				    parent_inode, &key, BTRFS_FT_DIR,
> -				    index);
> +				    index, NULL);
>   	if (unlikely(ret)) {
>   		btrfs_abort_transaction(trans, ret);
>   		goto fail;
>