* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox