| Message ID | 3dfd1a4c01795433444b45268da1ae75563642c1.1686164820.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: some fixes and updates around handling errors for tree mod log operations | expand |
On 2023/6/8 03:24, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > At push_nodes_for_insert(), instead of doing a BUG_ON() in case we fail to > record tree mod log operations, do a transaction abort and return the > error to the caller. There's really no need for the BUG_ON() as we can > release all resources in this context, and we have to abort because other > future tree searches that use the tree mod log (btrfs_search_old_slot()) > may get inconsistent results if other operations modify the tree after > that failure and before the tree mod log based search. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > fs/btrfs/ctree.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > index 2971e7d70d04..e3c949fa136f 100644 > --- a/fs/btrfs/ctree.c > +++ b/fs/btrfs/ctree.c > @@ -1300,7 +1300,12 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans, > btrfs_node_key(mid, &disk_key, 0); > ret = btrfs_tree_mod_log_insert_key(parent, pslot, > BTRFS_MOD_LOG_KEY_REPLACE); > - BUG_ON(ret < 0); > + if (ret < 0) { > + btrfs_tree_unlock(left); > + free_extent_buffer(left); I'm not sure if we only need to unlock and free @left. As just lines below, we have a two branches which one unlock and free @mid. Thanks, Qu > + btrfs_abort_transaction(trans, ret); > + return ret; > + } > btrfs_set_node_key(parent, &disk_key, pslot); > btrfs_mark_buffer_dirty(parent); > if (btrfs_header_nritems(left) > orig_slot) { > @@ -1355,7 +1360,12 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans, > btrfs_node_key(right, &disk_key, 0); > ret = btrfs_tree_mod_log_insert_key(parent, pslot + 1, > BTRFS_MOD_LOG_KEY_REPLACE); > - BUG_ON(ret < 0); > + if (ret < 0) { > + btrfs_tree_unlock(right); > + free_extent_buffer(right); > + btrfs_abort_transaction(trans, ret); > + return ret; > + } > btrfs_set_node_key(parent, &disk_key, pslot + 1); > btrfs_mark_buffer_dirty(parent); >
On Thu, Jun 8, 2023 at 10:02 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > On 2023/6/8 03:24, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > At push_nodes_for_insert(), instead of doing a BUG_ON() in case we fail to > > record tree mod log operations, do a transaction abort and return the > > error to the caller. There's really no need for the BUG_ON() as we can > > release all resources in this context, and we have to abort because other > > future tree searches that use the tree mod log (btrfs_search_old_slot()) > > may get inconsistent results if other operations modify the tree after > > that failure and before the tree mod log based search. > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > --- > > fs/btrfs/ctree.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > > index 2971e7d70d04..e3c949fa136f 100644 > > --- a/fs/btrfs/ctree.c > > +++ b/fs/btrfs/ctree.c > > @@ -1300,7 +1300,12 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans, > > btrfs_node_key(mid, &disk_key, 0); > > ret = btrfs_tree_mod_log_insert_key(parent, pslot, > > BTRFS_MOD_LOG_KEY_REPLACE); > > - BUG_ON(ret < 0); > > + if (ret < 0) { > > + btrfs_tree_unlock(left); > > + free_extent_buffer(left); > > I'm not sure if we only need to unlock and free @left. > > As just lines below, we have a two branches which one unlock and free @mid. mid is part of the path, so we shouldn't touch it at this point. It's released by the caller doing a btrfs_release_path() or btrfs_free_path(). Those branches unlock and free mid because they are replacing it in the path. Thanks. > > Thanks, > Qu > > > + btrfs_abort_transaction(trans, ret); > > + return ret; > > + } > > btrfs_set_node_key(parent, &disk_key, pslot); > > btrfs_mark_buffer_dirty(parent); > > if (btrfs_header_nritems(left) > orig_slot) { > > @@ -1355,7 +1360,12 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans, > > btrfs_node_key(right, &disk_key, 0); > > ret = btrfs_tree_mod_log_insert_key(parent, pslot + 1, > > BTRFS_MOD_LOG_KEY_REPLACE); > > - BUG_ON(ret < 0); > > + if (ret < 0) { > > + btrfs_tree_unlock(right); > > + free_extent_buffer(right); > > + btrfs_abort_transaction(trans, ret); > > + return ret; > > + } > > btrfs_set_node_key(parent, &disk_key, pslot + 1); > > btrfs_mark_buffer_dirty(parent); > >
On 2023/6/8 17:46, Filipe Manana wrote: > On Thu, Jun 8, 2023 at 10:02 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: >> >> >> >> On 2023/6/8 03:24, fdmanana@kernel.org wrote: >>> From: Filipe Manana <fdmanana@suse.com> >>> >>> At push_nodes_for_insert(), instead of doing a BUG_ON() in case we fail to >>> record tree mod log operations, do a transaction abort and return the >>> error to the caller. There's really no need for the BUG_ON() as we can >>> release all resources in this context, and we have to abort because other >>> future tree searches that use the tree mod log (btrfs_search_old_slot()) >>> may get inconsistent results if other operations modify the tree after >>> that failure and before the tree mod log based search. >>> >>> Signed-off-by: Filipe Manana <fdmanana@suse.com> >>> --- >>> fs/btrfs/ctree.c | 14 ++++++++++++-- >>> 1 file changed, 12 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c >>> index 2971e7d70d04..e3c949fa136f 100644 >>> --- a/fs/btrfs/ctree.c >>> +++ b/fs/btrfs/ctree.c >>> @@ -1300,7 +1300,12 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans, >>> btrfs_node_key(mid, &disk_key, 0); >>> ret = btrfs_tree_mod_log_insert_key(parent, pslot, >>> BTRFS_MOD_LOG_KEY_REPLACE); >>> - BUG_ON(ret < 0); >>> + if (ret < 0) { >>> + btrfs_tree_unlock(left); >>> + free_extent_buffer(left); >> >> I'm not sure if we only need to unlock and free @left. >> >> As just lines below, we have a two branches which one unlock and free @mid. > > mid is part of the path, so we shouldn't touch it at this point. > It's released by the caller doing a btrfs_release_path() or btrfs_free_path(). > > Those branches unlock and free mid because they are replacing it in the path. Thanks for the detailed reason. Then this looks good to me. Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > > Thanks. > >> >> Thanks, >> Qu >> >>> + btrfs_abort_transaction(trans, ret); >>> + return ret; >>> + } >>> btrfs_set_node_key(parent, &disk_key, pslot); >>> btrfs_mark_buffer_dirty(parent); >>> if (btrfs_header_nritems(left) > orig_slot) { >>> @@ -1355,7 +1360,12 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans, >>> btrfs_node_key(right, &disk_key, 0); >>> ret = btrfs_tree_mod_log_insert_key(parent, pslot + 1, >>> BTRFS_MOD_LOG_KEY_REPLACE); >>> - BUG_ON(ret < 0); >>> + if (ret < 0) { >>> + btrfs_tree_unlock(right); >>> + free_extent_buffer(right); >>> + btrfs_abort_transaction(trans, ret); >>> + return ret; >>> + } >>> btrfs_set_node_key(parent, &disk_key, pslot + 1); >>> btrfs_mark_buffer_dirty(parent); >>>
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index 2971e7d70d04..e3c949fa136f 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -1300,7 +1300,12 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans, btrfs_node_key(mid, &disk_key, 0); ret = btrfs_tree_mod_log_insert_key(parent, pslot, BTRFS_MOD_LOG_KEY_REPLACE); - BUG_ON(ret < 0); + if (ret < 0) { + btrfs_tree_unlock(left); + free_extent_buffer(left); + btrfs_abort_transaction(trans, ret); + return ret; + } btrfs_set_node_key(parent, &disk_key, pslot); btrfs_mark_buffer_dirty(parent); if (btrfs_header_nritems(left) > orig_slot) { @@ -1355,7 +1360,12 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans, btrfs_node_key(right, &disk_key, 0); ret = btrfs_tree_mod_log_insert_key(parent, pslot + 1, BTRFS_MOD_LOG_KEY_REPLACE); - BUG_ON(ret < 0); + if (ret < 0) { + btrfs_tree_unlock(right); + free_extent_buffer(right); + btrfs_abort_transaction(trans, ret); + return ret; + } btrfs_set_node_key(parent, &disk_key, pslot + 1); btrfs_mark_buffer_dirty(parent);