| Message ID | 20190910074019.23158-2-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Small code style cleanup for ctree.c | expand |
This patch also adds line stretching until 80 chars.
looks good.
Reviewed-by: Anand Jain <anand.jain@oracle.com>
On 10.09.19 г. 10:40 ч., Qu Wenruo wrote: > In btrfs_search_slot(), we something like: > > if (level != 0) { > /* Do search inside tree nodes*/ > } else { > /* Do search inside tree leaves */ > goto done; > } > > This caused extra indent for tree node search code. > Change it to something like: > > if (level == 0) { > /* Do search inside tree leaves */ > goto done' > } > /* Do search inside tree nodes */ > > So we have more space to maneuver our code, this is especially useful as > the tree nodes search code is more complex than the leaves search code. > > Signed-off-by: Qu Wenruo <wqu@suse.com> I actually thing this patch makes comprehending the function worse. Because the else is now somewhat implicit. E.g. one has to pay careful attention to the contents inside the first if and especially the unconditional 'goto done' to be able to understand the code after the 'if' construct.
On 2019/9/10 下午4:24, Nikolay Borisov wrote: > > > On 10.09.19 г. 10:40 ч., Qu Wenruo wrote: >> In btrfs_search_slot(), we something like: >> >> if (level != 0) { >> /* Do search inside tree nodes*/ >> } else { >> /* Do search inside tree leaves */ >> goto done; >> } >> >> This caused extra indent for tree node search code. >> Change it to something like: >> >> if (level == 0) { >> /* Do search inside tree leaves */ >> goto done' >> } >> /* Do search inside tree nodes */ >> >> So we have more space to maneuver our code, this is especially useful as >> the tree nodes search code is more complex than the leaves search code. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > I actually thing this patch makes comprehending the function worse. If the level == 0 lines is over 50 lines, maybe. But it's just 22 lines. > Because the else is now somewhat implicit. E.g. one has to pay careful > attention to the contents inside the first if and especially the > unconditional 'goto done' to be able to understand the code after the > 'if' construct. That's the same for the original code, you need to go a level upper to see we're in level > 0 branch. Thanks, Qu >
On 10.09.19 г. 11:31 ч., Qu Wenruo wrote: > > > On 2019/9/10 下午4:24, Nikolay Borisov wrote: >> >> >> On 10.09.19 г. 10:40 ч., Qu Wenruo wrote: >>> In btrfs_search_slot(), we something like: >>> >>> if (level != 0) { >>> /* Do search inside tree nodes*/ >>> } else { >>> /* Do search inside tree leaves */ >>> goto done; >>> } >>> >>> This caused extra indent for tree node search code. >>> Change it to something like: >>> >>> if (level == 0) { >>> /* Do search inside tree leaves */ >>> goto done' >>> } >>> /* Do search inside tree nodes */ >>> >>> So we have more space to maneuver our code, this is especially useful as >>> the tree nodes search code is more complex than the leaves search code. >>> >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >> >> I actually thing this patch makes comprehending the function worse. > > If the level == 0 lines is over 50 lines, maybe. > > But it's just 22 lines. >> Because the else is now somewhat implicit. E.g. one has to pay careful >> attention to the contents inside the first if and especially the >> unconditional 'goto done' to be able to understand the code after the >> 'if' construct. > > That's the same for the original code, you need to go a level upper to > see we're in level > 0 branch. But that's explicit with the 'if' > > Thanks, > Qu >> >
On Tue, Sep 10, 2019 at 11:42:47AM +0300, Nikolay Borisov wrote: > > > On 10.09.19 г. 11:31 ч., Qu Wenruo wrote: > > > > > > On 2019/9/10 下午4:24, Nikolay Borisov wrote: > >> > >> > >> On 10.09.19 г. 10:40 ч., Qu Wenruo wrote: > >>> In btrfs_search_slot(), we something like: > >>> > >>> if (level != 0) { > >>> /* Do search inside tree nodes*/ > >>> } else { > >>> /* Do search inside tree leaves */ > >>> goto done; > >>> } > >>> > >>> This caused extra indent for tree node search code. > >>> Change it to something like: > >>> > >>> if (level == 0) { > >>> /* Do search inside tree leaves */ > >>> goto done' > >>> } > >>> /* Do search inside tree nodes */ > >>> > >>> So we have more space to maneuver our code, this is especially useful as > >>> the tree nodes search code is more complex than the leaves search code. > >>> > >>> Signed-off-by: Qu Wenruo <wqu@suse.com> > >> > >> I actually thing this patch makes comprehending the function worse. > > > > If the level == 0 lines is over 50 lines, maybe. > > > > But it's just 22 lines. > >> Because the else is now somewhat implicit. E.g. one has to pay careful > >> attention to the contents inside the first if and especially the > >> unconditional 'goto done' to be able to understand the code after the > >> 'if' construct. > > > > That's the same for the original code, you need to go a level upper to > > see we're in level > 0 branch. > > But that's explicit with the 'if' Well, I don't see a strong reason for one or another. I see your point about the explicit 'if/else' for a condition that has two exclusive options. I looked at the code with the patch applied and regarding readability, the if (level == 0) block is short enough to be seen at once and is an 'take a shortcut here'. The indentation level reduction improvement seems justified to me.
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index 5df76c17775a..1e29183cdf62 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -2761,6 +2761,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, } while (b) { + int dec = 0; level = btrfs_header_level(b); /* @@ -2837,75 +2838,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, if (ret < 0) goto done; - if (level != 0) { - int dec = 0; - if (ret && slot > 0) { - dec = 1; - slot -= 1; - } - p->slots[level] = slot; - err = setup_nodes_for_search(trans, root, p, b, level, - ins_len, &write_lock_level); - if (err == -EAGAIN) - goto again; - if (err) { - ret = err; - goto done; - } - b = p->nodes[level]; - slot = p->slots[level]; - - /* - * slot 0 is special, if we change the key - * we have to update the parent pointer - * which means we must have a write lock - * on the parent - */ - if (slot == 0 && ins_len && - write_lock_level < level + 1) { - write_lock_level = level + 1; - btrfs_release_path(p); - goto again; - } - - unlock_up(p, level, lowest_unlock, - min_write_lock_level, &write_lock_level); - - if (level == lowest_level) { - if (dec) - p->slots[level]++; - goto done; - } - - err = read_block_for_search(root, p, &b, level, - slot, key); - if (err == -EAGAIN) - goto again; - if (err) { - ret = err; - goto done; - } - - if (!p->skip_locking) { - level = btrfs_header_level(b); - if (level <= write_lock_level) { - err = btrfs_try_tree_write_lock(b); - if (!err) { - btrfs_set_path_blocking(p); - btrfs_tree_lock(b); - } - p->locks[level] = BTRFS_WRITE_LOCK; - } else { - err = btrfs_tree_read_lock_atomic(b); - if (!err) { - btrfs_set_path_blocking(p); - btrfs_tree_read_lock(b); - } - p->locks[level] = BTRFS_READ_LOCK; - } - p->nodes[level] = b; - } - } else { + if (level == 0) { p->slots[level] = slot; if (ins_len > 0 && btrfs_leaf_free_space(b) < ins_len) { @@ -2916,8 +2849,8 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, } btrfs_set_path_blocking(p); - err = split_leaf(trans, root, key, - p, ins_len, ret == 0); + err = split_leaf(trans, root, key, p, ins_len, + ret == 0); BUG_ON(err > 0); if (err) { @@ -2930,6 +2863,70 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, min_write_lock_level, NULL); goto done; } + + if (ret && slot > 0) { + dec = 1; + slot -= 1; + } + p->slots[level] = slot; + err = setup_nodes_for_search(trans, root, p, b, level, ins_len, + &write_lock_level); + if (err == -EAGAIN) + goto again; + if (err) { + ret = err; + goto done; + } + b = p->nodes[level]; + slot = p->slots[level]; + + /* + * slot 0 is special, if we change the key we have to update + * the parent pointer which means we must have a write lock + * on the parent + */ + if (slot == 0 && ins_len && write_lock_level < level + 1) { + write_lock_level = level + 1; + btrfs_release_path(p); + goto again; + } + + unlock_up(p, level, lowest_unlock, min_write_lock_level, + &write_lock_level); + + if (level == lowest_level) { + if (dec) + p->slots[level]++; + goto done; + } + + err = read_block_for_search(root, p, &b, level, slot, key); + if (err == -EAGAIN) + goto again; + if (err) { + ret = err; + goto done; + } + + if (!p->skip_locking) { + level = btrfs_header_level(b); + if (level <= write_lock_level) { + err = btrfs_try_tree_write_lock(b); + if (!err) { + btrfs_set_path_blocking(p); + btrfs_tree_lock(b); + } + p->locks[level] = BTRFS_WRITE_LOCK; + } else { + err = btrfs_tree_read_lock_atomic(b); + if (!err) { + btrfs_set_path_blocking(p); + btrfs_tree_read_lock(b); + } + p->locks[level] = BTRFS_READ_LOCK; + } + p->nodes[level] = b; + } } ret = 1; done:
In btrfs_search_slot(), we something like: if (level != 0) { /* Do search inside tree nodes*/ } else { /* Do search inside tree leaves */ goto done; } This caused extra indent for tree node search code. Change it to something like: if (level == 0) { /* Do search inside tree leaves */ goto done' } /* Do search inside tree nodes */ So we have more space to maneuver our code, this is especially useful as the tree nodes search code is more complex than the leaves search code. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/ctree.c | 139 +++++++++++++++++++++++------------------------ 1 file changed, 68 insertions(+), 71 deletions(-)