[PATCH 0/6] btrfs_search_slot cleanups

[PATCH 0/6] btrfs_search_slot cleanups

[Liu Bo]

These're the cleanups I made for btrfs_search_slot() and its callees.

Liu Bo (6):
  Btrfs: remove superfluous free_extent_buffer
  Btrfs: use more straightforward extent_buffer_uptodate
  Btrfs: move get root of btrfs_search_slot to a helper
  Btrfs: remove unused check of skip_locking
  Btrfs: grab write lock directly if write_lock_level is the max level
  Btrfs: remove always true check in unlock_up

 fs/btrfs/ctree.c | 118 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 70 insertions(+), 48 deletions(-)

-- 
1.8.3.1

[PATCH 1/6] Btrfs: remove superfluous free_extent_buffer

[Liu Bo]

@tmp must be NULL at this point, free_extent_buffer is not needed at all.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 fs/btrfs/ctree.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index b3f6f300e492..9fa3d77c98d4 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2432,7 +2432,6 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path *path, int level)
 	btrfs_unlock_up_safe(p, level + 1);
 	btrfs_set_path_blocking(p);
 
-	free_extent_buffer(tmp);
 	if (p->reada != READA_NONE)
 		reada_for_search(fs_info, p, level, slot, key->objectid);
 
-- 
1.8.3.1

Re: [PATCH 1/6] Btrfs: remove superfluous free_extent_buffer

[Nikolay Borisov]

On 15.05.2018 20:52, Liu Bo wrote:
> @tmp must be NULL at this point, free_extent_buffer is not needed at all.

You need to explain that this code is executed only if
find_extent_buffer didn't return anything, meaning tmp is null,
otherwise it's hard to review this change without looking at the code.

Codewise it's correct:

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

> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
>  fs/btrfs/ctree.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index b3f6f300e492..9fa3d77c98d4 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2432,7 +2432,6 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path *path, int level)
>  	btrfs_unlock_up_safe(p, level + 1);
>  	btrfs_set_path_blocking(p);
>  
> -	free_extent_buffer(tmp);
>  	if (p->reada != READA_NONE)
>  		reada_for_search(fs_info, p, level, slot, key->objectid);
>  
> 

[PATCH 2/6] Btrfs: use more straightforward extent_buffer_uptodate

[Liu Bo]

In read_block_for_search(), it's straightforward to use
extent_buffer_uptodate() instead since 0 is passed as parent transid to
btrfs_buffer_uptodate(), which means the check for parent transid is not
needed.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 fs/btrfs/ctree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 9fa3d77c98d4..a96d308c51b8 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2445,7 +2445,7 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path *path, int level)
 		 * and give up so that our caller doesn't loop forever
 		 * on our EAGAINs.
 		 */
-		if (!btrfs_buffer_uptodate(tmp, 0, 0))
+		if (!extent_buffer_uptodate(tmp))
 			ret = -EIO;
 		free_extent_buffer(tmp);
 	} else {
-- 
1.8.3.1

Re: [PATCH 2/6] Btrfs: use more straightforward extent_buffer_uptodate

[Nikolay Borisov]

On 15.05.2018 20:52, Liu Bo wrote:
> In read_block_for_search(), it's straightforward to use
> extent_buffer_uptodate() instead since 0 is passed as parent transid to

"instead of the more heavyweight btrfs_buffer_update"

> btrfs_buffer_uptodate(), which means the check for parent transid is not
> needed.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Codewise LGTM:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/ctree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 9fa3d77c98d4..a96d308c51b8 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2445,7 +2445,7 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path *path, int level)
>  		 * and give up so that our caller doesn't loop forever
>  		 * on our EAGAINs.
>  		 */
> -		if (!btrfs_buffer_uptodate(tmp, 0, 0))
> +		if (!extent_buffer_uptodate(tmp))
>  			ret = -EIO;
>  		free_extent_buffer(tmp);
>  	} else {
> 

Re: [PATCH 2/6] Btrfs: use more straightforward extent_buffer_uptodate

[Liu Bo]

On Wed, May 16, 2018 at 2:43 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>
>
> On 15.05.2018 20:52, Liu Bo wrote:
>> In read_block_for_search(), it's straightforward to use
>> extent_buffer_uptodate() instead since 0 is passed as parent transid to
>
> "instead of the more heavyweight btrfs_buffer_update"
>

I don't think it's about heavyweight, they're actually equivalent in this case.

I just want to reduce the burden of reading these code as
verify_parent_transid() really has some corner cases to think about.

>> btrfs_buffer_uptodate(), which means the check for parent transid is not
>> needed.
>>
>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>
> Codewise LGTM:
>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Thanks for taking a look at this.

thanks,
liubo

>> ---
>>  fs/btrfs/ctree.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index 9fa3d77c98d4..a96d308c51b8 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -2445,7 +2445,7 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path *path, int level)
>>                * and give up so that our caller doesn't loop forever
>>                * on our EAGAINs.
>>                */
>> -             if (!btrfs_buffer_uptodate(tmp, 0, 0))
>> +             if (!extent_buffer_uptodate(tmp))
>>                       ret = -EIO;
>>               free_extent_buffer(tmp);
>>       } else {
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] Btrfs: move get root of btrfs_search_slot to a helper

[Liu Bo]

It's good to have a helper instead of having all get-root details
open-coded.

The new helper locks (if necessary) and sets root node of the path.

There is no functional change in this commit.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 fs/btrfs/ctree.c | 112 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index a96d308c51b8..399839df7a8f 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2598,6 +2598,72 @@ int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
 	return 0;
 }
 
+static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
+							struct btrfs_path *p,
+							int write_lock_level)
+{
+	struct btrfs_fs_info *fs_info = root->fs_info;
+	struct extent_buffer *b;
+	int root_lock;
+	int level = 0;
+
+	/*
+	 * we try very hard to do read locks on the root
+	 */
+	root_lock = BTRFS_READ_LOCK;
+
+	if (p->search_commit_root) {
+		/*
+		 * the commit roots are read only so we always do read locks
+		 */
+		if (p->need_commit_sem)
+			down_read(&fs_info->commit_root_sem);
+		b = root->commit_root;
+		extent_buffer_get(b);
+		level = btrfs_header_level(b);
+		if (p->need_commit_sem)
+			up_read(&fs_info->commit_root_sem);
+		if (!p->skip_locking)
+			btrfs_tree_read_lock(b);
+
+		goto out;
+	}
+
+	if (p->skip_locking) {
+		b = btrfs_root_node(root);
+		level = btrfs_header_level(b);
+		goto out;
+	}
+
+	/*
+	 * we don't know the level of the root node until we actually
+	 * have it read locked
+	 */
+	b = btrfs_read_lock_root_node(root);
+	level = btrfs_header_level(b);
+	if (level > write_lock_level)
+		goto out;
+
+	/*
+	 * whoops, must trade for write lock
+	 */
+	btrfs_tree_read_unlock(b);
+	free_extent_buffer(b);
+	b = btrfs_lock_root_node(root);
+	root_lock = BTRFS_WRITE_LOCK;
+	/*
+	 * the level might have changed, check again
+	 */
+	level = btrfs_header_level(b);
+
+out:
+	p->nodes[level] = b;
+	if (!p->skip_locking)
+		p->locks[level] = root_lock;
+	return b;
+}
+
+
 /*
  * btrfs_search_slot - look for a key in a tree and perform necessary
  * modifications to preserve tree invariants.
@@ -2634,7 +2700,6 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 	int err;
 	int level;
 	int lowest_unlock = 1;
-	int root_lock;
 	/* everything at write_lock_level or lower must be write locked */
 	int write_lock_level = 0;
 	u8 lowest_level = 0;
@@ -2672,50 +2737,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 
 again:
 	prev_cmp = -1;
-	/*
-	 * we try very hard to do read locks on the root
-	 */
-	root_lock = BTRFS_READ_LOCK;
-	level = 0;
-	if (p->search_commit_root) {
-		/*
-		 * the commit roots are read only
-		 * so we always do read locks
-		 */
-		if (p->need_commit_sem)
-			down_read(&fs_info->commit_root_sem);
-		b = root->commit_root;
-		extent_buffer_get(b);
-		level = btrfs_header_level(b);
-		if (p->need_commit_sem)
-			up_read(&fs_info->commit_root_sem);
-		if (!p->skip_locking)
-			btrfs_tree_read_lock(b);
-	} else {
-		if (p->skip_locking) {
-			b = btrfs_root_node(root);
-			level = btrfs_header_level(b);
-		} else {
-			/* we don't know the level of the root node
-			 * until we actually have it read locked
-			 */
-			b = btrfs_read_lock_root_node(root);
-			level = btrfs_header_level(b);
-			if (level <= write_lock_level) {
-				/* whoops, must trade for write lock */
-				btrfs_tree_read_unlock(b);
-				free_extent_buffer(b);
-				b = btrfs_lock_root_node(root);
-				root_lock = BTRFS_WRITE_LOCK;
-
-				/* the level might have changed, check again */
-				level = btrfs_header_level(b);
-			}
-		}
-	}
-	p->nodes[level] = b;
-	if (!p->skip_locking)
-		p->locks[level] = root_lock;
+	b = btrfs_search_slot_get_root(root, p, write_lock_level);
 
 	while (b) {
 		level = btrfs_header_level(b);
-- 
1.8.3.1

Re: [PATCH 3/6] Btrfs: move get root of btrfs_search_slot to a helper

[Nikolay Borisov]

On 15.05.2018 20:52, Liu Bo wrote:
> It's good to have a helper instead of having all get-root details
> open-coded.
> 
> The new helper locks (if necessary) and sets root node of the path.
> 
> There is no functional change in this commit.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Codewise it looks ok, you've inverted the conditional so as to give a
more linear flow of the code. I've checked all the various branches and
they seem semantically identical to the open coded version. One minor
nit is that I don't think this is the most suitable name for this
function but I don't really have a better suggestions which would be
more specific.

For the code (the changelog should be more explicit):

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

> ---
>  fs/btrfs/ctree.c | 112 +++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 67 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index a96d308c51b8..399839df7a8f 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2598,6 +2598,72 @@ int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
>  	return 0;
>  }
>  
> +static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
> +							struct btrfs_path *p,
> +							int write_lock_level)
> +{
> +	struct btrfs_fs_info *fs_info = root->fs_info;
> +	struct extent_buffer *b;
> +	int root_lock;
> +	int level = 0;
> +
> +	/*
> +	 * we try very hard to do read locks on the root
> +	 */
> +	root_lock = BTRFS_READ_LOCK;
> +
> +	if (p->search_commit_root) {
> +		/*
> +		 * the commit roots are read only so we always do read locks
> +		 */
> +		if (p->need_commit_sem)
> +			down_read(&fs_info->commit_root_sem);
> +		b = root->commit_root;
> +		extent_buffer_get(b);
> +		level = btrfs_header_level(b);
> +		if (p->need_commit_sem)
> +			up_read(&fs_info->commit_root_sem);
> +		if (!p->skip_locking)
> +			btrfs_tree_read_lock(b);
> +
> +		goto out;
> +	}
> +
> +	if (p->skip_locking) {
> +		b = btrfs_root_node(root);
> +		level = btrfs_header_level(b);
> +		goto out;
> +	}
> +
> +	/*
> +	 * we don't know the level of the root node until we actually
> +	 * have it read locked
> +	 */
> +	b = btrfs_read_lock_root_node(root);
> +	level = btrfs_header_level(b);
> +	if (level > write_lock_level)
> +		goto out;
> +
> +	/*
> +	 * whoops, must trade for write lock
> +	 */
> +	btrfs_tree_read_unlock(b);
> +	free_extent_buffer(b);
> +	b = btrfs_lock_root_node(root);
> +	root_lock = BTRFS_WRITE_LOCK;
> +	/*
> +	 * the level might have changed, check again
> +	 */
> +	level = btrfs_header_level(b);
> +
> +out:
> +	p->nodes[level] = b;
> +	if (!p->skip_locking)
> +		p->locks[level] = root_lock;
> +	return b;
> +}
> +
> +
>  /*
>   * btrfs_search_slot - look for a key in a tree and perform necessary
>   * modifications to preserve tree invariants.
> @@ -2634,7 +2700,6 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>  	int err;
>  	int level;
>  	int lowest_unlock = 1;
> -	int root_lock;
>  	/* everything at write_lock_level or lower must be write locked */
>  	int write_lock_level = 0;
>  	u8 lowest_level = 0;
> @@ -2672,50 +2737,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>  
>  again:
>  	prev_cmp = -1;
> -	/*
> -	 * we try very hard to do read locks on the root
> -	 */
> -	root_lock = BTRFS_READ_LOCK;
> -	level = 0;
> -	if (p->search_commit_root) {
> -		/*
> -		 * the commit roots are read only
> -		 * so we always do read locks
> -		 */
> -		if (p->need_commit_sem)
> -			down_read(&fs_info->commit_root_sem);
> -		b = root->commit_root;
> -		extent_buffer_get(b);
> -		level = btrfs_header_level(b);
> -		if (p->need_commit_sem)
> -			up_read(&fs_info->commit_root_sem);
> -		if (!p->skip_locking)
> -			btrfs_tree_read_lock(b);
> -	} else {
> -		if (p->skip_locking) {
> -			b = btrfs_root_node(root);
> -			level = btrfs_header_level(b);
> -		} else {
> -			/* we don't know the level of the root node
> -			 * until we actually have it read locked
> -			 */
> -			b = btrfs_read_lock_root_node(root);
> -			level = btrfs_header_level(b);
> -			if (level <= write_lock_level) {
> -				/* whoops, must trade for write lock */
> -				btrfs_tree_read_unlock(b);
> -				free_extent_buffer(b);
> -				b = btrfs_lock_root_node(root);
> -				root_lock = BTRFS_WRITE_LOCK;
> -
> -				/* the level might have changed, check again */
> -				level = btrfs_header_level(b);
> -			}
> -		}
> -	}
> -	p->nodes[level] = b;
> -	if (!p->skip_locking)
> -		p->locks[level] = root_lock;
> +	b = btrfs_search_slot_get_root(root, p, write_lock_level);
>  
>  	while (b) {
>  		level = btrfs_header_level(b);
> 

[PATCH 4/6] Btrfs: remove unused check of skip_locking

[Liu Bo]

The check is superfluous since all of callers who set search_for_commit
also have skip_locking set.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 fs/btrfs/ctree.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 399839df7a8f..cf34eca41d4e 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2623,8 +2623,6 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
 		level = btrfs_header_level(b);
 		if (p->need_commit_sem)
 			up_read(&fs_info->commit_root_sem);
-		if (!p->skip_locking)
-			btrfs_tree_read_lock(b);
 
 		goto out;
 	}
-- 
1.8.3.1

Re: [PATCH 4/6] Btrfs: remove unused check of skip_locking

[Nikolay Borisov]

On 15.05.2018 20:52, Liu Bo wrote:
> The check is superfluous since all of callers who set search_for_commit
> also have skip_locking set.

This is false. For example btrfs_qgroup_rescan_worker sets
search_commit_root = 1 but doesn't set skip locking. So either your
assumption is wrong or the code in btrfs_qgroup_rescan_worker has missed
that (which seems more likely).

I think this change necessitates either:

a) ASSERT(p->skip_locking) inside the if (p->search_commit_root) branch

or

b) Unconditionally setting ->skip_locking if we have set
search_commit_root and removing opencoded set of skip_locking alongside
search_commit_root.

I think b) is the better option since it provides "fire and forget"
semantics when you want to search the commit root, since it's only read
only.

> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
>  fs/btrfs/ctree.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 399839df7a8f..cf34eca41d4e 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2623,8 +2623,6 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>  		level = btrfs_header_level(b);
>  		if (p->need_commit_sem)
>  			up_read(&fs_info->commit_root_sem);
> -		if (!p->skip_locking)
> -			btrfs_tree_read_lock(b);
>  
>  		goto out;
>  	}
> 

Re: [PATCH 4/6] Btrfs: remove unused check of skip_locking

[Qu Wenruo]

On 2018年05月16日 15:03, Nikolay Borisov wrote:
> 
> 
> On 15.05.2018 20:52, Liu Bo wrote:
>> The check is superfluous since all of callers who set search_for_commit
>> also have skip_locking set.
> 
> This is false. For example btrfs_qgroup_rescan_worker sets
> search_commit_root = 1 but doesn't set skip locking. So either your
> assumption is wrong or the code in btrfs_qgroup_rescan_worker has missed
> that (which seems more likely).

Liu's assumption on all commit root searcher don't need lock looks good
to me, thus qgroup code could set skip_locking.

> 
> I think this change necessitates either:
> 
> a) ASSERT(p->skip_locking) inside the if (p->search_commit_root) branch
> 
> or
> 
> b) Unconditionally setting ->skip_locking if we have set
> search_commit_root and removing opencoded set of skip_locking alongside
> search_commit_root.
> 
> I think b) is the better option since it provides "fire and forget"
> semantics when you want to search the commit root, since it's only read
> only.

b) looks better to me.

Thanks,
Qu

> 
>>
>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>> ---
>>  fs/btrfs/ctree.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index 399839df7a8f..cf34eca41d4e 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -2623,8 +2623,6 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>>  		level = btrfs_header_level(b);
>>  		if (p->need_commit_sem)
>>  			up_read(&fs_info->commit_root_sem);
>> -		if (!p->skip_locking)
>> -			btrfs_tree_read_lock(b);
>>  
>>  		goto out;
>>  	}
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 4/6] Btrfs: remove unused check of skip_locking

[Liu Bo]

On Wed, May 16, 2018 at 3:03 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>
>
> On 15.05.2018 20:52, Liu Bo wrote:
>> The check is superfluous since all of callers who set search_for_commit
>> also have skip_locking set.
>
> This is false. For example btrfs_qgroup_rescan_worker sets
> search_commit_root = 1 but doesn't set skip locking. So either your
> assumption is wrong or the code in btrfs_qgroup_rescan_worker has missed
> that (which seems more likely).
>

I'm a bit confused as I didn't see btrfs_qgroup_rescan_worker() set
path->search_commit_root.

thanks,
liubo

> I think this change necessitates either:
>
> a) ASSERT(p->skip_locking) inside the if (p->search_commit_root) branch
>
> or
>
> b) Unconditionally setting ->skip_locking if we have set
> search_commit_root and removing opencoded set of skip_locking alongside
> search_commit_root.
>
> I think b) is the better option since it provides "fire and forget"
> semantics when you want to search the commit root, since it's only read
> only.
>
>>
>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>> ---
>>  fs/btrfs/ctree.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index 399839df7a8f..cf34eca41d4e 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -2623,8 +2623,6 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>>               level = btrfs_header_level(b);
>>               if (p->need_commit_sem)
>>                       up_read(&fs_info->commit_root_sem);
>> -             if (!p->skip_locking)
>> -                     btrfs_tree_read_lock(b);
>>
>>               goto out;
>>       }
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] Btrfs: remove unused check of skip_locking

[Nikolay Borisov]

On 18.05.2018 05:55, Liu Bo wrote:
> On Wed, May 16, 2018 at 3:03 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>>
>>
>> On 15.05.2018 20:52, Liu Bo wrote:
>>> The check is superfluous since all of callers who set search_for_commit
>>> also have skip_locking set.
>>
>> This is false. For example btrfs_qgroup_rescan_worker sets
>> search_commit_root = 1 but doesn't set skip locking. So either your
>> assumption is wrong or the code in btrfs_qgroup_rescan_worker has missed
>> that (which seems more likely).
>>
> 
> I'm a bit confused as I didn't see btrfs_qgroup_rescan_worker() set
> path->search_commit_root.

Then you are not basing your patches on the latest development branch
(misc-next) from David's github. The code in question is indeed very
recent:

e3884f5bd7cc ("btrfs: qgroup: Search commit root for rescan to avoid
missing extent")


> 
> thanks,
> liubo
> 
>> I think this change necessitates either:
>>
>> a) ASSERT(p->skip_locking) inside the if (p->search_commit_root) branch
>>
>> or
>>
>> b) Unconditionally setting ->skip_locking if we have set
>> search_commit_root and removing opencoded set of skip_locking alongside
>> search_commit_root.
>>
>> I think b) is the better option since it provides "fire and forget"
>> semantics when you want to search the commit root, since it's only read
>> only.
>>
>>>
>>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>>> ---
>>>  fs/btrfs/ctree.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>>> index 399839df7a8f..cf34eca41d4e 100644
>>> --- a/fs/btrfs/ctree.c
>>> +++ b/fs/btrfs/ctree.c
>>> @@ -2623,8 +2623,6 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>>>               level = btrfs_header_level(b);
>>>               if (p->need_commit_sem)
>>>                       up_read(&fs_info->commit_root_sem);
>>> -             if (!p->skip_locking)
>>> -                     btrfs_tree_read_lock(b);
>>>
>>>               goto out;
>>>       }
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 5/6] Btrfs: grab write lock directly if write_lock_level is the max level

[Liu Bo]

In case of (cow && (p->keep_locks || p->lowest_level)), write_lock_level
is the max level, and we should grab write lock of root node from the very
beginning.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 fs/btrfs/ctree.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index cf34eca41d4e..f7c3f581f647 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2633,20 +2633,23 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
 		goto out;
 	}
 
-	/*
-	 * we don't know the level of the root node until we actually
-	 * have it read locked
-	 */
-	b = btrfs_read_lock_root_node(root);
-	level = btrfs_header_level(b);
-	if (level > write_lock_level)
-		goto out;
+	if (write_lock_level < BTRFS_MAX_LEVEL) {
+		/*
+		 * we don't know the level of the root node until we actually
+		 * have it read locked
+		 */
+		b = btrfs_read_lock_root_node(root);
+		level = btrfs_header_level(b);
+		if (level > write_lock_level)
+			goto out;
+
+		/*
+		 * whoops, must trade for write lock
+		 */
+		btrfs_tree_read_unlock(b);
+		free_extent_buffer(b);
+	}
 
-	/*
-	 * whoops, must trade for write lock
-	 */
-	btrfs_tree_read_unlock(b);
-	free_extent_buffer(b);
 	b = btrfs_lock_root_node(root);
 	root_lock = BTRFS_WRITE_LOCK;
 	/*
-- 
1.8.3.1

Re: [PATCH 5/6] Btrfs: grab write lock directly if write_lock_level is the max level

[Nikolay Borisov]

On 15.05.2018 20:52, Liu Bo wrote:
> In case of (cow && (p->keep_locks || p->lowest_level)), write_lock_level
> is the max level, and we should grab write lock of root node from the very
> beginning.

THis needs to be expanded to explain what are the adverse effects (if
any) without this commit. And then explain how this commit actually
improve things.

> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
>  fs/btrfs/ctree.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index cf34eca41d4e..f7c3f581f647 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2633,20 +2633,23 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>  		goto out;
>  	}
>  
> -	/*
> -	 * we don't know the level of the root node until we actually
> -	 * have it read locked
> -	 */
> -	b = btrfs_read_lock_root_node(root);
> -	level = btrfs_header_level(b);
> -	if (level > write_lock_level)
> -		goto out;
> +	if (write_lock_level < BTRFS_MAX_LEVEL) {
> +		/*
> +		 * we don't know the level of the root node until we actually
> +		 * have it read locked
> +		 */
> +		b = btrfs_read_lock_root_node(root);
> +		level = btrfs_header_level(b);
> +		if (level > write_lock_level)
> +			goto out;
> +
> +		/*
> +		 * whoops, must trade for write lock
> +		 */
> +		btrfs_tree_read_unlock(b);
> +		free_extent_buffer(b);
> +	}

Here just seems you are adding 1 extra branch so more context please.

>  
> -	/*
> -	 * whoops, must trade for write lock
> -	 */
> -	btrfs_tree_read_unlock(b);
> -	free_extent_buffer(b);
>  	b = btrfs_lock_root_node(root);
>  	root_lock = BTRFS_WRITE_LOCK;
>  	/*
> 

[PATCH 6/6] Btrfs: remove always true check in unlock_up

[Liu Bo]

@path->lock[i] is always true at this point.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 fs/btrfs/ctree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index f7c3f581f647..16d28a4ec54f 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2330,7 +2330,7 @@ static noinline void unlock_up(struct btrfs_path *path, int level,
 			no_skips = 1;
 
 		t = path->nodes[i];
-		if (i >= lowest_unlock && i > skip_level && path->locks[i]) {
+		if (i >= lowest_unlock && i > skip_level) {
 			btrfs_tree_unlock_rw(t, path->locks[i]);
 			path->locks[i] = 0;
 			if (write_lock_level &&
-- 
1.8.3.1

Re: [PATCH 6/6] Btrfs: remove always true check in unlock_up

[Nikolay Borisov]

On 15.05.2018 20:52, Liu Bo wrote:
> @path->lock[i] is always true at this point.

You must explain why it's true. The reason is since at the beginning of
the for loop the check is performed :

   if (!path->locks[i])
      break;


Codewise it's ok:

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

> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
>  fs/btrfs/ctree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index f7c3f581f647..16d28a4ec54f 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2330,7 +2330,7 @@ static noinline void unlock_up(struct btrfs_path *path, int level,
>  			no_skips = 1;
>  
>  		t = path->nodes[i];
> -		if (i >= lowest_unlock && i > skip_level && path->locks[i]) {
> +		if (i >= lowest_unlock && i > skip_level) {
>  			btrfs_tree_unlock_rw(t, path->locks[i]);
>  			path->locks[i] = 0;
>  			if (write_lock_level &&
> 

Re: [PATCH 0/6] btrfs_search_slot cleanups

[David Sterba]

On Wed, May 16, 2018 at 01:52:02AM +0800, Liu Bo wrote:
> These're the cleanups I made for btrfs_search_slot() and its callees.

All the patches have very terse changelog and I have no idea why you
think the code change is correct so I can compare my review against
that.

It's touching code around locking that's never bad to be more
verbose and introduce a bit of the context, the simple sentence is maybe
clear to you when you go through the code, but imagine you read the
changelog in a week or month or a year. It still needs to make sense or
give enough pointers to find out from the code.

Please update and resend, thanks.