* [PATCH 0/2] btrfs: fix lost error value deleting root reference
@ 2022-08-22 11:53 fdmanana
2022-08-22 11:53 ` [PATCH 1/2] btrfs: fix silent failure when " fdmanana
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: fdmanana @ 2022-08-22 11:53 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
Fix a silent failure in case deleting a root reference fails when searching
for an item. Then make btrfs_del_root_ref() less likely to run into such
type of bug in the future. Details in the changelogs.
Filipe Manana (2):
btrfs: fix silent failure when deleting root reference
btrfs: simplify error handling at btrfs_del_root_ref()
fs/btrfs/root-tree.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
--
2.35.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] btrfs: fix silent failure when deleting root reference 2022-08-22 11:53 [PATCH 0/2] btrfs: fix lost error value deleting root reference fdmanana @ 2022-08-22 11:53 ` fdmanana 2022-08-22 11:53 ` [PATCH 2/2] btrfs: simplify error handling at btrfs_del_root_ref() fdmanana 2022-08-22 14:47 ` [PATCH v2 0/2] btrfs: fix lost error value deleting root reference fdmanana 2 siblings, 0 replies; 12+ messages in thread From: fdmanana @ 2022-08-22 11:53 UTC (permalink / raw) To: linux-btrfs From: Filipe Manana <fdmanana@suse.com> At btrfs_del_root_ref(), if btrfs_search_slot() returns an error, we end up returning from the function with a value of 0 (success). This happens because the function returns the value stored in the variable 'err', which is 0, while the error value we got from btrfs_search_slot() is stored in the 'ret' variable. So fix it by setting 'err' with the error value. Fixes: 8289ed9f93bef2 ("btrfs: replace the BUG_ON in btrfs_del_root_ref with proper error handling") Signed-off-by: Filipe Manana <fdmanana@suse.com> --- fs/btrfs/root-tree.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c index a64b26b16904..d647cb2938c0 100644 --- a/fs/btrfs/root-tree.c +++ b/fs/btrfs/root-tree.c @@ -349,9 +349,10 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, key.offset = ref_id; again: ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); - if (ret < 0) + if (ret < 0) { + err = ret; goto out; - if (ret == 0) { + } else if (ret == 0) { leaf = path->nodes[0]; ref = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_root_ref); -- 2.35.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] btrfs: simplify error handling at btrfs_del_root_ref() 2022-08-22 11:53 [PATCH 0/2] btrfs: fix lost error value deleting root reference fdmanana 2022-08-22 11:53 ` [PATCH 1/2] btrfs: fix silent failure when " fdmanana @ 2022-08-22 11:53 ` fdmanana 2022-08-22 14:47 ` [PATCH v2 0/2] btrfs: fix lost error value deleting root reference fdmanana 2 siblings, 0 replies; 12+ messages in thread From: fdmanana @ 2022-08-22 11:53 UTC (permalink / raw) To: linux-btrfs From: Filipe Manana <fdmanana@suse.com> At btrfs_del_root_ref() we are using two return variables, named 'ret' and 'err'. This makes it harder to follow and easier to return the wrong value in case an error happens - the previous patch in the series, which has the subject "btrfs: fix silent failure when deleting root reference", fixed a bug due to confusion created by these two variables. So change the function to use a single variable for tracking the return value of the function, using only 'ret', which is consistent with most of the codebase. Signed-off-by: Filipe Manana <fdmanana@suse.com> --- fs/btrfs/root-tree.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c index d647cb2938c0..3975a24c3fdc 100644 --- a/fs/btrfs/root-tree.c +++ b/fs/btrfs/root-tree.c @@ -337,7 +337,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, struct extent_buffer *leaf; struct btrfs_key key; unsigned long ptr; - int err = 0; int ret; path = btrfs_alloc_path(); @@ -350,7 +349,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, again: ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); if (ret < 0) { - err = ret; goto out; } else if (ret == 0) { leaf = path->nodes[0]; @@ -360,18 +358,28 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, if ((btrfs_root_ref_dirid(leaf, ref) != dirid) || (btrfs_root_ref_name_len(leaf, ref) != name_len) || memcmp_extent_buffer(leaf, name, ptr, name_len)) { - err = -ENOENT; + ret = -ENOENT; goto out; } *sequence = btrfs_root_ref_sequence(leaf, ref); ret = btrfs_del_item(trans, tree_root, path); - if (ret) { - err = ret; + if (ret) + goto out; + } else { + if (key.type == BTRFS_ROOT_REF_KEY) { + /* + * We've searched for a BTRFS_ROOT_BACKREF_KEY item and + * for a BTRFS_ROOT_REF_KEY item, so error out. + */ + ret = -ENOENT; goto out; } - } else - err = -ENOENT; + /* + * We have only searched for a BTRFS_ROOT_BACKREF_KEY item, so + * fallback below to search for BTRFS_ROOT_REF_KEY item. + */ + } if (key.type == BTRFS_ROOT_BACKREF_KEY) { btrfs_release_path(path); @@ -383,7 +391,7 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, out: btrfs_free_path(path); - return err; + return ret; } /* -- 2.35.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 0/2] btrfs: fix lost error value deleting root reference 2022-08-22 11:53 [PATCH 0/2] btrfs: fix lost error value deleting root reference fdmanana 2022-08-22 11:53 ` [PATCH 1/2] btrfs: fix silent failure when " fdmanana 2022-08-22 11:53 ` [PATCH 2/2] btrfs: simplify error handling at btrfs_del_root_ref() fdmanana @ 2022-08-22 14:47 ` fdmanana 2022-08-22 14:47 ` [PATCH v2 1/2] btrfs: fix silent failure when " fdmanana ` (2 more replies) 2 siblings, 3 replies; 12+ messages in thread From: fdmanana @ 2022-08-22 14:47 UTC (permalink / raw) To: linux-btrfs From: Filipe Manana <fdmanana@suse.com> Fix a silent failure in case deleting a root reference fails when searching for an item. Then make btrfs_del_root_ref() less likely to run into such type of bug in the future. Details in the changelogs. V2: Fix patch 2/2. If the BTRFS_ROOT_BACKREF_KEY item is not found, but the BTRFS_ROOT_REF_KEY is found, then before we would return -ENOENT and now we were returning 0 (unless an error happened when deleting the BTRFS_ROOT_REF_KEY item). Just return -ENOENT whenever one of the items is not found, all the callers abort the transaction if btrfs_del_root_ref() returns an error. Filipe Manana (2): btrfs: fix silent failure when deleting root reference btrfs: simplify error handling at btrfs_del_root_ref() fs/btrfs/root-tree.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) -- 2.35.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] btrfs: fix silent failure when deleting root reference 2022-08-22 14:47 ` [PATCH v2 0/2] btrfs: fix lost error value deleting root reference fdmanana @ 2022-08-22 14:47 ` fdmanana 2022-08-22 23:47 ` Qu Wenruo 2022-08-22 14:47 ` [PATCH v2 2/2] btrfs: simplify error handling at btrfs_del_root_ref() fdmanana 2022-08-22 18:30 ` [PATCH v2 0/2] btrfs: fix lost error value deleting root reference David Sterba 2 siblings, 1 reply; 12+ messages in thread From: fdmanana @ 2022-08-22 14:47 UTC (permalink / raw) To: linux-btrfs From: Filipe Manana <fdmanana@suse.com> At btrfs_del_root_ref(), if btrfs_search_slot() returns an error, we end up returning from the function with a value of 0 (success). This happens because the function returns the value stored in the variable 'err', which is 0, while the error value we got from btrfs_search_slot() is stored in the 'ret' variable. So fix it by setting 'err' with the error value. Fixes: 8289ed9f93bef2 ("btrfs: replace the BUG_ON in btrfs_del_root_ref with proper error handling") Signed-off-by: Filipe Manana <fdmanana@suse.com> --- fs/btrfs/root-tree.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c index a64b26b16904..d647cb2938c0 100644 --- a/fs/btrfs/root-tree.c +++ b/fs/btrfs/root-tree.c @@ -349,9 +349,10 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, key.offset = ref_id; again: ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); - if (ret < 0) + if (ret < 0) { + err = ret; goto out; - if (ret == 0) { + } else if (ret == 0) { leaf = path->nodes[0]; ref = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_root_ref); -- 2.35.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] btrfs: fix silent failure when deleting root reference 2022-08-22 14:47 ` [PATCH v2 1/2] btrfs: fix silent failure when " fdmanana @ 2022-08-22 23:47 ` Qu Wenruo 2022-08-23 8:11 ` Filipe Manana 0 siblings, 1 reply; 12+ messages in thread From: Qu Wenruo @ 2022-08-22 23:47 UTC (permalink / raw) To: fdmanana, linux-btrfs On 2022/8/22 22:47, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > At btrfs_del_root_ref(), if btrfs_search_slot() returns an error, we end > up returning from the function with a value of 0 (success). This happens > because the function returns the value stored in the variable 'err', which > is 0, while the error value we got from btrfs_search_slot() is stored in > the 'ret' variable. > > So fix it by setting 'err' with the error value. > > Fixes: 8289ed9f93bef2 ("btrfs: replace the BUG_ON in btrfs_del_root_ref with proper error handling") > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks for catching this, Qu > --- > fs/btrfs/root-tree.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > index a64b26b16904..d647cb2938c0 100644 > --- a/fs/btrfs/root-tree.c > +++ b/fs/btrfs/root-tree.c > @@ -349,9 +349,10 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > key.offset = ref_id; > again: > ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); > - if (ret < 0) > + if (ret < 0) { > + err = ret; > goto out; > - if (ret == 0) { Just a small nitpick here, since above if (ret < 0) branch will call "goto out", we don't need the "else" branch. The old "if (ret == 0) {" should be good enough. Thanks, Qu > + } else if (ret == 0) { > leaf = path->nodes[0]; > ref = btrfs_item_ptr(leaf, path->slots[0], > struct btrfs_root_ref); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] btrfs: fix silent failure when deleting root reference 2022-08-22 23:47 ` Qu Wenruo @ 2022-08-23 8:11 ` Filipe Manana 2022-08-23 17:15 ` David Sterba 0 siblings, 1 reply; 12+ messages in thread From: Filipe Manana @ 2022-08-23 8:11 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs On Tue, Aug 23, 2022 at 12:47 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > On 2022/8/22 22:47, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > At btrfs_del_root_ref(), if btrfs_search_slot() returns an error, we end > > up returning from the function with a value of 0 (success). This happens > > because the function returns the value stored in the variable 'err', which > > is 0, while the error value we got from btrfs_search_slot() is stored in > > the 'ret' variable. > > > > So fix it by setting 'err' with the error value. > > > > Fixes: 8289ed9f93bef2 ("btrfs: replace the BUG_ON in btrfs_del_root_ref with proper error handling") > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Thanks for catching this, > Qu > > --- > > fs/btrfs/root-tree.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > > index a64b26b16904..d647cb2938c0 100644 > > --- a/fs/btrfs/root-tree.c > > +++ b/fs/btrfs/root-tree.c > > @@ -349,9 +349,10 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > key.offset = ref_id; > > again: > > ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); > > - if (ret < 0) > > + if (ret < 0) { > > + err = ret; > > goto out; > > - if (ret == 0) { > > Just a small nitpick here, since above if (ret < 0) branch will call > "goto out", we don't need the "else" branch. > The old "if (ret == 0) {" should be good enough. Yes, it's not about correctness. It's just my preferred style. I find it more intuitive to have a single if-else that tests the same variable instead of having it tested by two distinct if statements. > > Thanks, > Qu > > > + } else if (ret == 0) { > > leaf = path->nodes[0]; > > ref = btrfs_item_ptr(leaf, path->slots[0], > > struct btrfs_root_ref); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] btrfs: fix silent failure when deleting root reference 2022-08-23 8:11 ` Filipe Manana @ 2022-08-23 17:15 ` David Sterba 0 siblings, 0 replies; 12+ messages in thread From: David Sterba @ 2022-08-23 17:15 UTC (permalink / raw) To: Filipe Manana; +Cc: Qu Wenruo, linux-btrfs On Tue, Aug 23, 2022 at 09:11:55AM +0100, Filipe Manana wrote: > On Tue, Aug 23, 2022 at 12:47 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > --- > > > fs/btrfs/root-tree.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > > > index a64b26b16904..d647cb2938c0 100644 > > > --- a/fs/btrfs/root-tree.c > > > +++ b/fs/btrfs/root-tree.c > > > @@ -349,9 +349,10 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > > key.offset = ref_id; > > > again: > > > ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); > > > - if (ret < 0) > > > + if (ret < 0) { > > > + err = ret; > > > goto out; > > > - if (ret == 0) { > > > > Just a small nitpick here, since above if (ret < 0) branch will call > > "goto out", we don't need the "else" branch. > > The old "if (ret == 0) {" should be good enough. > > Yes, it's not about correctness. It's just my preferred style. > I find it more intuitive to have a single if-else that tests the same > variable instead > of having it tested by two distinct if statements. Agreed with that style. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] btrfs: simplify error handling at btrfs_del_root_ref() 2022-08-22 14:47 ` [PATCH v2 0/2] btrfs: fix lost error value deleting root reference fdmanana 2022-08-22 14:47 ` [PATCH v2 1/2] btrfs: fix silent failure when " fdmanana @ 2022-08-22 14:47 ` fdmanana 2022-08-22 23:54 ` Qu Wenruo 2022-08-22 18:30 ` [PATCH v2 0/2] btrfs: fix lost error value deleting root reference David Sterba 2 siblings, 1 reply; 12+ messages in thread From: fdmanana @ 2022-08-22 14:47 UTC (permalink / raw) To: linux-btrfs From: Filipe Manana <fdmanana@suse.com> At btrfs_del_root_ref() we are using two return variables, named 'ret' and 'err'. This makes it harder to follow and easier to return the wrong value in case an error happens - the previous patch in the series, which has the subject "btrfs: fix silent failure when deleting root reference", fixed a bug due to confusion created by these two variables. So change the function to use a single variable for tracking the return value of the function, using only 'ret', which is consistent with most of the codebase. Signed-off-by: Filipe Manana <fdmanana@suse.com> --- fs/btrfs/root-tree.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c index d647cb2938c0..e1f599d7a916 100644 --- a/fs/btrfs/root-tree.c +++ b/fs/btrfs/root-tree.c @@ -337,7 +337,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, struct extent_buffer *leaf; struct btrfs_key key; unsigned long ptr; - int err = 0; int ret; path = btrfs_alloc_path(); @@ -350,7 +349,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, again: ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); if (ret < 0) { - err = ret; goto out; } else if (ret == 0) { leaf = path->nodes[0]; @@ -360,18 +358,18 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, if ((btrfs_root_ref_dirid(leaf, ref) != dirid) || (btrfs_root_ref_name_len(leaf, ref) != name_len) || memcmp_extent_buffer(leaf, name, ptr, name_len)) { - err = -ENOENT; + ret = -ENOENT; goto out; } *sequence = btrfs_root_ref_sequence(leaf, ref); ret = btrfs_del_item(trans, tree_root, path); - if (ret) { - err = ret; + if (ret) goto out; - } - } else - err = -ENOENT; + } else { + ret = -ENOENT; + goto out; + } if (key.type == BTRFS_ROOT_BACKREF_KEY) { btrfs_release_path(path); @@ -383,7 +381,7 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, out: btrfs_free_path(path); - return err; + return ret; } /* -- 2.35.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] btrfs: simplify error handling at btrfs_del_root_ref() 2022-08-22 14:47 ` [PATCH v2 2/2] btrfs: simplify error handling at btrfs_del_root_ref() fdmanana @ 2022-08-22 23:54 ` Qu Wenruo 2022-08-23 8:09 ` Filipe Manana 0 siblings, 1 reply; 12+ messages in thread From: Qu Wenruo @ 2022-08-22 23:54 UTC (permalink / raw) To: fdmanana, linux-btrfs On 2022/8/22 22:47, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > At btrfs_del_root_ref() we are using two return variables, named 'ret' and > 'err'. This makes it harder to follow and easier to return the wrong value > in case an error happens - the previous patch in the series, which has the > subject "btrfs: fix silent failure when deleting root reference", fixed a > bug due to confusion created by these two variables. > > So change the function to use a single variable for tracking the return > value of the function, using only 'ret', which is consistent with most of > the codebase. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Although one small nitpick inlined below. > --- > fs/btrfs/root-tree.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > index d647cb2938c0..e1f599d7a916 100644 > --- a/fs/btrfs/root-tree.c > +++ b/fs/btrfs/root-tree.c > @@ -337,7 +337,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > struct extent_buffer *leaf; > struct btrfs_key key; > unsigned long ptr; > - int err = 0; > int ret; > > path = btrfs_alloc_path(); > @@ -350,7 +349,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > again: > ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); > if (ret < 0) { > - err = ret; > goto out; > } else if (ret == 0) { > leaf = path->nodes[0]; > @@ -360,18 +358,18 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > if ((btrfs_root_ref_dirid(leaf, ref) != dirid) || > (btrfs_root_ref_name_len(leaf, ref) != name_len) || > memcmp_extent_buffer(leaf, name, ptr, name_len)) { > - err = -ENOENT; > + ret = -ENOENT; > goto out; > } > *sequence = btrfs_root_ref_sequence(leaf, ref); > > ret = btrfs_del_item(trans, tree_root, path); > - if (ret) { > - err = ret; > + if (ret) > goto out; > - } > - } else > - err = -ENOENT; > + } else { > + ret = -ENOENT; > + goto out; > + } > > if (key.type == BTRFS_ROOT_BACKREF_KEY) { > btrfs_release_path(path); To the if () check here can also be a cause of confusion. Can we split it into two dedicated btrfs_search_slot() calls (instead of current goto again with different keys) in a separate patch? I guess that's why the v1 version had some error got overriden, right? Thanks, Qu > @@ -383,7 +381,7 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > out: > btrfs_free_path(path); > - return err; > + return ret; > } > > /* ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] btrfs: simplify error handling at btrfs_del_root_ref() 2022-08-22 23:54 ` Qu Wenruo @ 2022-08-23 8:09 ` Filipe Manana 0 siblings, 0 replies; 12+ messages in thread From: Filipe Manana @ 2022-08-23 8:09 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs On Tue, Aug 23, 2022 at 07:54:12AM +0800, Qu Wenruo wrote: > > > On 2022/8/22 22:47, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > At btrfs_del_root_ref() we are using two return variables, named 'ret' and > > 'err'. This makes it harder to follow and easier to return the wrong value > > in case an error happens - the previous patch in the series, which has the > > subject "btrfs: fix silent failure when deleting root reference", fixed a > > bug due to confusion created by these two variables. > > > > So change the function to use a single variable for tracking the return > > value of the function, using only 'ret', which is consistent with most of > > the codebase. > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Although one small nitpick inlined below. > > > --- > > fs/btrfs/root-tree.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > > index d647cb2938c0..e1f599d7a916 100644 > > --- a/fs/btrfs/root-tree.c > > +++ b/fs/btrfs/root-tree.c > > @@ -337,7 +337,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > struct extent_buffer *leaf; > > struct btrfs_key key; > > unsigned long ptr; > > - int err = 0; > > int ret; > > > > path = btrfs_alloc_path(); > > @@ -350,7 +349,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > again: > > ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); > > if (ret < 0) { > > - err = ret; > > goto out; > > } else if (ret == 0) { > > leaf = path->nodes[0]; > > @@ -360,18 +358,18 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > if ((btrfs_root_ref_dirid(leaf, ref) != dirid) || > > (btrfs_root_ref_name_len(leaf, ref) != name_len) || > > memcmp_extent_buffer(leaf, name, ptr, name_len)) { > > - err = -ENOENT; > > + ret = -ENOENT; > > goto out; > > } > > *sequence = btrfs_root_ref_sequence(leaf, ref); > > > > ret = btrfs_del_item(trans, tree_root, path); > > - if (ret) { > > - err = ret; > > + if (ret) > > goto out; > > - } > > - } else > > - err = -ENOENT; > > + } else { > > + ret = -ENOENT; > > + goto out; > > + } > > > > if (key.type == BTRFS_ROOT_BACKREF_KEY) { > > btrfs_release_path(path); > > To the if () check here can also be a cause of confusion. > > Can we split it into two dedicated btrfs_search_slot() calls (instead of > current goto again with different keys) in a separate patch? > > I guess that's why the v1 version had some error got overriden, right? The problem in v1 was because I didn't think properly. I was under the wrong idea that we could have either one key type or the other, that it was to deal with some old format - like the v0 backref thing. Then I realized we got all v0 compatibility stuff removed some years ago, and that this is something different - both keys must always exist. In other words, it was not because of the if + goto or because of having two variables for the return value. I'm not sure getting rid of the if + goto logic and duplicating the deletion is better. It would duplicate a lot of code. Either way, my intention of this patch is really just to have a single variable for the return value instead of two. > > Thanks, > Qu > > @@ -383,7 +381,7 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > > > out: > > btrfs_free_path(path); > > - return err; > > + return ret; > > } > > > > /* ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/2] btrfs: fix lost error value deleting root reference 2022-08-22 14:47 ` [PATCH v2 0/2] btrfs: fix lost error value deleting root reference fdmanana 2022-08-22 14:47 ` [PATCH v2 1/2] btrfs: fix silent failure when " fdmanana 2022-08-22 14:47 ` [PATCH v2 2/2] btrfs: simplify error handling at btrfs_del_root_ref() fdmanana @ 2022-08-22 18:30 ` David Sterba 2 siblings, 0 replies; 12+ messages in thread From: David Sterba @ 2022-08-22 18:30 UTC (permalink / raw) To: fdmanana; +Cc: linux-btrfs On Mon, Aug 22, 2022 at 03:47:08PM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > Fix a silent failure in case deleting a root reference fails when searching > for an item. Then make btrfs_del_root_ref() less likely to run into such > type of bug in the future. Details in the changelogs. > > V2: Fix patch 2/2. If the BTRFS_ROOT_BACKREF_KEY item is not found, but > the BTRFS_ROOT_REF_KEY is found, then before we would return -ENOENT > and now we were returning 0 (unless an error happened when deleting > the BTRFS_ROOT_REF_KEY item). Just return -ENOENT whenever one of > the items is not found, all the callers abort the transaction if > btrfs_del_root_ref() returns an error. > > Filipe Manana (2): > btrfs: fix silent failure when deleting root reference > btrfs: simplify error handling at btrfs_del_root_ref() Added to misc-next, thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-08-23 18:55 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-08-22 11:53 [PATCH 0/2] btrfs: fix lost error value deleting root reference fdmanana 2022-08-22 11:53 ` [PATCH 1/2] btrfs: fix silent failure when " fdmanana 2022-08-22 11:53 ` [PATCH 2/2] btrfs: simplify error handling at btrfs_del_root_ref() fdmanana 2022-08-22 14:47 ` [PATCH v2 0/2] btrfs: fix lost error value deleting root reference fdmanana 2022-08-22 14:47 ` [PATCH v2 1/2] btrfs: fix silent failure when " fdmanana 2022-08-22 23:47 ` Qu Wenruo 2022-08-23 8:11 ` Filipe Manana 2022-08-23 17:15 ` David Sterba 2022-08-22 14:47 ` [PATCH v2 2/2] btrfs: simplify error handling at btrfs_del_root_ref() fdmanana 2022-08-22 23:54 ` Qu Wenruo 2022-08-23 8:09 ` Filipe Manana 2022-08-22 18:30 ` [PATCH v2 0/2] btrfs: fix lost error value deleting root reference David Sterba
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.