[PATCH 1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item()

[PATCH 1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item()

[Yangtao Li]

All cleanup paths lead to btrfs_path_free so we can define path with the
automatic free callback.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/btrfs/volumes.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index c8c21c55be53..a962efaec4ea 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3730,7 +3730,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
 	struct btrfs_trans_handle *trans;
 	struct btrfs_balance_item *item;
 	struct btrfs_disk_balance_args disk_bargs;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct extent_buffer *leaf;
 	struct btrfs_key key;
 	int ret, err;
@@ -3740,10 +3740,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
 		return -ENOMEM;
 
 	trans = btrfs_start_transaction(root, 0);
-	if (IS_ERR(trans)) {
-		btrfs_free_path(path);
+	if (IS_ERR(trans))
 		return PTR_ERR(trans);
-	}
 
 	key.objectid = BTRFS_BALANCE_OBJECTID;
 	key.type = BTRFS_TEMPORARY_ITEM_KEY;
@@ -3767,7 +3765,6 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
 	btrfs_set_balance_sys(leaf, item, &disk_bargs);
 	btrfs_set_balance_flags(leaf, item, bctl->flags);
 out:
-	btrfs_free_path(path);
 	err = btrfs_commit_transaction(trans);
 	if (err && !ret)
 		ret = err;
-- 
2.39.0

[PATCH 2/4] btrfs: use BTRFS_PATH_AUTO_FREE in del_balance_item()

[Yangtao Li]

All cleanup paths lead to btrfs_path_free so we can define path with the
automatic free callback.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/btrfs/volumes.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a962efaec4ea..375d92720e17 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3775,7 +3775,7 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_root *root = fs_info->tree_root;
 	struct btrfs_trans_handle *trans;
-	struct btrfs_path *path;
+	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_key key;
 	int ret, err;
 
@@ -3784,10 +3784,8 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
 		return -ENOMEM;
 
 	trans = btrfs_start_transaction_fallback_global_rsv(root, 0);
-	if (IS_ERR(trans)) {
-		btrfs_free_path(path);
+	if (IS_ERR(trans))
 		return PTR_ERR(trans);
-	}
 
 	key.objectid = BTRFS_BALANCE_OBJECTID;
 	key.type = BTRFS_TEMPORARY_ITEM_KEY;
@@ -3803,7 +3801,6 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
 
 	ret = btrfs_del_item(trans, root, path);
 out:
-	btrfs_free_path(path);
 	err = btrfs_commit_transaction(trans);
 	if (err && !ret)
 		ret = err;
-- 
2.39.0

Re: [PATCH 2/4] btrfs: use BTRFS_PATH_AUTO_FREE in del_balance_item()

[Filipe Manana]

On Tue, Apr 8, 2025 at 1:18 PM Yangtao Li <frank.li@vivo.com> wrote:
>
> All cleanup paths lead to btrfs_path_free so we can define path with the
> automatic free callback.
>
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>  fs/btrfs/volumes.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index a962efaec4ea..375d92720e17 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3775,7 +3775,7 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
>  {
>         struct btrfs_root *root = fs_info->tree_root;
>         struct btrfs_trans_handle *trans;
> -       struct btrfs_path *path;
> +       BTRFS_PATH_AUTO_FREE(path);
>         struct btrfs_key key;
>         int ret, err;
>
> @@ -3784,10 +3784,8 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
>                 return -ENOMEM;
>
>         trans = btrfs_start_transaction_fallback_global_rsv(root, 0);
> -       if (IS_ERR(trans)) {
> -               btrfs_free_path(path);
> +       if (IS_ERR(trans))
>                 return PTR_ERR(trans);
> -       }
>
>         key.objectid = BTRFS_BALANCE_OBJECTID;
>         key.type = BTRFS_TEMPORARY_ITEM_KEY;
> @@ -3803,7 +3801,6 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
>
>         ret = btrfs_del_item(trans, root, path);
>  out:
> -       btrfs_free_path(path);
>         err = btrfs_commit_transaction(trans);

Exactly the same comment as for the previous patch -
https://lore.kernel.org/linux-btrfs/CAL3q7H6ysGxpXs8P9iPY-Y1KNKPggGSFHR_tMv-34Q+Qf6PZTQ@mail.gmail.com/
This can result in a deadlock.

Thanks.


>         if (err && !ret)
>                 ret = err;
> --
> 2.39.0
>
>

[PATCH 3/4] btrfs: Fix transaction abort during failure in insert_balance_item()

[Yangtao Li]

Handle insert_empty_item errors by adding explicit
btrfs_abort_transaction/btrfs_end_transaction calls.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/btrfs/volumes.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 375d92720e17..347c475028e0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3733,7 +3733,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
 	BTRFS_PATH_AUTO_FREE(path);
 	struct extent_buffer *leaf;
 	struct btrfs_key key;
-	int ret, err;
+	int ret;
 
 	path = btrfs_alloc_path();
 	if (!path)
@@ -3749,8 +3749,11 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
 
 	ret = btrfs_insert_empty_item(trans, root, path, &key,
 				      sizeof(*item));
-	if (ret)
-		goto out;
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		btrfs_end_transaction(trans);
+		return ret;
+	}
 
 	leaf = path->nodes[0];
 	item = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_balance_item);
@@ -3764,11 +3767,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
 	btrfs_cpu_balance_args_to_disk(&disk_bargs, &bctl->sys);
 	btrfs_set_balance_sys(leaf, item, &disk_bargs);
 	btrfs_set_balance_flags(leaf, item, bctl->flags);
-out:
-	err = btrfs_commit_transaction(trans);
-	if (err && !ret)
-		ret = err;
-	return ret;
+
+	return btrfs_commit_transaction(trans);
 }
 
 static int del_balance_item(struct btrfs_fs_info *fs_info)
-- 
2.39.0

Re: [PATCH 3/4] btrfs: Fix transaction abort during failure in insert_balance_item()

[Filipe Manana]

On Tue, Apr 8, 2025 at 1:22 PM Yangtao Li <frank.li@vivo.com> wrote:
>
> Handle insert_empty_item errors by adding explicit
> btrfs_abort_transaction/btrfs_end_transaction calls.

But why should we do it?
More below.

>
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>  fs/btrfs/volumes.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 375d92720e17..347c475028e0 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3733,7 +3733,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>         BTRFS_PATH_AUTO_FREE(path);
>         struct extent_buffer *leaf;
>         struct btrfs_key key;
> -       int ret, err;
> +       int ret;
>
>         path = btrfs_alloc_path();
>         if (!path)
> @@ -3749,8 +3749,11 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>
>         ret = btrfs_insert_empty_item(trans, root, path, &key,
>                                       sizeof(*item));
> -       if (ret)
> -               goto out;
> +       if (ret) {
> +               btrfs_abort_transaction(trans, ret);
> +               btrfs_end_transaction(trans);

A transaction abort will turn the fs into RO mode, and it's meant to
be used when we can't proceed with changes to the fs after we did
partial changes, to avoid leaving things in an inconsistent state.
Here we don't abort because we haven't done any changes before using
the transaction handle, so an abort is pointless and will turn the fs
into RO mode unnecessarily.

Thanks.

> +               return ret;
> +       }
>
>         leaf = path->nodes[0];
>         item = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_balance_item);
> @@ -3764,11 +3767,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>         btrfs_cpu_balance_args_to_disk(&disk_bargs, &bctl->sys);
>         btrfs_set_balance_sys(leaf, item, &disk_bargs);
>         btrfs_set_balance_flags(leaf, item, bctl->flags);
> -out:
> -       err = btrfs_commit_transaction(trans);
> -       if (err && !ret)
> -               ret = err;
> -       return ret;
> +
> +       return btrfs_commit_transaction(trans);
>  }
>
>  static int del_balance_item(struct btrfs_fs_info *fs_info)
> --
> 2.39.0
>
>

[PATCH 4/4] btrfs: Fix transaction abort during failure in del_balance_item()

[Yangtao Li]

Handle errors by adding explicit btrfs_abort_transaction
and btrfs_end_transaction calls.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/btrfs/volumes.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 347c475028e0..23739d18d833 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3777,7 +3777,7 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
 	struct btrfs_trans_handle *trans;
 	BTRFS_PATH_AUTO_FREE(path);
 	struct btrfs_key key;
-	int ret, err;
+	int ret;
 
 	path = btrfs_alloc_path();
 	if (!path)
@@ -3800,10 +3800,13 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
 	}
 
 	ret = btrfs_del_item(trans, root, path);
+	if (ret)
+		goto out;
+
+	return btrfs_commit_transaction(trans);
 out:
-	err = btrfs_commit_transaction(trans);
-	if (err && !ret)
-		ret = err;
+	btrfs_abort_transaction(trans, ret);
+	btrfs_end_transaction(trans);
 	return ret;
 }
 
-- 
2.39.0

Re: [PATCH 4/4] btrfs: Fix transaction abort during failure in del_balance_item()

[Filipe Manana]

On Tue, Apr 8, 2025 at 1:19 PM Yangtao Li <frank.li@vivo.com> wrote:
>
> Handle errors by adding explicit btrfs_abort_transaction
> and btrfs_end_transaction calls.

Again, like in the previous patch, why?
This provides no reason at all why we should abort.
And the same comment below.

>
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>  fs/btrfs/volumes.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 347c475028e0..23739d18d833 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3777,7 +3777,7 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
>         struct btrfs_trans_handle *trans;
>         BTRFS_PATH_AUTO_FREE(path);
>         struct btrfs_key key;
> -       int ret, err;
> +       int ret;
>
>         path = btrfs_alloc_path();
>         if (!path)
> @@ -3800,10 +3800,13 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
>         }
>
>         ret = btrfs_del_item(trans, root, path);
> +       if (ret)
> +               goto out;
> +
> +       return btrfs_commit_transaction(trans);
>  out:
> -       err = btrfs_commit_transaction(trans);
> -       if (err && !ret)
> -               ret = err;
> +       btrfs_abort_transaction(trans, ret);
> +       btrfs_end_transaction(trans);

A transaction abort will turn the fs into RO mode, and it's meant to
be used when we can't proceed with changes to the fs after we did
partial changes, to avoid leaving things in an inconsistent state.
Here we don't abort because we haven't done any changes before using
the transaction handle, so an abort is pointless and will turn the fs
into RO mode unnecessarily.

Thanks.

>         return ret;
>  }
>
> --
> 2.39.0
>
>

Re: [PATCH 4/4] btrfs: Fix transaction abort during failure in del_balance_item()

[David Sterba]

On Tue, Apr 08, 2025 at 03:53:04PM +0100, Filipe Manana wrote:
> On Tue, Apr 8, 2025 at 1:19 PM Yangtao Li <frank.li@vivo.com> wrote:
> >
> > Handle errors by adding explicit btrfs_abort_transaction
> > and btrfs_end_transaction calls.
> 
> Again, like in the previous patch, why?
> This provides no reason at all why we should abort.
> And the same comment below.
> 
> >
> > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > ---
> >  fs/btrfs/volumes.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 347c475028e0..23739d18d833 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -3777,7 +3777,7 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
> >         struct btrfs_trans_handle *trans;
> >         BTRFS_PATH_AUTO_FREE(path);
> >         struct btrfs_key key;
> > -       int ret, err;
> > +       int ret;
> >
> >         path = btrfs_alloc_path();
> >         if (!path)
> > @@ -3800,10 +3800,13 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
> >         }
> >
> >         ret = btrfs_del_item(trans, root, path);
> > +       if (ret)
> > +               goto out;
> > +
> > +       return btrfs_commit_transaction(trans);
> >  out:
> > -       err = btrfs_commit_transaction(trans);
> > -       if (err && !ret)
> > -               ret = err;
> > +       btrfs_abort_transaction(trans, ret);
> > +       btrfs_end_transaction(trans);
> 
> A transaction abort will turn the fs into RO mode, and it's meant to
> be used when we can't proceed with changes to the fs after we did
> partial changes, to avoid leaving things in an inconsistent state.
> Here we don't abort because we haven't done any changes before using
> the transaction handle, so an abort is pointless and will turn the fs
> into RO mode unnecessarily.

The del_balance_item() case seems to be unique, there's only one caller
reset_balance_state() that calls btrfs_handle_fs_error() in case of an
error. This is almost the same as a transaction abort, but
del_balance_item() may be called from various contexts.

The error handling may be improved here, e.g. some callers may care
about the actual error of del_balance_item/reset_balance_state, but
rather a hard transaction abort it could be better to handle it more
gracefully for operations that are restartable, like return an EAGAIN.

Re: [PATCH 4/4] btrfs: Fix transaction abort during failure in del_balance_item()

[Filipe Manana]

On Wed, Apr 9, 2025 at 12:04 AM David Sterba <dsterba@suse.cz> wrote:
>
> On Tue, Apr 08, 2025 at 03:53:04PM +0100, Filipe Manana wrote:
> > On Tue, Apr 8, 2025 at 1:19 PM Yangtao Li <frank.li@vivo.com> wrote:
> > >
> > > Handle errors by adding explicit btrfs_abort_transaction
> > > and btrfs_end_transaction calls.
> >
> > Again, like in the previous patch, why?
> > This provides no reason at all why we should abort.
> > And the same comment below.
> >
> > >
> > > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > > ---
> > >  fs/btrfs/volumes.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > > index 347c475028e0..23739d18d833 100644
> > > --- a/fs/btrfs/volumes.c
> > > +++ b/fs/btrfs/volumes.c
> > > @@ -3777,7 +3777,7 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
> > >         struct btrfs_trans_handle *trans;
> > >         BTRFS_PATH_AUTO_FREE(path);
> > >         struct btrfs_key key;
> > > -       int ret, err;
> > > +       int ret;
> > >
> > >         path = btrfs_alloc_path();
> > >         if (!path)
> > > @@ -3800,10 +3800,13 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
> > >         }
> > >
> > >         ret = btrfs_del_item(trans, root, path);
> > > +       if (ret)
> > > +               goto out;
> > > +
> > > +       return btrfs_commit_transaction(trans);
> > >  out:
> > > -       err = btrfs_commit_transaction(trans);
> > > -       if (err && !ret)
> > > -               ret = err;
> > > +       btrfs_abort_transaction(trans, ret);
> > > +       btrfs_end_transaction(trans);
> >
> > A transaction abort will turn the fs into RO mode, and it's meant to
> > be used when we can't proceed with changes to the fs after we did
> > partial changes, to avoid leaving things in an inconsistent state.
> > Here we don't abort because we haven't done any changes before using
> > the transaction handle, so an abort is pointless and will turn the fs
> > into RO mode unnecessarily.
>
> The del_balance_item() case seems to be unique, there's only one caller
> reset_balance_state() that calls btrfs_handle_fs_error() in case of an
> error. This is almost the same as a transaction abort, but
> del_balance_item() may be called from various contexts.
>
> The error handling may be improved here, e.g. some callers may care
> about the actual error of del_balance_item/reset_balance_state, but
> rather a hard transaction abort it could be better to handle it more
> gracefully for operations that are restartable, like return an EAGAIN.

That's just not possible in 2 cases out of 3 (btrfs_cancel_balance()
being the exception where it's possible), since we already have an
error return value to return to user space.
The btrfs_handle_fs_error() call is exaggerated and doesn't accomplish
anything as the balance item was already persisted in a transaction
committed previously.

Re: [PATCH 1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item()

[Filipe Manana]

On Tue, Apr 8, 2025 at 1:18 PM Yangtao Li <frank.li@vivo.com> wrote:
>
> All cleanup paths lead to btrfs_path_free so we can define path with the
> automatic free callback.
>
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>  fs/btrfs/volumes.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index c8c21c55be53..a962efaec4ea 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3730,7 +3730,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>         struct btrfs_trans_handle *trans;
>         struct btrfs_balance_item *item;
>         struct btrfs_disk_balance_args disk_bargs;
> -       struct btrfs_path *path;
> +       BTRFS_PATH_AUTO_FREE(path);
>         struct extent_buffer *leaf;
>         struct btrfs_key key;
>         int ret, err;
> @@ -3740,10 +3740,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>                 return -ENOMEM;
>
>         trans = btrfs_start_transaction(root, 0);
> -       if (IS_ERR(trans)) {
> -               btrfs_free_path(path);
> +       if (IS_ERR(trans))
>                 return PTR_ERR(trans);
> -       }
>
>         key.objectid = BTRFS_BALANCE_OBJECTID;
>         key.type = BTRFS_TEMPORARY_ITEM_KEY;
> @@ -3767,7 +3765,6 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>         btrfs_set_balance_sys(leaf, item, &disk_bargs);
>         btrfs_set_balance_flags(leaf, item, bctl->flags);
>  out:
> -       btrfs_free_path(path);
>         err = btrfs_commit_transaction(trans);

This isn't a good idea at all.
We're now committing a transaction while holding a write lock on some
leaf of the tree root - this can result in a deadlock as the
transaction commit needs to update the tree root (see
update_cowonly_root()).

Thanks.


>         if (err && !ret)
>                 ret = err;
> --
> 2.39.0
>
>

Re: [PATCH 1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item()

[Daniel Vacek]

On Tue, 8 Apr 2025 at 16:47, Filipe Manana <fdmanana@kernel.org> wrote:
>
> On Tue, Apr 8, 2025 at 1:18 PM Yangtao Li <frank.li@vivo.com> wrote:
> >
> > All cleanup paths lead to btrfs_path_free so we can define path with the
> > automatic free callback.
> >
> > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > ---
> >  fs/btrfs/volumes.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index c8c21c55be53..a962efaec4ea 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -3730,7 +3730,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> >         struct btrfs_trans_handle *trans;
> >         struct btrfs_balance_item *item;
> >         struct btrfs_disk_balance_args disk_bargs;
> > -       struct btrfs_path *path;
> > +       BTRFS_PATH_AUTO_FREE(path);
> >         struct extent_buffer *leaf;
> >         struct btrfs_key key;
> >         int ret, err;
> > @@ -3740,10 +3740,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> >                 return -ENOMEM;
> >
> >         trans = btrfs_start_transaction(root, 0);
> > -       if (IS_ERR(trans)) {
> > -               btrfs_free_path(path);
> > +       if (IS_ERR(trans))
> >                 return PTR_ERR(trans);
> > -       }
> >
> >         key.objectid = BTRFS_BALANCE_OBJECTID;
> >         key.type = BTRFS_TEMPORARY_ITEM_KEY;
> > @@ -3767,7 +3765,6 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> >         btrfs_set_balance_sys(leaf, item, &disk_bargs);
> >         btrfs_set_balance_flags(leaf, item, bctl->flags);
> >  out:
> > -       btrfs_free_path(path);
> >         err = btrfs_commit_transaction(trans);
>
> This isn't a good idea at all.
> We're now committing a transaction while holding a write lock on some
> leaf of the tree root - this can result in a deadlock as the
> transaction commit needs to update the tree root (see
> update_cowonly_root()).

I do not follow. This actually looks good to me.
Is there really any functional change? What am I missing?

--nX

> Thanks.
>
>
> >         if (err && !ret)
> >                 ret = err;
> > --
> > 2.39.0
> >
> >
>

Re: [PATCH 1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item()

[Filipe Manana]

On Tue, Apr 8, 2025 at 6:36 PM Daniel Vacek <neelx@suse.com> wrote:
>
> On Tue, 8 Apr 2025 at 16:47, Filipe Manana <fdmanana@kernel.org> wrote:
> >
> > On Tue, Apr 8, 2025 at 1:18 PM Yangtao Li <frank.li@vivo.com> wrote:
> > >
> > > All cleanup paths lead to btrfs_path_free so we can define path with the
> > > automatic free callback.
> > >
> > > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > > ---
> > >  fs/btrfs/volumes.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > > index c8c21c55be53..a962efaec4ea 100644
> > > --- a/fs/btrfs/volumes.c
> > > +++ b/fs/btrfs/volumes.c
> > > @@ -3730,7 +3730,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> > >         struct btrfs_trans_handle *trans;
> > >         struct btrfs_balance_item *item;
> > >         struct btrfs_disk_balance_args disk_bargs;
> > > -       struct btrfs_path *path;
> > > +       BTRFS_PATH_AUTO_FREE(path);
> > >         struct extent_buffer *leaf;
> > >         struct btrfs_key key;
> > >         int ret, err;
> > > @@ -3740,10 +3740,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> > >                 return -ENOMEM;
> > >
> > >         trans = btrfs_start_transaction(root, 0);
> > > -       if (IS_ERR(trans)) {
> > > -               btrfs_free_path(path);
> > > +       if (IS_ERR(trans))
> > >                 return PTR_ERR(trans);
> > > -       }
> > >
> > >         key.objectid = BTRFS_BALANCE_OBJECTID;
> > >         key.type = BTRFS_TEMPORARY_ITEM_KEY;
> > > @@ -3767,7 +3765,6 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> > >         btrfs_set_balance_sys(leaf, item, &disk_bargs);
> > >         btrfs_set_balance_flags(leaf, item, bctl->flags);
> > >  out:
> > > -       btrfs_free_path(path);
> > >         err = btrfs_commit_transaction(trans);
> >
> > This isn't a good idea at all.
> > We're now committing a transaction while holding a write lock on some
> > leaf of the tree root - this can result in a deadlock as the
> > transaction commit needs to update the tree root (see
> > update_cowonly_root()).
>
> I do not follow. This actually looks good to me.

path->nodes[0] has a write locked leaf, returned by btrfs_insert_empty_item().

> Is there really any functional change? What am I missing?

Yes there is, a huge one. Even if a transaction commit didn't need to
update the root tree, it would be performance wise to commit a
transaction while holding a lock on a leaf unnecessarily.

>
> --nX
>
> > Thanks.
> >
> >
> > >         if (err && !ret)
> > >                 ret = err;
> > > --
> > > 2.39.0
> > >
> > >
> >

Re: [PATCH 1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item()

[Daniel Vacek]

On Tue, 8 Apr 2025 at 19:41, Filipe Manana <fdmanana@kernel.org> wrote:
>
> On Tue, Apr 8, 2025 at 6:36 PM Daniel Vacek <neelx@suse.com> wrote:
> >
> > On Tue, 8 Apr 2025 at 16:47, Filipe Manana <fdmanana@kernel.org> wrote:
> > >
> > > On Tue, Apr 8, 2025 at 1:18 PM Yangtao Li <frank.li@vivo.com> wrote:
> > > >
> > > > All cleanup paths lead to btrfs_path_free so we can define path with the
> > > > automatic free callback.
> > > >
> > > > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > > > ---
> > > >  fs/btrfs/volumes.c | 7 ++-----
> > > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > > > index c8c21c55be53..a962efaec4ea 100644
> > > > --- a/fs/btrfs/volumes.c
> > > > +++ b/fs/btrfs/volumes.c
> > > > @@ -3730,7 +3730,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> > > >         struct btrfs_trans_handle *trans;
> > > >         struct btrfs_balance_item *item;
> > > >         struct btrfs_disk_balance_args disk_bargs;
> > > > -       struct btrfs_path *path;
> > > > +       BTRFS_PATH_AUTO_FREE(path);
> > > >         struct extent_buffer *leaf;
> > > >         struct btrfs_key key;
> > > >         int ret, err;
> > > > @@ -3740,10 +3740,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> > > >                 return -ENOMEM;
> > > >
> > > >         trans = btrfs_start_transaction(root, 0);
> > > > -       if (IS_ERR(trans)) {
> > > > -               btrfs_free_path(path);
> > > > +       if (IS_ERR(trans))
> > > >                 return PTR_ERR(trans);
> > > > -       }
> > > >
> > > >         key.objectid = BTRFS_BALANCE_OBJECTID;
> > > >         key.type = BTRFS_TEMPORARY_ITEM_KEY;
> > > > @@ -3767,7 +3765,6 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> > > >         btrfs_set_balance_sys(leaf, item, &disk_bargs);
> > > >         btrfs_set_balance_flags(leaf, item, bctl->flags);
> > > >  out:
> > > > -       btrfs_free_path(path);
> > > >         err = btrfs_commit_transaction(trans);
> > >
> > > This isn't a good idea at all.
> > > We're now committing a transaction while holding a write lock on some
> > > leaf of the tree root - this can result in a deadlock as the
> > > transaction commit needs to update the tree root (see
> > > update_cowonly_root()).
> >
> > I do not follow. This actually looks good to me.
>
> path->nodes[0] has a write locked leaf, returned by btrfs_insert_empty_item().

Well, the first return is before calling this function and the final
return is only after committing.

Again, the function keeps it's former structure as it was before this
patch. I still don't see any logical or functional change.

I'm lost. This still looks to me just as a straightforward cleanup.

> > Is there really any functional change? What am I missing?
>
> Yes there is, a huge one. Even if a transaction commit didn't need to
> update the root tree, it would be performance wise to commit a
> transaction while holding a lock on a leaf unnecessarily.
>
> >
> > --nX
> >
> > > Thanks.
> > >
> > >
> > > >         if (err && !ret)
> > > >                 ret = err;
> > > > --
> > > > 2.39.0
> > > >
> > > >
> > >

Re: [PATCH 1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item()

[Filipe Manana]

On Tue, Apr 8, 2025 at 6:47 PM Daniel Vacek <neelx@suse.com> wrote:
>
> On Tue, 8 Apr 2025 at 19:41, Filipe Manana <fdmanana@kernel.org> wrote:
> >
> > On Tue, Apr 8, 2025 at 6:36 PM Daniel Vacek <neelx@suse.com> wrote:
> > >
> > > On Tue, 8 Apr 2025 at 16:47, Filipe Manana <fdmanana@kernel.org> wrote:
> > > >
> > > > On Tue, Apr 8, 2025 at 1:18 PM Yangtao Li <frank.li@vivo.com> wrote:
> > > > >
> > > > > All cleanup paths lead to btrfs_path_free so we can define path with the
> > > > > automatic free callback.
> > > > >
> > > > > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > > > > ---
> > > > >  fs/btrfs/volumes.c | 7 ++-----
> > > > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > > > > index c8c21c55be53..a962efaec4ea 100644
> > > > > --- a/fs/btrfs/volumes.c
> > > > > +++ b/fs/btrfs/volumes.c
> > > > > @@ -3730,7 +3730,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> > > > >         struct btrfs_trans_handle *trans;
> > > > >         struct btrfs_balance_item *item;
> > > > >         struct btrfs_disk_balance_args disk_bargs;
> > > > > -       struct btrfs_path *path;
> > > > > +       BTRFS_PATH_AUTO_FREE(path);
> > > > >         struct extent_buffer *leaf;
> > > > >         struct btrfs_key key;
> > > > >         int ret, err;
> > > > > @@ -3740,10 +3740,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> > > > >                 return -ENOMEM;
> > > > >
> > > > >         trans = btrfs_start_transaction(root, 0);
> > > > > -       if (IS_ERR(trans)) {
> > > > > -               btrfs_free_path(path);
> > > > > +       if (IS_ERR(trans))
> > > > >                 return PTR_ERR(trans);
> > > > > -       }
> > > > >
> > > > >         key.objectid = BTRFS_BALANCE_OBJECTID;
> > > > >         key.type = BTRFS_TEMPORARY_ITEM_KEY;
> > > > > @@ -3767,7 +3765,6 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> > > > >         btrfs_set_balance_sys(leaf, item, &disk_bargs);
> > > > >         btrfs_set_balance_flags(leaf, item, bctl->flags);
> > > > >  out:
> > > > > -       btrfs_free_path(path);
> > > > >         err = btrfs_commit_transaction(trans);
> > > >
> > > > This isn't a good idea at all.
> > > > We're now committing a transaction while holding a write lock on some
> > > > leaf of the tree root - this can result in a deadlock as the
> > > > transaction commit needs to update the tree root (see
> > > > update_cowonly_root()).
> > >
> > > I do not follow. This actually looks good to me.
> >
> > path->nodes[0] has a write locked leaf, returned by btrfs_insert_empty_item().
>
> Well, the first return is before calling this function and the final
> return is only after committing.
>
> Again, the function keeps it's former structure as it was before this
> patch. I still don't see any logical or functional change.

I don't know how to put it simpler to you. There's a path setup by
btrfs_insert_empty_item(), with a leaf locked in write mode.
You can't just do a transaction commit while holding it locked, as
this can deadlock because updating the tree root is part of the
critical section of a transaction commit.
It seems trivial to me...

>
> I'm lost. This still looks to me just as a straightforward cleanup.
>
> > > Is there really any functional change? What am I missing?
> >
> > Yes there is, a huge one. Even if a transaction commit didn't need to
> > update the root tree, it would be performance wise to commit a
> > transaction while holding a lock on a leaf unnecessarily.
> >
> > >
> > > --nX
> > >
> > > > Thanks.
> > > >
> > > >
> > > > >         if (err && !ret)
> > > > >                 ret = err;
> > > > > --
> > > > > 2.39.0
> > > > >
> > > > >
> > > >

Re: [PATCH 1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item()

[Daniel Vacek]

On Tue, 8 Apr 2025 at 19:52, Filipe Manana <fdmanana@kernel.org> wrote:
>
> On Tue, Apr 8, 2025 at 6:47 PM Daniel Vacek <neelx@suse.com> wrote:
> >
> > On Tue, 8 Apr 2025 at 19:41, Filipe Manana <fdmanana@kernel.org> wrote:
> > >
> > > On Tue, Apr 8, 2025 at 6:36 PM Daniel Vacek <neelx@suse.com> wrote:
> > > >
> > > > On Tue, 8 Apr 2025 at 16:47, Filipe Manana <fdmanana@kernel.org> wrote:
> > > > >
> > > > > On Tue, Apr 8, 2025 at 1:18 PM Yangtao Li <frank.li@vivo.com> wrote:
> > > > > >
> > > > > > All cleanup paths lead to btrfs_path_free so we can define path with the
> > > > > > automatic free callback.
> > > > > >
> > > > > > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > > > > > ---
> > > > > >  fs/btrfs/volumes.c | 7 ++-----
> > > > > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > > > > > index c8c21c55be53..a962efaec4ea 100644
> > > > > > --- a/fs/btrfs/volumes.c
> > > > > > +++ b/fs/btrfs/volumes.c
> > > > > > @@ -3730,7 +3730,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> > > > > >         struct btrfs_trans_handle *trans;
> > > > > >         struct btrfs_balance_item *item;
> > > > > >         struct btrfs_disk_balance_args disk_bargs;
> > > > > > -       struct btrfs_path *path;
> > > > > > +       BTRFS_PATH_AUTO_FREE(path);
> > > > > >         struct extent_buffer *leaf;
> > > > > >         struct btrfs_key key;
> > > > > >         int ret, err;
> > > > > > @@ -3740,10 +3740,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> > > > > >                 return -ENOMEM;
> > > > > >
> > > > > >         trans = btrfs_start_transaction(root, 0);
> > > > > > -       if (IS_ERR(trans)) {
> > > > > > -               btrfs_free_path(path);
> > > > > > +       if (IS_ERR(trans))
> > > > > >                 return PTR_ERR(trans);
> > > > > > -       }
> > > > > >
> > > > > >         key.objectid = BTRFS_BALANCE_OBJECTID;
> > > > > >         key.type = BTRFS_TEMPORARY_ITEM_KEY;
> > > > > > @@ -3767,7 +3765,6 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> > > > > >         btrfs_set_balance_sys(leaf, item, &disk_bargs);
> > > > > >         btrfs_set_balance_flags(leaf, item, bctl->flags);
> > > > > >  out:
> > > > > > -       btrfs_free_path(path);
> > > > > >         err = btrfs_commit_transaction(trans);
> > > > >
> > > > > This isn't a good idea at all.
> > > > > We're now committing a transaction while holding a write lock on some
> > > > > leaf of the tree root - this can result in a deadlock as the
> > > > > transaction commit needs to update the tree root (see
> > > > > update_cowonly_root()).
> > > >
> > > > I do not follow. This actually looks good to me.
> > >
> > > path->nodes[0] has a write locked leaf, returned by btrfs_insert_empty_item().
> >
> > Well, the first return is before calling this function and the final
> > return is only after committing.
> >
> > Again, the function keeps it's former structure as it was before this
> > patch. I still don't see any logical or functional change.
>
> I don't know how to put it simpler to you. There's a path setup by
> btrfs_insert_empty_item(), with a leaf locked in write mode.
> You can't just do a transaction commit while holding it locked, as
> this can deadlock because updating the tree root is part of the
> critical section of a transaction commit.
> It seems trivial to me...

Nah, of course. I see. Simply put the btrfs_free_path() needs to be
called before btrfs_commit_transaction() due to need to
btrfs_tree_unlock_rw() in btrfs_release_path() first.

So the sequence matters. The path implicit destructor on return from
insert_balance_item() is only called too late.

Thanks for the details.

> >
> > I'm lost. This still looks to me just as a straightforward cleanup.
> >
> > > > Is there really any functional change? What am I missing?
> > >
> > > Yes there is, a huge one. Even if a transaction commit didn't need to
> > > update the root tree, it would be performance wise to commit a
> > > transaction while holding a lock on a leaf unnecessarily.
> > >
> > > >
> > > > --nX
> > > >
> > > > > Thanks.
> > > > >
> > > > >
> > > > > >         if (err && !ret)
> > > > > >                 ret = err;
> > > > > > --
> > > > > > 2.39.0
> > > > > >
> > > > > >
> > > > >

回复: [PATCH 1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item()

[李扬韬]

> This isn't a good idea at all.
> We're now committing a transaction while holding a write lock on some leaf of the tree root - this can result in a deadlock as the transaction commit needs to update the tree root (see update_cowonly_root()).>

Thx for pointing out it. I missed it......

Is there anything we need to modify about the del_balance_item function?

Yangtao

Re: [PATCH 1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item()

[David Sterba]

On Tue, Apr 08, 2025 at 06:29:30AM -0600, Yangtao Li wrote:
> All cleanup paths lead to btrfs_path_free so we can define path with the
> automatic free callback.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>  fs/btrfs/volumes.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index c8c21c55be53..a962efaec4ea 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3730,7 +3730,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>  	struct btrfs_trans_handle *trans;
>  	struct btrfs_balance_item *item;
>  	struct btrfs_disk_balance_args disk_bargs;
> -	struct btrfs_path *path;
> +	BTRFS_PATH_AUTO_FREE(path);
>  	struct extent_buffer *leaf;
>  	struct btrfs_key key;
>  	int ret, err;
> @@ -3740,10 +3740,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>  		return -ENOMEM;
>  
>  	trans = btrfs_start_transaction(root, 0);
> -	if (IS_ERR(trans)) {
> -		btrfs_free_path(path);
> +	if (IS_ERR(trans))
>  		return PTR_ERR(trans);
> -	}
>  
>  	key.objectid = BTRFS_BALANCE_OBJECTID;
>  	key.type = BTRFS_TEMPORARY_ITEM_KEY;
> @@ -3767,7 +3765,6 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>  	btrfs_set_balance_sys(leaf, item, &disk_bargs);
>  	btrfs_set_balance_flags(leaf, item, bctl->flags);
>  out:
> -	btrfs_free_path(path);

The pattern where btrfs_free_path() is not called at the end and is
followed by other code is probably not worth converting. It would be
possible to replace it by

	btrfs_release_path(path);
	path = NULL;

that drops the locks and refs from the path but this does not save us
much compared to the straightforward BTRFS_PATH_AUTO_FREE() conversions.
Also release will still keep the object allocated although we don't need
it. As a principle, resources, locks and critical sections should be as
short as possible.

Unfortunatelly I've probably fished all the trivial and almost-trivial
conversions, we don't want 100% use of BTRFS_PATH_AUTO_FREE(), only when
it improves the code.

You may still find cases worth converting, the typical pattern is
btrfs_path_alloc() somewhere near top of the function and
btrfs_free_path() called right before a return.

Re: [PATCH 1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item()

[Filipe Manana]

On Tue, Apr 8, 2025 at 11:50 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Tue, Apr 08, 2025 at 06:29:30AM -0600, Yangtao Li wrote:
> > All cleanup paths lead to btrfs_path_free so we can define path with the
> > automatic free callback.
> >
> > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > ---
> >  fs/btrfs/volumes.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index c8c21c55be53..a962efaec4ea 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -3730,7 +3730,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> >       struct btrfs_trans_handle *trans;
> >       struct btrfs_balance_item *item;
> >       struct btrfs_disk_balance_args disk_bargs;
> > -     struct btrfs_path *path;
> > +     BTRFS_PATH_AUTO_FREE(path);
> >       struct extent_buffer *leaf;
> >       struct btrfs_key key;
> >       int ret, err;
> > @@ -3740,10 +3740,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> >               return -ENOMEM;
> >
> >       trans = btrfs_start_transaction(root, 0);
> > -     if (IS_ERR(trans)) {
> > -             btrfs_free_path(path);
> > +     if (IS_ERR(trans))
> >               return PTR_ERR(trans);
> > -     }
> >
> >       key.objectid = BTRFS_BALANCE_OBJECTID;
> >       key.type = BTRFS_TEMPORARY_ITEM_KEY;
> > @@ -3767,7 +3765,6 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
> >       btrfs_set_balance_sys(leaf, item, &disk_bargs);
> >       btrfs_set_balance_flags(leaf, item, bctl->flags);
> >  out:
> > -     btrfs_free_path(path);
>
> The pattern where btrfs_free_path() is not called at the end and is
> followed by other code is probably not worth converting. It would be
> possible to replace it by
>
>         btrfs_release_path(path);
>         path = NULL;

Should be btrfs_free_path() instead, otherwise it leaks memory.
Or just do the release without setting the path to NULL and leaving
the path auto free.

>
> that drops the locks and refs from the path but this does not save us
> much compared to the straightforward BTRFS_PATH_AUTO_FREE() conversions.
> Also release will still keep the object allocated although we don't need
> it. As a principle, resources, locks and critical sections should be as
> short as possible.
>
> Unfortunatelly I've probably fished all the trivial and almost-trivial
> conversions, we don't want 100% use of BTRFS_PATH_AUTO_FREE(), only when
> it improves the code.
>
> You may still find cases worth converting, the typical pattern is
> btrfs_path_alloc() somewhere near top of the function and
> btrfs_free_path() called right before a return.
>