[PATCH v2 0/6] btrfs_search_slot cleanups

[PATCH v2 0/6] btrfs_search_slot cleanups

[Liu Bo]

Here are a collection of patches I did for btrfs_search_slot().

v2: more explicit commit log for each patch.

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 | 121 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 73 insertions(+), 48 deletions(-)

-- 
1.8.3.1

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

[Liu Bo]

read_block_for_search() can be simplified as,

tmp = find_extent_buffer();
if (tmp)
   return;

free_extent_buffer();
read_tree_block();

Apparently, @tmp must be NULL at this point, free_extent_buffer() is not
needed.

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

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

[Liu Bo]

If parent_transid "0" is passed to btrfs_buffer_uptodate(),
btrfs_buffer_uptodate() is equivalent to extent_buffer_uptodate(), but
extent_buffer_uptodate() is preferred since we don't have to look into
verify_parent_transid().

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

[PATCH v2 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.

Also invert the checks to make the code flow easier to read.

There is no functional change in this commit.

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

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index a96d308c51b8..d12fc0474e21 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2598,6 +2598,75 @@ 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;
+	/*
+	 * Callers are responsible for drop b's refs.
+	 */
+	return b;
+}
+
+
 /*
  * btrfs_search_slot - look for a key in a tree and perform necessary
  * modifications to preserve tree invariants.
@@ -2634,7 +2703,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 +2740,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

[PATCH v2 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 d12fc0474e21..8d3b09038f37 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

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

[Liu Bo]

Typically, when acquiring root node's lock, btrfs tries its best to get
read lock and trade for write lock if @write_lock_level implies to do so.

In case of (cow && (p->keep_locks || p->lowest_level)), write_lock_level
is set to BTRFS_MAX_LEVEL, which means we need to acquire root node's
write lock directly.

In this particular case, the dance of acquiring read lock and then trading
for write lock can be saved.

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 8d3b09038f37..e619f7e01794 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

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

[Liu Bo]

As unlock_up() is written as

for () {
   if (!path->locks[i])
       break;
   ...
   if (... && path->locks[i]) {
   }
}

Apparently, @path->locks[i] is always true at this 'if'.

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 e619f7e01794..65dbebb8cc59 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 v2 1/6] Btrfs: remove superfluous free_extent_buffer

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 940 bytes --]



On 2018年05月18日 11:00, Liu Bo wrote:
> read_block_for_search() can be simplified as,
> 
> tmp = find_extent_buffer();
> if (tmp)
>    return;
> 
> free_extent_buffer();
> read_tree_block();
> 
> Apparently, @tmp must be NULL at this point, free_extent_buffer() is not
> needed.>
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Reviewed-by: Qu wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>  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);
>  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1098 bytes --]



On 2018年05月18日 11:00, Liu Bo wrote:
> If parent_transid "0" is passed to btrfs_buffer_uptodate(),
> btrfs_buffer_uptodate() is equivalent to extent_buffer_uptodate(), but
> extent_buffer_uptodate() is preferred since we don't have to look into
> verify_parent_transid().
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

And considering how little extra work we do in btrfs_buffer_uptodate(),
what about make it an inline function?

Thanks,
Qu

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4679 bytes --]



On 2018年05月18日 11:00, 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.
> 
> Also invert the checks to make the code flow easier to read.
> 
> There is no functional change in this commit.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Less indents with better use of goto tag, indeed easier to read.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  fs/btrfs/ctree.c | 115 +++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 70 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index a96d308c51b8..d12fc0474e21 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2598,6 +2598,75 @@ 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;
> +	/*
> +	 * Callers are responsible for drop b's refs.
> +	 */
> +	return b;
> +}
> +
> +
>  /*
>   * btrfs_search_slot - look for a key in a tree and perform necessary
>   * modifications to preserve tree invariants.
> @@ -2634,7 +2703,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 +2740,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);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 906 bytes --]



On 2018年05月18日 11:00, Liu Bo wrote:
> 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>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Although more obvious comment about search_commit_root and skip_locking
in ctree.h will be much better.

Thanks,
Qu

> ---
>  fs/btrfs/ctree.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index d12fc0474e21..8d3b09038f37 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;
>  	}
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 985 bytes --]



On 2018年05月18日 11:00, Liu Bo wrote:
> As unlock_up() is written as
> 
> for () {
>    if (!path->locks[i])
>        break;
>    ...
>    if (... && path->locks[i]) {
>    }
> }
> 
> Apparently, @path->locks[i] is always true at this 'if'.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  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 e619f7e01794..65dbebb8cc59 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 &&
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[David Sterba]

On Fri, May 18, 2018 at 01:17:16PM +0800, Qu Wenruo wrote:
> 
> 
> On 2018年05月18日 11:00, Liu Bo wrote:
> > If parent_transid "0" is passed to btrfs_buffer_uptodate(),
> > btrfs_buffer_uptodate() is equivalent to extent_buffer_uptodate(), but
> > extent_buffer_uptodate() is preferred since we don't have to look into
> > verify_parent_transid().
> > 
> > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> And considering how little extra work we do in btrfs_buffer_uptodate(),
> what about make it an inline function?

The actual picture as the compiler sees the function can be quite
different, eg if there are static functions called from that one, they
could get inlined and suddenly btrfs_buffer_uptodate becomes
btrfs_buffer_uptodate + verify_parent_transid.

IOW, leave the inlining on the compiler and use 'static inline' only for
really lightweight functions.

Re: [PATCH v2 0/6] btrfs_search_slot cleanups

[Su Yue]

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]

Hi Liu and David,
	During my local xfstests on kdave/for-next, btrfs/139 failed and
btrfs BUG_ON due to qgroup rescan.
	The bisect result is commit 560215eb3f32("Merge branch
'ext/liubo/search-cleanups-wip' into for-next-next-v4.18-20180521")
which seems merged this patchset.
	The dmesg file is attached.

Thanks,
Su



On 05/18/2018 11:00 AM, Liu Bo wrote:
> Here are a collection of patches I did for btrfs_search_slot().
> 
> v2: more explicit commit log for each patch.
> 
> 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 | 121 +++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 73 insertions(+), 48 deletions(-)
> 



[-- Attachment #2: btrfs_139.dmesg --]
[-- Type: text/plain, Size: 7573 bytes --]

[   46.129166] BTRFS info (device vdb): disk space caching is enabled
[   46.130545] BTRFS info (device vdb): has skinny extents
[   46.171386] mount (2798) used greatest stack depth: 12920 bytes left
[   46.508170] BTRFS: device fsid 83a117c7-a9ea-4bf5-b42f-7092078610d5 devid 1 transid 5 /dev/vdc
[   46.562428] BTRFS info (device vdc): disk space caching is enabled
[   46.563690] BTRFS info (device vdc): has skinny extents
[   46.564563] BTRFS info (device vdc): flagging fs with big metadata feature
[   46.587441] BTRFS info (device vdc): checking UUID tree
[   46.766765] BTRFS info (device vdb): disk space caching is enabled
[   46.768197] BTRFS info (device vdb): has skinny extents
[   46.875534] run fstests btrfs/139 at 2018-05-22 18:40:36
[   47.559411] BTRFS: device fsid 065f3825-057e-451f-8722-0d94d4a3f36f devid 1 transid 5 /dev/vdc
[   47.612001] BTRFS info (device vdc): disk space caching is enabled
[   47.613254] BTRFS info (device vdc): has skinny extents
[   47.614147] BTRFS info (device vdc): flagging fs with big metadata feature
[   47.632377] BTRFS info (device vdc): checking UUID tree
[   47.681656] btrfs (3176) used greatest stack depth: 12632 bytes left
[   47.691156] ------------[ cut here ]------------
[   47.692084] kernel BUG at fs/btrfs/locking.c:286!
[   47.692929] invalid opcode: 0000 [#1] SMP KASAN PTI
[   47.694128] Modules linked in: btrfs xor zstd_decompress zstd_compress xxhash raid6_pq efivarfs xfs
[   47.695593] CPU: 3 PID: 1060 Comm: kworker/u8:5 Not tainted 4.17.0-rc6+ #20
[   47.696708] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
[   47.697985] Workqueue: btrfs-qgroup-rescan btrfs_qgroup_rescan_helper [btrfs]
[   47.699167] RIP: 0010:btrfs_assert_tree_read_locked+0xe4/0x100 [btrfs]
[   47.700243] RSP: 0018:ffff88014d50ed68 EFLAGS: 00010246
[   47.701094] RAX: dffffc0000000000 RBX: 1ffff10029aa1dad RCX: ffffffffa0845ac9
[   47.702254] RDX: 1ffff10029aa1db1 RSI: 0000000000000004 RDI: ffff88014fe29d18
[   47.703413] RBP: 1ffff10029aa1db1 R08: ffffed0029fc53a4 R09: ffffed0029fc53a3
[   47.704669] R10: ffffed0029fc53a3 R11: ffff88014fe29d1b R12: 0000000000000000
[   47.705905] R13: dffffc0000000000 R14: ffff880152dd10c0 R15: dffffc0000000000
[   47.708169] FS:  0000000000000000(0000) GS:ffff88015b200000(0000) knlGS:0000000000000000
[   47.709522] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   47.714771] CR2: 00005630aef46298 CR3: 0000000004216006 CR4: 00000000003606e0
[   47.717890] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   47.720258] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   47.722128] Call Trace:
[   47.723306]  ? btrfs_ioctl+0x7d20/0x7d20 [btrfs]
[   47.725323]  btrfs_set_lock_blocking_rw+0x2d1/0x420 [btrfs]
[   47.726989]  ? btrfs_assert_tree_locked+0x100/0x100 [btrfs]
[   47.728657]  ? __mutex_lock+0xdd8/0x1750
[   47.729962]  ? find_held_lock+0x3a/0x1c0
[   47.731316]  ? btrfs_qgroup_rescan_worker+0x381/0x11d0 [btrfs]
[   47.732884]  ? mutex_trylock+0x240/0x240
[   47.734257]  ? wait_for_completion+0x6a0/0x6a0
[   47.735643]  btrfs_set_path_blocking+0x79/0xe0 [btrfs]
[   47.737306]  btrfs_clear_path_blocking+0x60/0x160 [btrfs]
[   47.738823]  btrfs_search_slot+0x81e/0x23c0 [btrfs]
[   47.740298]  ? btrfs_record_root_in_trans+0xf2/0x140 [btrfs]
[   47.741835]  ? split_leaf+0x13c0/0x13c0 [btrfs]
[   47.743272]  ? btrfs_commit_transaction+0x2a00/0x2a00 [btrfs]
[   47.744821]  ? init_object+0x4e/0x80
[   47.746122]  ? btrfs_qgroup_rescan_worker+0xac/0x11d0 [btrfs]
[   47.747727]  ? __lock_is_held+0xb4/0x140
[   47.749061]  ? btrfs_qgroup_rescan_worker+0xac/0x11d0 [btrfs]
[   47.750654]  btrfs_search_slot_for_read+0x105/0x2e0 [btrfs]
[   47.752218]  btrfs_qgroup_rescan_worker+0x3bb/0x11d0 [btrfs]
[   47.754506]  ? sched_clock_cpu+0x18/0x180
[   47.755819]  ? add_qgroup_rb+0x4a0/0x4a0 [btrfs]
[   47.757335]  ? lock_acquire+0x470/0x470
[   47.758594]  ? do_raw_spin_unlock+0xa6/0x2f0
[   47.759870]  ? do_raw_spin_trylock+0x1a0/0x1a0
[   47.761184]  ? do_raw_spin_trylock+0x10/0x1a0
[   47.762425]  ? do_raw_spin_lock+0x1c0/0x1c0
[   47.763663]  normal_work_helper+0x244/0x1300 [btrfs]
[   47.764988]  ? native_sched_clock+0x7d/0x1b0
[   47.766350]  ? __btrfs_alloc_workqueue+0x770/0x770 [btrfs]
[   47.767888]  ? sched_clock_cpu+0x18/0x180
[   47.769166]  ? native_sched_clock+0x7d/0x1b0
[   47.770408]  ? cyc2ns_read_end+0x10/0x10
[   47.771552]  ? lock_acquire+0x470/0x470
[   47.772679]  ? sched_clock_cpu+0x18/0x180
[   47.773899]  ? find_held_lock+0x3a/0x1c0
[   47.775075]  ? save_trace+0x2e0/0x2e0
[   47.776194]  ? lock_acquire+0x199/0x470
[   47.777321]  ? process_one_work+0xa2f/0x1bf0
[   47.778494]  ? lock_downgrade+0x6a0/0x6a0
[   47.779632]  ? do_raw_spin_trylock+0x1a0/0x1a0
[   47.780876]  ? __lock_is_held+0xb4/0x140
[   47.781989]  process_one_work+0xad9/0x1bf0
[   47.783267]  ? _raw_spin_unlock_irq+0x29/0x40
[   47.784740]  ? finish_task_switch+0x1cf/0x800
[   47.785961]  ? cancel_delayed_work_sync+0x10/0x10
[   47.787263]  ? __switch_to_asm+0x24/0x60
[   47.788350]  ? __switch_to_asm+0x30/0x60
[   47.789424]  ? __switch_to_asm+0x24/0x60
[   47.790525]  ? __switch_to_asm+0x30/0x60
[   47.791610]  ? __switch_to_asm+0x24/0x60
[   47.792663]  ? __switch_to_asm+0x30/0x60
[   47.793732]  ? __switch_to_asm+0x24/0x60
[   47.794762]  ? __switch_to_asm+0x30/0x60
[   47.795791]  ? __switch_to_asm+0x30/0x60
[   47.796851]  ? __switch_to_asm+0x24/0x60
[   47.797845]  ? __schedule+0x6d1/0x2110
[   47.798803]  ? dmar_validate_one_drhd+0x200/0x200
[   47.799896]  ? native_sched_clock+0x7d/0x1b0
[   47.800916]  ? cyc2ns_read_end+0x10/0x10
[   47.801906]  ? do_raw_spin_unlock+0xa6/0x2f0
[   47.802933]  ? find_held_lock+0x3a/0x1c0
[   47.803978]  ? worker_thread+0x39d/0x1140
[   47.804978]  ? lock_contended+0xed0/0xed0
[   47.805972]  ? lock_downgrade+0x6a0/0x6a0
[   47.806976]  ? do_raw_spin_unlock+0xa6/0x2f0
[   47.807995]  ? do_raw_spin_trylock+0x100/0x1a0
[   47.809037]  ? do_raw_spin_lock+0x1c0/0x1c0
[   47.810055]  worker_thread+0x1a4/0x1140
[   47.810969]  ? process_one_work+0x1bf0/0x1bf0
[   47.812029]  ? dmar_validate_one_drhd+0x200/0x200
[   47.813256]  ? cyc2ns_read_end+0x10/0x10
[   47.814527]  ? ret_from_fork+0x3a/0x50
[   47.815541]  ? save_trace+0x2e0/0x2e0
[   47.816550]  ? sched_clock_local+0xe4/0x150
[   47.817570]  ? find_held_lock+0x3a/0x1c0
[   47.818550]  ? schedule+0xe5/0x380
[   47.819444]  ? lock_acquire+0x470/0x470
[   47.820404]  ? __schedule+0x2110/0x2110
[   47.821362]  ? do_raw_spin_unlock+0xa6/0x2f0
[   47.822370]  ? do_raw_spin_trylock+0x1a0/0x1a0
[   47.823421]  ? do_raw_spin_lock+0x1c0/0x1c0
[   47.824432]  ? parse_args.cold.17+0x1fa/0x1fa
[   47.825479]  ? _raw_spin_unlock_irqrestore+0x43/0x50
[   47.826763]  ? process_one_work+0x1bf0/0x1bf0
[   47.827793]  ? process_one_work+0x1bf0/0x1bf0
[   47.828822]  kthread+0x30c/0x3d0
[   47.829698]  ? kthread_create_worker_on_cpu+0xb0/0xb0
[   47.830897]  ret_from_fork+0x3a/0x50
[   47.831863] Code: df 48 c1 ed 03 c6 44 05 00 f8 45 85 e4 74 1c 48 01 c3 48 c7 03 00 00 00 00 c7 43 08 00 00 00 00 48 83 c4 60 5b 5d 41 5c 41 5d c3 <0f> 0b 48 89 ef e8 f2 33 f9 e0 eb c0 48 89 ef e8 48 33 f9 e0 eb 
[   47.836318] RIP: btrfs_assert_tree_read_locked+0xe4/0x100 [btrfs] RSP: ffff88014d50ed68
[   47.838249] ---[ end trace c6091b67dcfbaa2f ]---
[   47.839687] kworker/u8:5 (1060) used greatest stack depth: 5864 bytes left
[   61.319838] audit: type=1006 audit(1526985651.318:4): pid=3182 uid=0 old-auid=4294967295 auid=1000 tty=(none) old-ses=4294967295 ses=3 res=1

[-- Attachment #3: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 1791 bytes --]

Re: [PATCH v2 0/6] btrfs_search_slot cleanups

[David Sterba]

On Tue, May 22, 2018 at 07:05:14PM +0800, Su Yue wrote:
> Hi Liu and David,
> 	During my local xfstests on kdave/for-next, btrfs/139 failed and
> btrfs BUG_ON due to qgroup rescan.
> 	The bisect result is commit 560215eb3f32("Merge branch
> 'ext/liubo/search-cleanups-wip' into for-next-next-v4.18-20180521")
> which seems merged this patchset.
> 	The dmesg file is attached.
>
> On 05/18/2018 11:00 AM, Liu Bo wrote:
> > Here are a collection of patches I did for btrfs_search_slot().
> > 
> > v2: more explicit commit log for each patch.
> > 
> > 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 | 121 +++++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 73 insertions(+), 48 deletions(-)
> > 
> 
> 

> [   46.129166] BTRFS info (device vdb): disk space caching is enabled
> [   46.130545] BTRFS info (device vdb): has skinny extents
> [   46.171386] mount (2798) used greatest stack depth: 12920 bytes left
> [   46.508170] BTRFS: device fsid 83a117c7-a9ea-4bf5-b42f-7092078610d5 devid 1 transid 5 /dev/vdc
> [   46.562428] BTRFS info (device vdc): disk space caching is enabled
> [   46.563690] BTRFS info (device vdc): has skinny extents
> [   46.564563] BTRFS info (device vdc): flagging fs with big metadata feature
> [   46.587441] BTRFS info (device vdc): checking UUID tree
> [   46.766765] BTRFS info (device vdb): disk space caching is enabled
> [   46.768197] BTRFS info (device vdb): has skinny extents
> [   46.875534] run fstests btrfs/139 at 2018-05-22 18:40:36
> [   47.559411] BTRFS: device fsid 065f3825-057e-451f-8722-0d94d4a3f36f devid 1 transid 5 /dev/vdc
> [   47.612001] BTRFS info (device vdc): disk space caching is enabled
> [   47.613254] BTRFS info (device vdc): has skinny extents
> [   47.614147] BTRFS info (device vdc): flagging fs with big metadata feature
> [   47.632377] BTRFS info (device vdc): checking UUID tree
> [   47.681656] btrfs (3176) used greatest stack depth: 12632 bytes left
> [   47.691156] ------------[ cut here ]------------
> [   47.692084] kernel BUG at fs/btrfs/locking.c:286!

I saw the crash too but did not investigate the root cause. So I'll
remove the branch from for-next until it's fixed. Thanks for the report.

Re: [PATCH v2 0/6] btrfs_search_slot cleanups

[Nikolay Borisov]

On 22.05.2018 15:02, David Sterba wrote:
> On Tue, May 22, 2018 at 07:05:14PM +0800, Su Yue wrote:
>> Hi Liu and David,
>> 	During my local xfstests on kdave/for-next, btrfs/139 failed and
>> btrfs BUG_ON due to qgroup rescan.
>> 	The bisect result is commit 560215eb3f32("Merge branch
>> 'ext/liubo/search-cleanups-wip' into for-next-next-v4.18-20180521")
>> which seems merged this patchset.
>> 	The dmesg file is attached.
>>
>> On 05/18/2018 11:00 AM, Liu Bo wrote:
>>> Here are a collection of patches I did for btrfs_search_slot().
>>>
>>> v2: more explicit commit log for each patch.
>>>
>>> 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 | 121 +++++++++++++++++++++++++++++++++----------------------
>>>  1 file changed, 73 insertions(+), 48 deletions(-)
>>>
>>
>>
> 
>> [   46.129166] BTRFS info (device vdb): disk space caching is enabled
>> [   46.130545] BTRFS info (device vdb): has skinny extents
>> [   46.171386] mount (2798) used greatest stack depth: 12920 bytes left
>> [   46.508170] BTRFS: device fsid 83a117c7-a9ea-4bf5-b42f-7092078610d5 devid 1 transid 5 /dev/vdc
>> [   46.562428] BTRFS info (device vdc): disk space caching is enabled
>> [   46.563690] BTRFS info (device vdc): has skinny extents
>> [   46.564563] BTRFS info (device vdc): flagging fs with big metadata feature
>> [   46.587441] BTRFS info (device vdc): checking UUID tree
>> [   46.766765] BTRFS info (device vdb): disk space caching is enabled
>> [   46.768197] BTRFS info (device vdb): has skinny extents
>> [   46.875534] run fstests btrfs/139 at 2018-05-22 18:40:36
>> [   47.559411] BTRFS: device fsid 065f3825-057e-451f-8722-0d94d4a3f36f devid 1 transid 5 /dev/vdc
>> [   47.612001] BTRFS info (device vdc): disk space caching is enabled
>> [   47.613254] BTRFS info (device vdc): has skinny extents
>> [   47.614147] BTRFS info (device vdc): flagging fs with big metadata feature
>> [   47.632377] BTRFS info (device vdc): checking UUID tree
>> [   47.681656] btrfs (3176) used greatest stack depth: 12632 bytes left
>> [   47.691156] ------------[ cut here ]------------
>> [   47.692084] kernel BUG at fs/btrfs/locking.c:286!
> 
> I saw the crash too but did not investigate the root cause. So I'll
> remove the branch from for-next until it's fixed. Thanks for the report.

I think the problem stems from Qu's patch, which sets search_commit_root
=1 but doesn't set skip_locking, as a result we don't lock the tree when
we obtain a reference to the root node, yet later when traversing the
tree due to skip_locking not being set we try to lock it, and this
causes btrfs_assert_tree_locked to triggers. Can you test whether the
following diff solves the issues:


diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index bc19a7d11c98..23fadb640c59 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2702,6 +2702,7 @@ static void btrfs_qgroup_rescan_worker(struct
btrfs_work *work)
         * should be recorded by qgroup
         */
        path->search_commit_root = 1;
+       path->skip_locking = 1;

        err = 0;
        while (!err && !btrfs_fs_closing(fs_info)) {


If it does, this only means we need to make skip_locking = 1 being
conditional on search_commit_root being set and this situation should be
handled in btrfs_search_slot.

Re: [PATCH v2 0/6] btrfs_search_slot cleanups

[Su Yue]

[-- Attachment #1: Type: text/plain, Size: 4135 bytes --]



On 05/22/2018 08:35 PM, Nikolay Borisov wrote:
> 
> 
> On 22.05.2018 15:02, David Sterba wrote:
>> On Tue, May 22, 2018 at 07:05:14PM +0800, Su Yue wrote:
>>> Hi Liu and David,
>>> 	During my local xfstests on kdave/for-next, btrfs/139 failed and
>>> btrfs BUG_ON due to qgroup rescan.
>>> 	The bisect result is commit 560215eb3f32("Merge branch
>>> 'ext/liubo/search-cleanups-wip' into for-next-next-v4.18-20180521")
>>> which seems merged this patchset.
>>> 	The dmesg file is attached.
>>>
>>> On 05/18/2018 11:00 AM, Liu Bo wrote:
>>>> Here are a collection of patches I did for btrfs_search_slot().
>>>>
>>>> v2: more explicit commit log for each patch.
>>>>
>>>> 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 | 121 +++++++++++++++++++++++++++++++++----------------------
>>>>  1 file changed, 73 insertions(+), 48 deletions(-)
>>>>
>>>
>>>
>>
>>> [   46.129166] BTRFS info (device vdb): disk space caching is enabled
>>> [   46.130545] BTRFS info (device vdb): has skinny extents
>>> [   46.171386] mount (2798) used greatest stack depth: 12920 bytes left
>>> [   46.508170] BTRFS: device fsid 83a117c7-a9ea-4bf5-b42f-7092078610d5 devid 1 transid 5 /dev/vdc
>>> [   46.562428] BTRFS info (device vdc): disk space caching is enabled
>>> [   46.563690] BTRFS info (device vdc): has skinny extents
>>> [   46.564563] BTRFS info (device vdc): flagging fs with big metadata feature
>>> [   46.587441] BTRFS info (device vdc): checking UUID tree
>>> [   46.766765] BTRFS info (device vdb): disk space caching is enabled
>>> [   46.768197] BTRFS info (device vdb): has skinny extents
>>> [   46.875534] run fstests btrfs/139 at 2018-05-22 18:40:36
>>> [   47.559411] BTRFS: device fsid 065f3825-057e-451f-8722-0d94d4a3f36f devid 1 transid 5 /dev/vdc
>>> [   47.612001] BTRFS info (device vdc): disk space caching is enabled
>>> [   47.613254] BTRFS info (device vdc): has skinny extents
>>> [   47.614147] BTRFS info (device vdc): flagging fs with big metadata feature
>>> [   47.632377] BTRFS info (device vdc): checking UUID tree
>>> [   47.681656] btrfs (3176) used greatest stack depth: 12632 bytes left
>>> [   47.691156] ------------[ cut here ]------------
>>> [   47.692084] kernel BUG at fs/btrfs/locking.c:286!
>>
>> I saw the crash too but did not investigate the root cause. So I'll
>> remove the branch from for-next until it's fixed. Thanks for the report.
> 
> I think the problem stems from Qu's patch, which sets search_commit_root
> =1 but doesn't set skip_locking, as a result we don't lock the tree when
> we obtain a reference to the root node, yet later when traversing the
> tree due to skip_locking not being set we try to lock it, and this
> causes btrfs_assert_tree_locked to triggers. Can you test whether the
> following diff solves the issues:
> 
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index bc19a7d11c98..23fadb640c59 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2702,6 +2702,7 @@ static void btrfs_qgroup_rescan_worker(struct
> btrfs_work *work)
>          * should be recorded by qgroup
>          */
>         path->search_commit_root = 1;
> +       path->skip_locking = 1;
> 
>         err = 0;
>         while (!err && !btrfs_fs_closing(fs_info)) {
> 
> 
> If it does, this only means we need to make skip_locking = 1 being
> conditional on search_commit_root being set and this situation should be
> handled in btrfs_search_slot.

After patching the change, btrfs/139 passes without BUG_ON.

Thanks,
Su

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



[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 1791 bytes --]

Re: [PATCH v2 0/6] btrfs_search_slot cleanups

[David Sterba]

On Wed, May 23, 2018 at 10:16:55AM +0800, Su Yue wrote:
> >>> [   47.692084] kernel BUG at fs/btrfs/locking.c:286!
> >>
> >> I saw the crash too but did not investigate the root cause. So I'll
> >> remove the branch from for-next until it's fixed. Thanks for the report.
> > 
> > I think the problem stems from Qu's patch, which sets search_commit_root
> > =1 but doesn't set skip_locking, as a result we don't lock the tree when
> > we obtain a reference to the root node, yet later when traversing the
> > tree due to skip_locking not being set we try to lock it, and this
> > causes btrfs_assert_tree_locked to triggers. Can you test whether the
> > following diff solves the issues:
> > 
> > 
> > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> > index bc19a7d11c98..23fadb640c59 100644
> > --- a/fs/btrfs/qgroup.c
> > +++ b/fs/btrfs/qgroup.c
> > @@ -2702,6 +2702,7 @@ static void btrfs_qgroup_rescan_worker(struct
> > btrfs_work *work)
> >          * should be recorded by qgroup
> >          */
> >         path->search_commit_root = 1;
> > +       path->skip_locking = 1;
> > 
> >         err = 0;
> >         while (!err && !btrfs_fs_closing(fs_info)) {
> > 
> > 
> > If it does, this only means we need to make skip_locking = 1 being
> > conditional on search_commit_root being set and this situation should be
> > handled in btrfs_search_slot.
> 
> After patching the change, btrfs/139 passes without BUG_ON.

Confirmed, I've added the fixup to for-next, Liu Bo's branch is there to
and the crash did not show up.

Re: [PATCH v2 0/6] btrfs_search_slot cleanups

[Liu Bo]

On Wed, May 23, 2018 at 02:34:40PM +0200, David Sterba wrote:
> On Wed, May 23, 2018 at 10:16:55AM +0800, Su Yue wrote:
> > >>> [   47.692084] kernel BUG at fs/btrfs/locking.c:286!
> > >>
> > >> I saw the crash too but did not investigate the root cause. So I'll
> > >> remove the branch from for-next until it's fixed. Thanks for the report.
> > > 
> > > I think the problem stems from Qu's patch, which sets search_commit_root
> > > =1 but doesn't set skip_locking, as a result we don't lock the tree when
> > > we obtain a reference to the root node, yet later when traversing the
> > > tree due to skip_locking not being set we try to lock it, and this
> > > causes btrfs_assert_tree_locked to triggers. Can you test whether the
> > > following diff solves the issues:
> > > 
> > > 
> > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> > > index bc19a7d11c98..23fadb640c59 100644
> > > --- a/fs/btrfs/qgroup.c
> > > +++ b/fs/btrfs/qgroup.c
> > > @@ -2702,6 +2702,7 @@ static void btrfs_qgroup_rescan_worker(struct
> > > btrfs_work *work)
> > >          * should be recorded by qgroup
> > >          */
> > >         path->search_commit_root = 1;
> > > +       path->skip_locking = 1;
> > > 
> > >         err = 0;
> > >         while (!err && !btrfs_fs_closing(fs_info)) {
> > > 
> > > 
> > > If it does, this only means we need to make skip_locking = 1 being
> > > conditional on search_commit_root being set and this situation should be
> > > handled in btrfs_search_slot.
> > 
> > After patching the change, btrfs/139 passes without BUG_ON.
> 
> Confirmed, I've added the fixup to for-next, Liu Bo's branch is there to
> and the crash did not show up.

Thanks a lot for the testing and fixes!

thanks,
-liubo

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

[David Sterba]

On Fri, May 18, 2018 at 01:27:50PM +0800, Qu Wenruo wrote:
> 
> 
> On 2018年05月18日 11:00, Liu Bo wrote:
> > 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>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> Although more obvious comment about search_commit_root and skip_locking
> in ctree.h will be much better.

Not only a comment but also an ASSERT, this is too easy to get wrong.
That all currenct callers do search_commit_root + skip_locking will not
catch any future callers. And there was an example reported.

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

[David Sterba]

On Fri, May 18, 2018 at 11:00:23AM +0800, Liu Bo wrote:
> Typically, when acquiring root node's lock, btrfs tries its best to get
> read lock and trade for write lock if @write_lock_level implies to do so.
> 
> In case of (cow && (p->keep_locks || p->lowest_level)), write_lock_level
> is set to BTRFS_MAX_LEVEL, which means we need to acquire root node's
> write lock directly.
> 
> In this particular case, the dance of acquiring read lock and then trading
> for write lock can be saved.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Reviewed-by: David Sterba <dsterba@suse.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 8d3b09038f37..e619f7e01794 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;

I've added a comment why the check below is done.

> +	if (write_lock_level < BTRFS_MAX_LEVEL) {

Re: [PATCH v2 0/6] btrfs_search_slot cleanups

[David Sterba]

On Fri, May 18, 2018 at 11:00:18AM +0800, Liu Bo wrote:
> Here are a collection of patches I did for btrfs_search_slot().
> 
> v2: more explicit commit log for each patch.
> 
> 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

1,2,4,5,6 merged. Please update patch 3 and add the ASSERT. Thanks.

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

[Nikolay Borisov]

On 28.05.2018 17:21, David Sterba wrote:
> On Fri, May 18, 2018 at 01:27:50PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2018年05月18日 11:00, Liu Bo wrote:
>>> 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>
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> Although more obvious comment about search_commit_root and skip_locking
>> in ctree.h will be much better.
> 
> Not only a comment but also an ASSERT, this is too easy to get wrong.
> That all currenct callers do search_commit_root + skip_locking will not
> catch any future callers. And there was an example reported.

How about my initial suggestion of setting skip)_locking in
btrfs_search_slot if we see commit_root set? Let's try and make the life
of callers as easier as possible.
> --
> 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 v2 0/6] btrfs_search_slot cleanups

[David Sterba]

On Mon, May 28, 2018 at 04:40:28PM +0200, David Sterba wrote:
> On Fri, May 18, 2018 at 11:00:18AM +0800, Liu Bo wrote:
> > Here are a collection of patches I did for btrfs_search_slot().
> > 
> > v2: more explicit commit log for each patch.
> > 
> > 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
> 
> 1,2,4,5,6 merged. Please update patch 3 and add the ASSERT. Thanks.

Sorry, it's the 4/6 that's not merged and I'm awaiting an updated
versino. Thanks.

Re: [PATCH v2 0/6] btrfs_search_slot cleanups

[Liu Bo]

On Mon, May 28, 2018 at 06:17:18PM +0200, David Sterba wrote:
> On Mon, May 28, 2018 at 04:40:28PM +0200, David Sterba wrote:
> > On Fri, May 18, 2018 at 11:00:18AM +0800, Liu Bo wrote:
> > > Here are a collection of patches I did for btrfs_search_slot().
> > > 
> > > v2: more explicit commit log for each patch.
> > > 
> > > 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
> > 
> > 1,2,4,5,6 merged. Please update patch 3 and add the ASSERT. Thanks.
> 
> Sorry, it's the 4/6 that's not merged and I'm awaiting an updated
> versino. Thanks.

Thank you, David, I'll update it.

thanks,
-liubo

[PATCH v3] 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.

ASSERT() is put in place to ensure skip_locking is set by callers.

Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
v3: Add assertion and comments to ensure skip_locking is not forgot by callers.
v2: Rebase.

 fs/btrfs/ctree.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index a2bcfdf85726..9c4f6b919d51 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2623,8 +2623,11 @@ 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);
+		/*
+		 * Ensure that all callers have set skip_locking when
+		 * p->search_commit_root = 1.
+		 */
+		ASSERT(p->skip_locking == 1);
 
 		goto out;
 	}
-- 
1.8.3.1

Re: [PATCH v3] Btrfs: remove unused check of skip_locking

[David Sterba]

On Tue, May 29, 2018 at 09:27:06PM +0800, Liu Bo wrote:
> The check is superfluous since all of callers who set search_for_commit
> also have skip_locking set.
> 
> ASSERT() is put in place to ensure skip_locking is set by callers.
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Added to the rest, thanks.