| Message ID | e81eab657c200a78dd43747fb28e942289082f98.1698698978.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: do not utilize goto to implement delayed inode ref deletion | expand |
On Mon, Oct 30, 2023 at 9:07 PM 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> Reviewed-by: Filipe Manana <fdmanana@suse.com> Looks good, thanks. > --- > CHANGELOG > v2: > - Move the leaf assignment into the if branch where we do the search > This is where the leaf get updated, no need to update @leaf > unconditionally which can be confusing. > > 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..0f8fa9751b5d 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]--; > + leaf = path->nodes[0]; > + } else { > + path->slots[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, > -- > 2.42.0 >
On Tue, Oct 31, 2023 at 07:37:20AM +1030, Qu Wenruo 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> Added to misc-next, thanks.
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c index c640f87038a6..0f8fa9751b5d 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]--; + leaf = path->nodes[0]; + } else { + path->slots[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> --- CHANGELOG v2: - Move the leaf assignment into the if branch where we do the search This is where the leaf get updated, no need to update @leaf unconditionally which can be confusing. 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(-)