| Message ID | 20250408122933.121056-1-frank.li@vivo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item() | expand |
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 > >
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 > > > > >
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 > > > > > > > >
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 > > > > > > > > > > >
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 > > > > > > > > > > > > > >
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 > > > > > > > > > > > > > > > > >
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.
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. >
> 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
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;
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(-)