[PATCH 0/3] Small code style cleanup for ctree.c

[PATCH 0/3] Small code style cleanup for ctree.c

[Qu Wenruo]

Some small enhance found during the btrfs_verify_level_key() rework.

Qu Wenruo (3):
  btrfs: ctree: Reduce one indent level for btrfs_search_slot()
  btrfs: ctree: Reduce one indent level for btrfs_search_old_slot()
  btrfs: ctree: Remove stalled comment of setting up path lock

 fs/btrfs/ctree.c | 211 +++++++++++++++++++++++------------------------
 1 file changed, 101 insertions(+), 110 deletions(-)

-- 
2.23.0

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

[Qu Wenruo]

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(-)

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:
-- 
2.23.0

[PATCH 2/3] btrfs: ctree: Reduce one indent level for btrfs_search_old_slot()

[Qu Wenruo]

Pretty much the same refactor for btrfs_search_slot().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.c | 68 +++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 35 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 1e29183cdf62..3be8b32c0d37 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2983,6 +2983,7 @@ int btrfs_search_old_slot(struct btrfs_root *root, const struct btrfs_key *key,
 	p->locks[level] = BTRFS_READ_LOCK;
 
 	while (b) {
+		int dec = 0;
 		level = btrfs_header_level(b);
 		p->nodes[level] = b;
 
@@ -3003,48 +3004,45 @@ int btrfs_search_old_slot(struct btrfs_root *root, const struct btrfs_key *key,
 		if (ret < 0)
 			goto done;
 
-		if (level != 0) {
-			int dec = 0;
-			if (ret && slot > 0) {
-				dec = 1;
-				slot -= 1;
-			}
+		if (level == 0) {
 			p->slots[level] = slot;
 			unlock_up(p, level, lowest_unlock, 0, NULL);
+			goto done;
+		}
+		if (ret && slot > 0) {
+			dec = 1;
+			slot -= 1;
+		}
+		p->slots[level] = slot;
+		unlock_up(p, level, lowest_unlock, 0, NULL);
 
-			if (level == lowest_level) {
-				if (dec)
-					p->slots[level]++;
-				goto done;
-			}
+		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;
-			}
+		err = read_block_for_search(root, p, &b, level, slot, key);
+		if (err == -EAGAIN)
+			goto again;
+		if (err) {
+			ret = err;
+			goto done;
+		}
 
-			level = btrfs_header_level(b);
-			err = btrfs_tree_read_lock_atomic(b);
-			if (!err) {
-				btrfs_set_path_blocking(p);
-				btrfs_tree_read_lock(b);
-			}
-			b = tree_mod_log_rewind(fs_info, p, b, time_seq);
-			if (!b) {
-				ret = -ENOMEM;
-				goto done;
-			}
-			p->locks[level] = BTRFS_READ_LOCK;
-			p->nodes[level] = b;
-		} else {
-			p->slots[level] = slot;
-			unlock_up(p, level, lowest_unlock, 0, NULL);
+		level = btrfs_header_level(b);
+		err = btrfs_tree_read_lock_atomic(b);
+		if (!err) {
+			btrfs_set_path_blocking(p);
+			btrfs_tree_read_lock(b);
+		}
+		b = tree_mod_log_rewind(fs_info, p, b, time_seq);
+		if (!b) {
+			ret = -ENOMEM;
 			goto done;
 		}
+		p->locks[level] = BTRFS_READ_LOCK;
+		p->nodes[level] = b;
 	}
 	ret = 1;
 done:
-- 
2.23.0

[PATCH 3/3] btrfs: ctree: Remove stalled comment of setting up path lock

[Qu Wenruo]

The following comment shows up in btrfs_search_slot() with out much
sense:

	/*
	 * setup the path here so we can release it under lock
	 * contention with the cow code
	 */
	if (cow) {
		/* code touching path->lock[] is far away from here */
	}

It turns out that just some stalled comment which is not cleaned up
properly.

The original code is introduced in commit 65b51a009e29
("btrfs_search_slot: reduce lock contention by cowing in two stages"):
+
+               /*
+                * setup the path here so we can release it under lock
+                * contention with the cow code
+                */
+               p->nodes[level] = b;
+               if (!p->skip_locking)
+                       p->locks[level] = 1;
+

But in current code base, we have different timing modifying path lock,
so just remove that stalled comment.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 3be8b32c0d37..a2e264190eee 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2764,10 +2764,6 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		int dec = 0;
 		level = btrfs_header_level(b);
 
-		/*
-		 * setup the path here so we can release it under lock
-		 * contention with the cow code
-		 */
 		if (cow) {
 			bool last_level = (level == (BTRFS_MAX_LEVEL - 1));
 
-- 
2.23.0

Re: [PATCH 3/3] btrfs: ctree: Remove stalled comment of setting up path lock

[Nikolay Borisov]

On 10.09.19 г. 10:40 ч., Qu Wenruo wrote:
> The following comment shows up in btrfs_search_slot() with out much
> sense:
> 
> 	/*
> 	 * setup the path here so we can release it under lock
> 	 * contention with the cow code
> 	 */
> 	if (cow) {
> 		/* code touching path->lock[] is far away from here */
> 	}
> 
> It turns out that just some stalled comment which is not cleaned up
> properly.
> 
> The original code is introduced in commit 65b51a009e29
> ("btrfs_search_slot: reduce lock contention by cowing in two stages"):
> +
> +               /*
> +                * setup the path here so we can release it under lock
> +                * contention with the cow code
> +                */
> +               p->nodes[level] = b;
> +               if (!p->skip_locking)
> +                       p->locks[level] = 1;
> +
> 
> But in current code base, we have different timing modifying path lock,
> so just remove that stalled comment.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/ctree.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 3be8b32c0d37..a2e264190eee 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2764,10 +2764,6 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>  		int dec = 0;
>  		level = btrfs_header_level(b);
>  
> -		/*
> -		 * setup the path here so we can release it under lock
> -		 * contention with the cow code
> -		 */
>  		if (cow) {
>  			bool last_level = (level == (BTRFS_MAX_LEVEL - 1));
>  
> 

Re: [PATCH 1/3] btrfs: ctree: Reduce one indent level for btrfs_search_slot()

[Anand Jain]

This patch also adds line stretching until 80 chars.

looks good.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Re: [PATCH 2/3] btrfs: ctree: Reduce one indent level for btrfs_search_old_slot()

[Anand Jain]

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks.

Re: [PATCH 1/3] btrfs: ctree: Reduce one indent level for btrfs_search_slot()

[Nikolay Borisov]

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.

Re: [PATCH 1/3] btrfs: ctree: Reduce one indent level for btrfs_search_slot()

[Qu Wenruo]

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
>

Re: [PATCH 1/3] btrfs: ctree: Reduce one indent level for btrfs_search_slot()

[Nikolay Borisov]

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
>>
> 

Re: [PATCH 1/3] btrfs: ctree: Reduce one indent level for btrfs_search_slot()

[David Sterba]

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.

Re: [PATCH 2/3] btrfs: ctree: Reduce one indent level for btrfs_search_old_slot()

[David Sterba]

On Tue, Sep 10, 2019 at 03:40:18PM +0800, Qu Wenruo wrote:
> Pretty much the same refactor for btrfs_search_slot().

Please write something more sensible than that next time, this changelog
makes sense for a short period of time when the patches are in the
mailing list and the context is obvious, but once the patch is merged
the reference to "same thing as in some funrction" does not help.

Re: [PATCH 0/3] Small code style cleanup for ctree.c

[David Sterba]

On Tue, Sep 10, 2019 at 03:40:16PM +0800, Qu Wenruo wrote:
> Some small enhance found during the btrfs_verify_level_key() rework.
> 
> Qu Wenruo (3):
>   btrfs: ctree: Reduce one indent level for btrfs_search_slot()
>   btrfs: ctree: Reduce one indent level for btrfs_search_old_slot()
>   btrfs: ctree: Remove stalled comment of setting up path lock

Added to misc-next with some adjustments, thanks.