[1/3] btrfs: ctree: Reduce one indent level for btrfs_search_slot()
diff mbox series

Message ID 20190910074019.23158-2-wqu@suse.com
State New
Headers show
Series
  • Small code style cleanup for ctree.c
Related show

Commit Message

Qu Wenruo Sept. 10, 2019, 7:40 a.m. UTC
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(-)

Comments

Anand Jain Sept. 10, 2019, 7:54 a.m. UTC | #1
This patch also adds line stretching until 80 chars.

looks good.

Reviewed-by: Anand Jain <anand.jain@oracle.com>
Nikolay Borisov Sept. 10, 2019, 8:24 a.m. UTC | #2
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.
Qu Wenruo Sept. 10, 2019, 8:31 a.m. UTC | #3
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
>
Nikolay Borisov Sept. 10, 2019, 8:42 a.m. UTC | #4
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
>>
>
David Sterba Sept. 23, 2019, 4:09 p.m. UTC | #5
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.

Patch
diff mbox series

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: