[PATCH 0/2] btrfs: updates to error path for delayed dir index insertion failure

[PATCH 0/2] btrfs: updates to error path for delayed dir index insertion failure

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Some updates to the error path for delayed dir index insertion failure,
motivated by a recent syzbot report:

  https://lore.kernel.org/linux-btrfs/00000000000036e1290603e097e0@google.com/

Details in the changelogs.

Filipe Manana (2):
  btrfs: improve error message after failure to add delayed dir index item
  btrfs: remove BUG() after failure to insert delayed dir index item

 fs/btrfs/delayed-inode.c | 71 ++++++++++++++++++++++++++--------------
 1 file changed, 46 insertions(+), 25 deletions(-)

-- 
2.40.1

[PATCH 1/2] btrfs: improve error message after failure to add delayed dir index item

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If we fail to add a delayed dir index item because there's already another
item with the same index number, we print an error message (and then BUG).
However that message isn't very helpful to debug anything because we don't
know what's the index number and what are the values of index counters in
the inode and its delayed inode (index_cnt fields of struct btrfs_inode
and struct btrfs_delayed_node).

So update the error message to include the index number and counters.

We actually had a recent case where this issue was hit by a syzbot report
(see the link below).

Link: https://lore.kernel.org/linux-btrfs/00000000000036e1290603e097e0@google.com/
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/delayed-inode.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 08ecb4d0cc45..f9dae729811b 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1498,9 +1498,10 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
 	ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
 	if (unlikely(ret)) {
 		btrfs_err(trans->fs_info,
-			  "err add delayed dir index item(name: %.*s) into the insertion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)",
-			  name_len, name, delayed_node->root->root_key.objectid,
-			  delayed_node->inode_id, ret);
+"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
+			  name_len, name, index, btrfs_root_id(delayed_node->root),
+			  delayed_node->inode_id, dir->index_cnt,
+			  delayed_node->index_cnt, ret);
 		BUG();
 	}
 	mutex_unlock(&delayed_node->mutex);
-- 
2.40.1

[PATCH 2/2] btrfs: remove BUG() after failure to insert delayed dir index item

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Instead of calling BUG() when we fail to insert a delayed dir index item
into the delayed node's tree, we can just release all the resources we
have allocated/acquired before and return the error to the caller. This is
fine because all existing call chains undo anything they have done before
calling btrfs_insert_delayed_dir_index() or BUG_ON (when creating pending
snapshots in the transaction commit path).

So remove the BUG() call and do proper error handling.

This relates to a syzbot report linked below, but does not fix it because
it only prevents hitting a BUG(), it does not fix the issue where somehow
we attempt to use twice the same index number for different index items.

Link: https://lore.kernel.org/linux-btrfs/00000000000036e1290603e097e0@google.com/
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/delayed-inode.c | 72 +++++++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index f9dae729811b..5da7442a1d96 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1413,7 +1413,29 @@ void btrfs_balance_delayed_items(struct btrfs_fs_info *fs_info)
 	btrfs_wq_run_delayed_node(delayed_root, fs_info, BTRFS_DELAYED_BATCH);
 }
 
-/* Will return 0 or -ENOMEM */
+static void btrfs_release_dir_index_item_space(struct btrfs_trans_handle *trans)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	const u64 bytes = btrfs_calc_insert_metadata_size(fs_info, 1);
+
+	if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags))
+		return;
+
+	/*
+	 * Adding the new dir index item does not require touching another
+	 * leaf, so we can release 1 unit of metadata that was previously
+	 * reserved when starting the transaction. This applies only to
+	 * the case where we had a transaction start and excludes the
+	 * transaction join case (when replaying log trees).
+	 */
+	trace_btrfs_space_reservation(fs_info, "transaction",
+				      trans->transid, bytes, 0);
+	btrfs_block_rsv_release(fs_info, trans->block_rsv, bytes, NULL);
+	ASSERT(trans->bytes_reserved >= bytes);
+	trans->bytes_reserved -= bytes;
+}
+
+/* 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,
@@ -1455,6 +1477,27 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
 
 	mutex_lock(&delayed_node->mutex);
 
+	/*
+	 * First attempt to insert the delayed item. This is to make the error
+	 * handling path simpler in case we fail (-EEXIST). There's no risk of
+	 * any other task coming in and running the delayed item before we do
+	 * the metadata space reservation below, because we are holding the
+	 * delayed node's mutex and that mutex must also be locked before the
+	 * node's delayed items can be run.
+	 */
+	ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
+	if (unlikely(ret)) {
+		btrfs_err(trans->fs_info,
+"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
+			  name_len, name, index, btrfs_root_id(delayed_node->root),
+			  delayed_node->inode_id, dir->index_cnt,
+			  delayed_node->index_cnt, ret);
+		mutex_unlock(&delayed_node->mutex);
+		btrfs_release_delayed_item(delayed_item);
+		btrfs_release_dir_index_item_space(trans);
+		goto release_node;
+	}
+
 	if (delayed_node->index_item_leaves == 0 ||
 	    delayed_node->curr_index_batch_size + data_len > leaf_data_size) {
 		delayed_node->curr_index_batch_size = data_len;
@@ -1478,31 +1521,8 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
 		}
 
 		delayed_node->index_item_leaves++;
-	} else if (!test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags)) {
-		const u64 bytes = btrfs_calc_insert_metadata_size(fs_info, 1);
-
-		/*
-		 * Adding the new dir index item does not require touching another
-		 * leaf, so we can release 1 unit of metadata that was previously
-		 * reserved when starting the transaction. This applies only to
-		 * the case where we had a transaction start and excludes the
-		 * transaction join case (when replaying log trees).
-		 */
-		trace_btrfs_space_reservation(fs_info, "transaction",
-					      trans->transid, bytes, 0);
-		btrfs_block_rsv_release(fs_info, trans->block_rsv, bytes, NULL);
-		ASSERT(trans->bytes_reserved >= bytes);
-		trans->bytes_reserved -= bytes;
-	}
-
-	ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
-	if (unlikely(ret)) {
-		btrfs_err(trans->fs_info,
-"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
-			  name_len, name, index, btrfs_root_id(delayed_node->root),
-			  delayed_node->inode_id, dir->index_cnt,
-			  delayed_node->index_cnt, ret);
-		BUG();
+	} else {
+		btrfs_release_dir_index_item_space(trans);
 	}
 	mutex_unlock(&delayed_node->mutex);
 
-- 
2.40.1

[PATCH v2 0/3] btrfs: updates to error path for delayed dir index insertion failure

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Some updates to the error path for delayed dir index insertion failure,
motivated by a recent syzbot report:

  https://lore.kernel.org/linux-btrfs/00000000000036e1290603e097e0@google.com/

Details in the changelogs.

v2: Fixed error path in patch 2 to release delayed item before unlocking
    the delayed node. Added patch 3 to prevent such mistakes in the future.

Filipe Manana (3):
  btrfs: improve error message after failure to add delayed dir index item
  btrfs: remove BUG() after failure to insert delayed dir index item
  btrfs: assert delayed node locked when removing delayed item

 fs/btrfs/delayed-inode.c | 85 ++++++++++++++++++++++++++--------------
 1 file changed, 55 insertions(+), 30 deletions(-)

-- 
2.40.1

[PATCH v2 1/3] btrfs: improve error message after failure to add delayed dir index item

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If we fail to add a delayed dir index item because there's already another
item with the same index number, we print an error message (and then BUG).
However that message isn't very helpful to debug anything because we don't
know what's the index number and what are the values of index counters in
the inode and its delayed inode (index_cnt fields of struct btrfs_inode
and struct btrfs_delayed_node).

So update the error message to include the index number and counters.

We actually had a recent case where this issue was hit by a syzbot report
(see the link below).

Link: https://lore.kernel.org/linux-btrfs/00000000000036e1290603e097e0@google.com/
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/delayed-inode.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 08ecb4d0cc45..f9dae729811b 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1498,9 +1498,10 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
 	ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
 	if (unlikely(ret)) {
 		btrfs_err(trans->fs_info,
-			  "err add delayed dir index item(name: %.*s) into the insertion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)",
-			  name_len, name, delayed_node->root->root_key.objectid,
-			  delayed_node->inode_id, ret);
+"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
+			  name_len, name, index, btrfs_root_id(delayed_node->root),
+			  delayed_node->inode_id, dir->index_cnt,
+			  delayed_node->index_cnt, ret);
 		BUG();
 	}
 	mutex_unlock(&delayed_node->mutex);
-- 
2.40.1

[PATCH v2 2/3] btrfs: remove BUG() after failure to insert delayed dir index item

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Instead of calling BUG() when we fail to insert a delayed dir index item
into the delayed node's tree, we can just release all the resources we
have allocated/acquired before and return the error to the caller. This is
fine because all existing call chains undo anything they have done before
calling btrfs_insert_delayed_dir_index() or BUG_ON (when creating pending
snapshots in the transaction commit path).

So remove the BUG() call and do proper error handling.

This relates to a syzbot report linked below, but does not fix it because
it only prevents hitting a BUG(), it does not fix the issue where somehow
we attempt to use twice the same index number for different index items.

Link: https://lore.kernel.org/linux-btrfs/00000000000036e1290603e097e0@google.com/
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/delayed-inode.c | 74 +++++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index f9dae729811b..eb175ae52245 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1413,7 +1413,29 @@ void btrfs_balance_delayed_items(struct btrfs_fs_info *fs_info)
 	btrfs_wq_run_delayed_node(delayed_root, fs_info, BTRFS_DELAYED_BATCH);
 }
 
-/* Will return 0 or -ENOMEM */
+static void btrfs_release_dir_index_item_space(struct btrfs_trans_handle *trans)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	const u64 bytes = btrfs_calc_insert_metadata_size(fs_info, 1);
+
+	if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags))
+		return;
+
+	/*
+	 * Adding the new dir index item does not require touching another
+	 * leaf, so we can release 1 unit of metadata that was previously
+	 * reserved when starting the transaction. This applies only to
+	 * the case where we had a transaction start and excludes the
+	 * transaction join case (when replaying log trees).
+	 */
+	trace_btrfs_space_reservation(fs_info, "transaction",
+				      trans->transid, bytes, 0);
+	btrfs_block_rsv_release(fs_info, trans->block_rsv, bytes, NULL);
+	ASSERT(trans->bytes_reserved >= bytes);
+	trans->bytes_reserved -= bytes;
+}
+
+/* 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,
@@ -1455,6 +1477,27 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
 
 	mutex_lock(&delayed_node->mutex);
 
+	/*
+	 * First attempt to insert the delayed item. This is to make the error
+	 * handling path simpler in case we fail (-EEXIST). There's no risk of
+	 * any other task coming in and running the delayed item before we do
+	 * the metadata space reservation below, because we are holding the
+	 * delayed node's mutex and that mutex must also be locked before the
+	 * node's delayed items can be run.
+	 */
+	ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
+	if (unlikely(ret)) {
+		btrfs_err(trans->fs_info,
+"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
+			  name_len, name, index, btrfs_root_id(delayed_node->root),
+			  delayed_node->inode_id, dir->index_cnt,
+			  delayed_node->index_cnt, ret);
+		btrfs_release_delayed_item(delayed_item);
+		btrfs_release_dir_index_item_space(trans);
+		mutex_unlock(&delayed_node->mutex);
+		goto release_node;
+	}
+
 	if (delayed_node->index_item_leaves == 0 ||
 	    delayed_node->curr_index_batch_size + data_len > leaf_data_size) {
 		delayed_node->curr_index_batch_size = data_len;
@@ -1472,37 +1515,14 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
 		 * impossible.
 		 */
 		if (WARN_ON(ret)) {
-			mutex_unlock(&delayed_node->mutex);
 			btrfs_release_delayed_item(delayed_item);
+			mutex_unlock(&delayed_node->mutex);
 			goto release_node;
 		}
 
 		delayed_node->index_item_leaves++;
-	} else if (!test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags)) {
-		const u64 bytes = btrfs_calc_insert_metadata_size(fs_info, 1);
-
-		/*
-		 * Adding the new dir index item does not require touching another
-		 * leaf, so we can release 1 unit of metadata that was previously
-		 * reserved when starting the transaction. This applies only to
-		 * the case where we had a transaction start and excludes the
-		 * transaction join case (when replaying log trees).
-		 */
-		trace_btrfs_space_reservation(fs_info, "transaction",
-					      trans->transid, bytes, 0);
-		btrfs_block_rsv_release(fs_info, trans->block_rsv, bytes, NULL);
-		ASSERT(trans->bytes_reserved >= bytes);
-		trans->bytes_reserved -= bytes;
-	}
-
-	ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
-	if (unlikely(ret)) {
-		btrfs_err(trans->fs_info,
-"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
-			  name_len, name, index, btrfs_root_id(delayed_node->root),
-			  delayed_node->inode_id, dir->index_cnt,
-			  delayed_node->index_cnt, ret);
-		BUG();
+	} else {
+		btrfs_release_dir_index_item_space(trans);
 	}
 	mutex_unlock(&delayed_node->mutex);
 
-- 
2.40.1

[PATCH v2 3/3] btrfs: assert delayed node locked when removing delayed item

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When removing a delayed item, or releasing which will remove it as well,
we will modify one of the delayed node's rbtrees and item counter if the
delayed item is in one of the rbtrees. This require having the delayed
node's mutex locked, otherwise we will race with other tasks modifying
the rbtrees and the counter.

This is motivated by a previous version of another patch actually calling
btrfs_release_delayed_item() after unlocking the delayed node's mutex and
against a delayed item that is in a rbtree.

So assert at __btrfs_remove_delayed_item() that the delayed node's mutex
is locked.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/delayed-inode.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index eb175ae52245..8534285f760d 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -412,6 +412,7 @@ static void finish_one_item(struct btrfs_delayed_root *delayed_root)
 
 static void __btrfs_remove_delayed_item(struct btrfs_delayed_item *delayed_item)
 {
+	struct btrfs_delayed_node *delayed_node = delayed_item->delayed_node;
 	struct rb_root_cached *root;
 	struct btrfs_delayed_root *delayed_root;
 
@@ -419,18 +420,21 @@ static void __btrfs_remove_delayed_item(struct btrfs_delayed_item *delayed_item)
 	if (RB_EMPTY_NODE(&delayed_item->rb_node))
 		return;
 
-	delayed_root = delayed_item->delayed_node->root->fs_info->delayed_root;
+	/* If it's in a rbtree, then we need to have delayed node locked. */
+	lockdep_assert_held(&delayed_node->mutex);
+
+	delayed_root = delayed_node->root->fs_info->delayed_root;
 
 	BUG_ON(!delayed_root);
 
 	if (delayed_item->type == BTRFS_DELAYED_INSERTION_ITEM)
-		root = &delayed_item->delayed_node->ins_root;
+		root = &delayed_node->ins_root;
 	else
-		root = &delayed_item->delayed_node->del_root;
+		root = &delayed_node->del_root;
 
 	rb_erase_cached(&delayed_item->rb_node, root);
 	RB_CLEAR_NODE(&delayed_item->rb_node);
-	delayed_item->delayed_node->count--;
+	delayed_node->count--;
 
 	finish_one_item(delayed_root);
 }
-- 
2.40.1

Re: [PATCH v2 1/3] btrfs: improve error message after failure to add delayed dir index item

[Qu Wenruo]

On 2023/8/28 16:06, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> If we fail to add a delayed dir index item because there's already another
> item with the same index number, we print an error message (and then BUG).
> However that message isn't very helpful to debug anything because we don't
> know what's the index number and what are the values of index counters in
> the inode and its delayed inode (index_cnt fields of struct btrfs_inode
> and struct btrfs_delayed_node).
>
> So update the error message to include the index number and counters.
>
> We actually had a recent case where this issue was hit by a syzbot report
> (see the link below).
>
> Link: https://lore.kernel.org/linux-btrfs/00000000000036e1290603e097e0@google.com/
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/delayed-inode.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index 08ecb4d0cc45..f9dae729811b 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -1498,9 +1498,10 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
>   	ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
>   	if (unlikely(ret)) {
>   		btrfs_err(trans->fs_info,
> -			  "err add delayed dir index item(name: %.*s) into the insertion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)",
> -			  name_len, name, delayed_node->root->root_key.objectid,
> -			  delayed_node->inode_id, ret);
> +"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
> +			  name_len, name, index, btrfs_root_id(delayed_node->root),
> +			  delayed_node->inode_id, dir->index_cnt,
> +			  delayed_node->index_cnt, ret);
>   		BUG();
>   	}
>   	mutex_unlock(&delayed_node->mutex);

Re: [PATCH v2 2/3] btrfs: remove BUG() after failure to insert delayed dir index item

[Qu Wenruo]

On 2023/8/28 16:06, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> Instead of calling BUG() when we fail to insert a delayed dir index item
> into the delayed node's tree, we can just release all the resources we
> have allocated/acquired before and return the error to the caller. This is
> fine because all existing call chains undo anything they have done before
> calling btrfs_insert_delayed_dir_index() or BUG_ON (when creating pending
> snapshots in the transaction commit path).
>
> So remove the BUG() call and do proper error handling.
>
> This relates to a syzbot report linked below, but does not fix it because
> it only prevents hitting a BUG(), it does not fix the issue where somehow
> we attempt to use twice the same index number for different index items.
>
> Link: https://lore.kernel.org/linux-btrfs/00000000000036e1290603e097e0@google.com/
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>   fs/btrfs/delayed-inode.c | 74 +++++++++++++++++++++++++---------------
>   1 file changed, 47 insertions(+), 27 deletions(-)
>
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index f9dae729811b..eb175ae52245 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -1413,7 +1413,29 @@ void btrfs_balance_delayed_items(struct btrfs_fs_info *fs_info)
>   	btrfs_wq_run_delayed_node(delayed_root, fs_info, BTRFS_DELAYED_BATCH);
>   }
>
> -/* Will return 0 or -ENOMEM */
> +static void btrfs_release_dir_index_item_space(struct btrfs_trans_handle *trans)
> +{
> +	struct btrfs_fs_info *fs_info = trans->fs_info;
> +	const u64 bytes = btrfs_calc_insert_metadata_size(fs_info, 1);
> +
> +	if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags))
> +		return;
> +
> +	/*
> +	 * Adding the new dir index item does not require touching another
> +	 * leaf, so we can release 1 unit of metadata that was previously
> +	 * reserved when starting the transaction. This applies only to
> +	 * the case where we had a transaction start and excludes the
> +	 * transaction join case (when replaying log trees).
> +	 */
> +	trace_btrfs_space_reservation(fs_info, "transaction",
> +				      trans->transid, bytes, 0);

I know this is from the old code, but shouldn't we use "-bytes" instead?

Otherwise looks fine to me.

Thanks,
Qu

> +	btrfs_block_rsv_release(fs_info, trans->block_rsv, bytes, NULL);
> +	ASSERT(trans->bytes_reserved >= bytes);
> +	trans->bytes_reserved -= bytes;
> +}
> +
> +/* 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,
> @@ -1455,6 +1477,27 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
>
>   	mutex_lock(&delayed_node->mutex);
>
> +	/*
> +	 * First attempt to insert the delayed item. This is to make the error
> +	 * handling path simpler in case we fail (-EEXIST). There's no risk of
> +	 * any other task coming in and running the delayed item before we do
> +	 * the metadata space reservation below, because we are holding the
> +	 * delayed node's mutex and that mutex must also be locked before the
> +	 * node's delayed items can be run.
> +	 */
> +	ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
> +	if (unlikely(ret)) {
> +		btrfs_err(trans->fs_info,
> +"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
> +			  name_len, name, index, btrfs_root_id(delayed_node->root),
> +			  delayed_node->inode_id, dir->index_cnt,
> +			  delayed_node->index_cnt, ret);
> +		btrfs_release_delayed_item(delayed_item);
> +		btrfs_release_dir_index_item_space(trans); > +		mutex_unlock(&delayed_node->mutex);
> +		goto release_node;
> +	}
> +
>   	if (delayed_node->index_item_leaves == 0 ||
>   	    delayed_node->curr_index_batch_size + data_len > leaf_data_size) {
>   		delayed_node->curr_index_batch_size = data_len;
> @@ -1472,37 +1515,14 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
>   		 * impossible.
>   		 */
>   		if (WARN_ON(ret)) {
> -			mutex_unlock(&delayed_node->mutex);
>   			btrfs_release_delayed_item(delayed_item);
> +			mutex_unlock(&delayed_node->mutex);
>   			goto release_node;
>   		}
>
>   		delayed_node->index_item_leaves++;
> -	} else if (!test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags)) {
> -		const u64 bytes = btrfs_calc_insert_metadata_size(fs_info, 1);
> -
> -		/*
> -		 * Adding the new dir index item does not require touching another
> -		 * leaf, so we can release 1 unit of metadata that was previously
> -		 * reserved when starting the transaction. This applies only to
> -		 * the case where we had a transaction start and excludes the
> -		 * transaction join case (when replaying log trees).
> -		 */
> -		trace_btrfs_space_reservation(fs_info, "transaction",
> -					      trans->transid, bytes, 0);
> -		btrfs_block_rsv_release(fs_info, trans->block_rsv, bytes, NULL);
> -		ASSERT(trans->bytes_reserved >= bytes);
> -		trans->bytes_reserved -= bytes;
> -	}
> -
> -	ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
> -	if (unlikely(ret)) {
> -		btrfs_err(trans->fs_info,
> -"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
> -			  name_len, name, index, btrfs_root_id(delayed_node->root),
> -			  delayed_node->inode_id, dir->index_cnt,
> -			  delayed_node->index_cnt, ret);
> -		BUG();
> +	} else {
> +		btrfs_release_dir_index_item_space(trans);
>   	}
>   	mutex_unlock(&delayed_node->mutex);
>

Re: [PATCH v2 3/3] btrfs: assert delayed node locked when removing delayed item

[Qu Wenruo]

On 2023/8/28 16:06, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> When removing a delayed item, or releasing which will remove it as well,
> we will modify one of the delayed node's rbtrees and item counter if the
> delayed item is in one of the rbtrees. This require having the delayed
> node's mutex locked, otherwise we will race with other tasks modifying
> the rbtrees and the counter.
>
> This is motivated by a previous version of another patch actually calling
> btrfs_release_delayed_item() after unlocking the delayed node's mutex and
> against a delayed item that is in a rbtree.
>
> So assert at __btrfs_remove_delayed_item() that the delayed node's mutex
> is locked.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Although it would be even better to add lockdep asserts for both
__btrfs_add_delayed_item() and __btrfs_remove_delayed_item().

Thanks,
Qu
> ---
>   fs/btrfs/delayed-inode.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index eb175ae52245..8534285f760d 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -412,6 +412,7 @@ static void finish_one_item(struct btrfs_delayed_root *delayed_root)
>
>   static void __btrfs_remove_delayed_item(struct btrfs_delayed_item *delayed_item)
>   {
> +	struct btrfs_delayed_node *delayed_node = delayed_item->delayed_node;
>   	struct rb_root_cached *root;
>   	struct btrfs_delayed_root *delayed_root;
>
> @@ -419,18 +420,21 @@ static void __btrfs_remove_delayed_item(struct btrfs_delayed_item *delayed_item)
>   	if (RB_EMPTY_NODE(&delayed_item->rb_node))
>   		return;
>
> -	delayed_root = delayed_item->delayed_node->root->fs_info->delayed_root;
> +	/* If it's in a rbtree, then we need to have delayed node locked. */
> +	lockdep_assert_held(&delayed_node->mutex);
> +
> +	delayed_root = delayed_node->root->fs_info->delayed_root;
>
>   	BUG_ON(!delayed_root);
>
>   	if (delayed_item->type == BTRFS_DELAYED_INSERTION_ITEM)
> -		root = &delayed_item->delayed_node->ins_root;
> +		root = &delayed_node->ins_root;
>   	else
> -		root = &delayed_item->delayed_node->del_root;
> +		root = &delayed_node->del_root;
>
>   	rb_erase_cached(&delayed_item->rb_node, root);
>   	RB_CLEAR_NODE(&delayed_item->rb_node);
> -	delayed_item->delayed_node->count--;
> +	delayed_node->count--;
>
>   	finish_one_item(delayed_root);
>   }

Re: [PATCH v2 2/3] btrfs: remove BUG() after failure to insert delayed dir index item

[Filipe Manana]

On Mon, Aug 28, 2023 at 9:42 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> On 2023/8/28 16:06, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > Instead of calling BUG() when we fail to insert a delayed dir index item
> > into the delayed node's tree, we can just release all the resources we
> > have allocated/acquired before and return the error to the caller. This is
> > fine because all existing call chains undo anything they have done before
> > calling btrfs_insert_delayed_dir_index() or BUG_ON (when creating pending
> > snapshots in the transaction commit path).
> >
> > So remove the BUG() call and do proper error handling.
> >
> > This relates to a syzbot report linked below, but does not fix it because
> > it only prevents hitting a BUG(), it does not fix the issue where somehow
> > we attempt to use twice the same index number for different index items.
> >
> > Link: https://lore.kernel.org/linux-btrfs/00000000000036e1290603e097e0@google.com/
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >   fs/btrfs/delayed-inode.c | 74 +++++++++++++++++++++++++---------------
> >   1 file changed, 47 insertions(+), 27 deletions(-)
> >
> > diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> > index f9dae729811b..eb175ae52245 100644
> > --- a/fs/btrfs/delayed-inode.c
> > +++ b/fs/btrfs/delayed-inode.c
> > @@ -1413,7 +1413,29 @@ void btrfs_balance_delayed_items(struct btrfs_fs_info *fs_info)
> >       btrfs_wq_run_delayed_node(delayed_root, fs_info, BTRFS_DELAYED_BATCH);
> >   }
> >
> > -/* Will return 0 or -ENOMEM */
> > +static void btrfs_release_dir_index_item_space(struct btrfs_trans_handle *trans)
> > +{
> > +     struct btrfs_fs_info *fs_info = trans->fs_info;
> > +     const u64 bytes = btrfs_calc_insert_metadata_size(fs_info, 1);
> > +
> > +     if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags))
> > +             return;
> > +
> > +     /*
> > +      * Adding the new dir index item does not require touching another
> > +      * leaf, so we can release 1 unit of metadata that was previously
> > +      * reserved when starting the transaction. This applies only to
> > +      * the case where we had a transaction start and excludes the
> > +      * transaction join case (when replaying log trees).
> > +      */
> > +     trace_btrfs_space_reservation(fs_info, "transaction",
> > +                                   trans->transid, bytes, 0);
>
> I know this is from the old code, but shouldn't we use "-bytes" instead?

Nop.
The last argument, a value of 0, indicates that it's a space release.
Allocations get a value of 1 for that last argument.


>
> Otherwise looks fine to me.
>
> Thanks,
> Qu
>
> > +     btrfs_block_rsv_release(fs_info, trans->block_rsv, bytes, NULL);
> > +     ASSERT(trans->bytes_reserved >= bytes);
> > +     trans->bytes_reserved -= bytes;
> > +}
> > +
> > +/* 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,
> > @@ -1455,6 +1477,27 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
> >
> >       mutex_lock(&delayed_node->mutex);
> >
> > +     /*
> > +      * First attempt to insert the delayed item. This is to make the error
> > +      * handling path simpler in case we fail (-EEXIST). There's no risk of
> > +      * any other task coming in and running the delayed item before we do
> > +      * the metadata space reservation below, because we are holding the
> > +      * delayed node's mutex and that mutex must also be locked before the
> > +      * node's delayed items can be run.
> > +      */
> > +     ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
> > +     if (unlikely(ret)) {
> > +             btrfs_err(trans->fs_info,
> > +"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
> > +                       name_len, name, index, btrfs_root_id(delayed_node->root),
> > +                       delayed_node->inode_id, dir->index_cnt,
> > +                       delayed_node->index_cnt, ret);
> > +             btrfs_release_delayed_item(delayed_item);
> > +             btrfs_release_dir_index_item_space(trans); > +          mutex_unlock(&delayed_node->mutex);
> > +             goto release_node;
> > +     }
> > +
> >       if (delayed_node->index_item_leaves == 0 ||
> >           delayed_node->curr_index_batch_size + data_len > leaf_data_size) {
> >               delayed_node->curr_index_batch_size = data_len;
> > @@ -1472,37 +1515,14 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
> >                * impossible.
> >                */
> >               if (WARN_ON(ret)) {
> > -                     mutex_unlock(&delayed_node->mutex);
> >                       btrfs_release_delayed_item(delayed_item);
> > +                     mutex_unlock(&delayed_node->mutex);
> >                       goto release_node;
> >               }
> >
> >               delayed_node->index_item_leaves++;
> > -     } else if (!test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags)) {
> > -             const u64 bytes = btrfs_calc_insert_metadata_size(fs_info, 1);
> > -
> > -             /*
> > -              * Adding the new dir index item does not require touching another
> > -              * leaf, so we can release 1 unit of metadata that was previously
> > -              * reserved when starting the transaction. This applies only to
> > -              * the case where we had a transaction start and excludes the
> > -              * transaction join case (when replaying log trees).
> > -              */
> > -             trace_btrfs_space_reservation(fs_info, "transaction",
> > -                                           trans->transid, bytes, 0);
> > -             btrfs_block_rsv_release(fs_info, trans->block_rsv, bytes, NULL);
> > -             ASSERT(trans->bytes_reserved >= bytes);
> > -             trans->bytes_reserved -= bytes;
> > -     }
> > -
> > -     ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
> > -     if (unlikely(ret)) {
> > -             btrfs_err(trans->fs_info,
> > -"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
> > -                       name_len, name, index, btrfs_root_id(delayed_node->root),
> > -                       delayed_node->inode_id, dir->index_cnt,
> > -                       delayed_node->index_cnt, ret);
> > -             BUG();
> > +     } else {
> > +             btrfs_release_dir_index_item_space(trans);
> >       }
> >       mutex_unlock(&delayed_node->mutex);
> >

Re: [PATCH v2 2/3] btrfs: remove BUG() after failure to insert delayed dir index item

[Qu Wenruo]

On 2023/8/28 16:53, Filipe Manana wrote:
> On Mon, Aug 28, 2023 at 9:42 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> On 2023/8/28 16:06, fdmanana@kernel.org wrote:
>>> From: Filipe Manana <fdmanana@suse.com>
>>>
>>> Instead of calling BUG() when we fail to insert a delayed dir index item
>>> into the delayed node's tree, we can just release all the resources we
>>> have allocated/acquired before and return the error to the caller. This is
>>> fine because all existing call chains undo anything they have done before
>>> calling btrfs_insert_delayed_dir_index() or BUG_ON (when creating pending
>>> snapshots in the transaction commit path).
>>>
>>> So remove the BUG() call and do proper error handling.
>>>
>>> This relates to a syzbot report linked below, but does not fix it because
>>> it only prevents hitting a BUG(), it does not fix the issue where somehow
>>> we attempt to use twice the same index number for different index items.
>>>
>>> Link: https://lore.kernel.org/linux-btrfs/00000000000036e1290603e097e0@google.com/
>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>> ---
>>>    fs/btrfs/delayed-inode.c | 74 +++++++++++++++++++++++++---------------
>>>    1 file changed, 47 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
>>> index f9dae729811b..eb175ae52245 100644
>>> --- a/fs/btrfs/delayed-inode.c
>>> +++ b/fs/btrfs/delayed-inode.c
>>> @@ -1413,7 +1413,29 @@ void btrfs_balance_delayed_items(struct btrfs_fs_info *fs_info)
>>>        btrfs_wq_run_delayed_node(delayed_root, fs_info, BTRFS_DELAYED_BATCH);
>>>    }
>>>
>>> -/* Will return 0 or -ENOMEM */
>>> +static void btrfs_release_dir_index_item_space(struct btrfs_trans_handle *trans)
>>> +{
>>> +     struct btrfs_fs_info *fs_info = trans->fs_info;
>>> +     const u64 bytes = btrfs_calc_insert_metadata_size(fs_info, 1);
>>> +
>>> +     if (test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags))
>>> +             return;
>>> +
>>> +     /*
>>> +      * Adding the new dir index item does not require touching another
>>> +      * leaf, so we can release 1 unit of metadata that was previously
>>> +      * reserved when starting the transaction. This applies only to
>>> +      * the case where we had a transaction start and excludes the
>>> +      * transaction join case (when replaying log trees).
>>> +      */
>>> +     trace_btrfs_space_reservation(fs_info, "transaction",
>>> +                                   trans->transid, bytes, 0);
>>
>> I know this is from the old code, but shouldn't we use "-bytes" instead?
>
> Nop.
> The last argument, a value of 0, indicates that it's a space release.
> Allocations get a value of 1 for that last argument.

Oh, my bad.

Then it looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

>
>
>>
>> Otherwise looks fine to me.
>>
>> Thanks,
>> Qu
>>
>>> +     btrfs_block_rsv_release(fs_info, trans->block_rsv, bytes, NULL);
>>> +     ASSERT(trans->bytes_reserved >= bytes);
>>> +     trans->bytes_reserved -= bytes;
>>> +}
>>> +
>>> +/* 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,
>>> @@ -1455,6 +1477,27 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
>>>
>>>        mutex_lock(&delayed_node->mutex);
>>>
>>> +     /*
>>> +      * First attempt to insert the delayed item. This is to make the error
>>> +      * handling path simpler in case we fail (-EEXIST). There's no risk of
>>> +      * any other task coming in and running the delayed item before we do
>>> +      * the metadata space reservation below, because we are holding the
>>> +      * delayed node's mutex and that mutex must also be locked before the
>>> +      * node's delayed items can be run.
>>> +      */
>>> +     ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
>>> +     if (unlikely(ret)) {
>>> +             btrfs_err(trans->fs_info,
>>> +"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
>>> +                       name_len, name, index, btrfs_root_id(delayed_node->root),
>>> +                       delayed_node->inode_id, dir->index_cnt,
>>> +                       delayed_node->index_cnt, ret);
>>> +             btrfs_release_delayed_item(delayed_item);
>>> +             btrfs_release_dir_index_item_space(trans); > +          mutex_unlock(&delayed_node->mutex);
>>> +             goto release_node;
>>> +     }
>>> +
>>>        if (delayed_node->index_item_leaves == 0 ||
>>>            delayed_node->curr_index_batch_size + data_len > leaf_data_size) {
>>>                delayed_node->curr_index_batch_size = data_len;
>>> @@ -1472,37 +1515,14 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
>>>                 * impossible.
>>>                 */
>>>                if (WARN_ON(ret)) {
>>> -                     mutex_unlock(&delayed_node->mutex);
>>>                        btrfs_release_delayed_item(delayed_item);
>>> +                     mutex_unlock(&delayed_node->mutex);
>>>                        goto release_node;
>>>                }
>>>
>>>                delayed_node->index_item_leaves++;
>>> -     } else if (!test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags)) {
>>> -             const u64 bytes = btrfs_calc_insert_metadata_size(fs_info, 1);
>>> -
>>> -             /*
>>> -              * Adding the new dir index item does not require touching another
>>> -              * leaf, so we can release 1 unit of metadata that was previously
>>> -              * reserved when starting the transaction. This applies only to
>>> -              * the case where we had a transaction start and excludes the
>>> -              * transaction join case (when replaying log trees).
>>> -              */
>>> -             trace_btrfs_space_reservation(fs_info, "transaction",
>>> -                                           trans->transid, bytes, 0);
>>> -             btrfs_block_rsv_release(fs_info, trans->block_rsv, bytes, NULL);
>>> -             ASSERT(trans->bytes_reserved >= bytes);
>>> -             trans->bytes_reserved -= bytes;
>>> -     }
>>> -
>>> -     ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
>>> -     if (unlikely(ret)) {
>>> -             btrfs_err(trans->fs_info,
>>> -"error adding delayed dir index item, name: %.*s, index: %llu, root: %llu, dir: %llu, dir->index_cnt: %llu, delayed_node->index_cnt: %llu, error: %d",
>>> -                       name_len, name, index, btrfs_root_id(delayed_node->root),
>>> -                       delayed_node->inode_id, dir->index_cnt,
>>> -                       delayed_node->index_cnt, ret);
>>> -             BUG();
>>> +     } else {
>>> +             btrfs_release_dir_index_item_space(trans);
>>>        }
>>>        mutex_unlock(&delayed_node->mutex);
>>>

Re: [PATCH v2 0/3] btrfs: updates to error path for delayed dir index insertion failure

[David Sterba]

On Mon, Aug 28, 2023 at 09:06:41AM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Some updates to the error path for delayed dir index insertion failure,
> motivated by a recent syzbot report:
> 
>   https://lore.kernel.org/linux-btrfs/00000000000036e1290603e097e0@google.com/
> 
> Details in the changelogs.
> 
> v2: Fixed error path in patch 2 to release delayed item before unlocking
>     the delayed node. Added patch 3 to prevent such mistakes in the future.
> 
> Filipe Manana (3):
>   btrfs: improve error message after failure to add delayed dir index item
>   btrfs: remove BUG() after failure to insert delayed dir index item
>   btrfs: assert delayed node locked when removing delayed item

Added to misc-next, thanks.