| Message ID | bb0503f5f3838fed86bdabf8d15ce71561a307fd.1698658344.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: do not utilize goto to implement delayed inode ref deletion | expand |
On Mon, Oct 30, 2023 at 9:36 AM Qu Wenruo <wqu@suse.com> wrote: > > [PROBLEM] > The function __btrfs_update_delayed_inode() is doing something not > meeting the code standard of today: > > path->slots[0]++ > if (path->slots[0] >= btrfs_header_nritems(leaf)) > goto search; > again: > if (!is_the_target_inode_ref()) > goto out; > ret = btrfs_delete_item(); > /* Some cleanup. */ > return ret; > > search: > ret = search_for_the_last_inode_ref(); > goto again; > > With the tag named "again", it's pretty common to think it's a loop, but > the truth is, we only need to do the search once, to locate the last > (also the first, since there should only be one INODE_REF or > INODE_EXTREF now) ref of the inode. > > [FIX] > Instead of the weird jumps, just do them in a stream-lined fashion. > This removes those weird tags, and add extra comments on why we can do > the different searches. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > This is just a cleanup while I was investigating a weird bug inside the > same function. > > The bug is, the mentioned function returned -ENOENT and caused > transaction abort. > The weird part is, when that happened (btrfs_lookup_inode() failed) dump > tree (only one case though) showed there is indeed no INODE_ITEM, but we > still have the INODE_REF and even one EXTENT_DATA. > > Any clue would be very appreciated. > --- > fs/btrfs/delayed-inode.c | 46 ++++++++++++++++++++++------------------ > 1 file changed, 25 insertions(+), 21 deletions(-) > > diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c > index c640f87038a6..efbbe5e0ee6e 100644 > --- a/fs/btrfs/delayed-inode.c > +++ b/fs/btrfs/delayed-inode.c > @@ -1036,14 +1036,34 @@ static int __btrfs_update_delayed_inode(struct btrfs_trans_handle *trans, > if (!test_bit(BTRFS_DELAYED_NODE_DEL_IREF, &node->flags)) > goto out; > > - path->slots[0]++; > - if (path->slots[0] >= btrfs_header_nritems(leaf)) > - goto search; > -again: > + /* > + * Now we're going to delete the INODE_REF/EXTREF, which should be > + * the only one ref left. > + * Check if the next item is an INODE_REF/EXTREF. > + * > + * But if we're the last item already, release and search for the last > + * INODE_REF/EXTREF > + */ > + if (path->slots[0] + 1 >= btrfs_header_nritems(leaf)) { > + key.objectid = node->inode_id; > + key.type = BTRFS_INODE_EXTREF_KEY; > + key.offset = (u64)-1; > + > + btrfs_release_path(path); > + ret = btrfs_search_slot(trans, root, &key, path, -1, 1); > + if (ret < 0) > + goto err_out; > + ASSERT(ret > 0); > + ASSERT(path->slots[0] > 0); > + ret = 0; > + path->slots[0]--; > + } else { > + path->slots[0]++; > + } > + leaf = path->nodes[0]; The assignment of the leaf should be inside the if statement, because otherwise we're in the same leaf, so it's confusing to see it reassigned here. Otherwise it looks fine to me, thanks. > btrfs_item_key_to_cpu(leaf, &key, path->slots[0]); > if (key.objectid != node->inode_id) > goto out; > - > if (key.type != BTRFS_INODE_REF_KEY && > key.type != BTRFS_INODE_EXTREF_KEY) > goto out; > @@ -1070,22 +1090,6 @@ static int __btrfs_update_delayed_inode(struct btrfs_trans_handle *trans, > btrfs_abort_transaction(trans, ret); > > return ret; > - > -search: > - btrfs_release_path(path); > - > - key.type = BTRFS_INODE_EXTREF_KEY; > - key.offset = -1; > - > - ret = btrfs_search_slot(trans, root, &key, path, -1, 1); > - if (ret < 0) > - goto err_out; > - ASSERT(ret); > - > - ret = 0; > - leaf = path->nodes[0]; > - path->slots[0]--; > - goto again; > } > > static inline int btrfs_update_delayed_inode(struct btrfs_trans_handle *trans, > -- > 2.42.0 >
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c index c640f87038a6..efbbe5e0ee6e 100644 --- a/fs/btrfs/delayed-inode.c +++ b/fs/btrfs/delayed-inode.c @@ -1036,14 +1036,34 @@ static int __btrfs_update_delayed_inode(struct btrfs_trans_handle *trans, if (!test_bit(BTRFS_DELAYED_NODE_DEL_IREF, &node->flags)) goto out; - path->slots[0]++; - if (path->slots[0] >= btrfs_header_nritems(leaf)) - goto search; -again: + /* + * Now we're going to delete the INODE_REF/EXTREF, which should be + * the only one ref left. + * Check if the next item is an INODE_REF/EXTREF. + * + * But if we're the last item already, release and search for the last + * INODE_REF/EXTREF + */ + if (path->slots[0] + 1 >= btrfs_header_nritems(leaf)) { + key.objectid = node->inode_id; + key.type = BTRFS_INODE_EXTREF_KEY; + key.offset = (u64)-1; + + btrfs_release_path(path); + ret = btrfs_search_slot(trans, root, &key, path, -1, 1); + if (ret < 0) + goto err_out; + ASSERT(ret > 0); + ASSERT(path->slots[0] > 0); + ret = 0; + path->slots[0]--; + } else { + path->slots[0]++; + } + leaf = path->nodes[0]; btrfs_item_key_to_cpu(leaf, &key, path->slots[0]); if (key.objectid != node->inode_id) goto out; - if (key.type != BTRFS_INODE_REF_KEY && key.type != BTRFS_INODE_EXTREF_KEY) goto out; @@ -1070,22 +1090,6 @@ static int __btrfs_update_delayed_inode(struct btrfs_trans_handle *trans, btrfs_abort_transaction(trans, ret); return ret; - -search: - btrfs_release_path(path); - - key.type = BTRFS_INODE_EXTREF_KEY; - key.offset = -1; - - ret = btrfs_search_slot(trans, root, &key, path, -1, 1); - if (ret < 0) - goto err_out; - ASSERT(ret); - - ret = 0; - leaf = path->nodes[0]; - path->slots[0]--; - goto again; } static inline int btrfs_update_delayed_inode(struct btrfs_trans_handle *trans,
[PROBLEM] The function __btrfs_update_delayed_inode() is doing something not meeting the code standard of today: path->slots[0]++ if (path->slots[0] >= btrfs_header_nritems(leaf)) goto search; again: if (!is_the_target_inode_ref()) goto out; ret = btrfs_delete_item(); /* Some cleanup. */ return ret; search: ret = search_for_the_last_inode_ref(); goto again; With the tag named "again", it's pretty common to think it's a loop, but the truth is, we only need to do the search once, to locate the last (also the first, since there should only be one INODE_REF or INODE_EXTREF now) ref of the inode. [FIX] Instead of the weird jumps, just do them in a stream-lined fashion. This removes those weird tags, and add extra comments on why we can do the different searches. Signed-off-by: Qu Wenruo <wqu@suse.com> --- This is just a cleanup while I was investigating a weird bug inside the same function. The bug is, the mentioned function returned -ENOENT and caused transaction abort. The weird part is, when that happened (btrfs_lookup_inode() failed) dump tree (only one case though) showed there is indeed no INODE_ITEM, but we still have the INODE_REF and even one EXTENT_DATA. Any clue would be very appreciated. --- fs/btrfs/delayed-inode.c | 46 ++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 21 deletions(-)