[PATCH 0/4] Misc cleanups

[PATCH 0/4] Misc cleanups

[Nikolay Borisov]

Here are a couples of cleanups of things I observed while looking at the 
extent_buffer management code. 

Patch 1 rewrites a do {} while into a simple for() construct. This survived 
xfstest + selftests 

Patch 2 substitutes and outdated comment for a lockdep_assert_held call

Patch 3 rename the idiotic EXTENT_BUFFER_DUMMY to something more meaningful

Patch 4 removes some cargo-cult copied code when performing qgroup leaf scan 

Nikolay Borisov (4):
  btrfs: Refactor loop in btrfs_release_extent_buffer_page
  btrfs: Document locking require via lockdep_assert_held
  btrfs: Rename EXTENT_BUFFER_DUMMY to EXTENT_BUFFER_PRIVATE
  btrfs: Remove unnecessary locking code in qgroup_rescan_leaf

 fs/btrfs/disk-io.c   |  2 +-
 fs/btrfs/extent_io.c | 26 +++++++++++---------------
 fs/btrfs/extent_io.h |  2 +-
 fs/btrfs/qgroup.c    |  7 +------
 4 files changed, 14 insertions(+), 23 deletions(-)

-- 
2.7.4

[PATCH 1/4] btrfs: Refactor loop in btrfs_release_extent_buffer_page

[Nikolay Borisov]

The purpose of the function is to free all the pages comprising an
extent buffer. This can be achieved with a simple for loop rather than
the slitghly more involved 'do {} while' construct. So rewrite the
loop using a 'for' construct. Additionally we can never have an
extent_buffer that is 0 pages so remove the check for index == 0. No
functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent_io.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index cce6087d6880..4180a3b7e725 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4641,19 +4641,14 @@ int extent_buffer_under_io(struct extent_buffer *eb)
  */
 static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
 {
-	unsigned long index;
-	struct page *page;
+	int i;
 	int mapped = !test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
 
 	BUG_ON(extent_buffer_under_io(eb));
 
-	index = num_extent_pages(eb->start, eb->len);
-	if (index == 0)
-		return;
+	for (i = 0; i < num_extent_pages(eb->start, eb->len); i++) {
+		struct page *page = eb->pages[i];
 
-	do {
-		index--;
-		page = eb->pages[index];
 		if (!page)
 			continue;
 		if (mapped)
@@ -4685,7 +4680,7 @@ static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
 
 		/* One for when we allocated the page */
 		put_page(page);
-	} while (index != 0);
+	}
 }
 
 /*
-- 
2.7.4

[PATCH 2/4] btrfs: Document locking require via lockdep_assert_held

[Nikolay Borisov]

Remove stale comment since there is no longer an eb->eb_lock and
document the locking expectation with a lockdep_assert_held statement.
No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent_io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 4180a3b7e725..6ac15804bab1 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5064,9 +5064,10 @@ static inline void btrfs_release_extent_buffer_rcu(struct rcu_head *head)
 	__free_extent_buffer(eb);
 }
 
-/* Expects to have eb->eb_lock already held */
 static int release_extent_buffer(struct extent_buffer *eb)
 {
+	lockdep_assert_held(&eb->refs_lock);
+
 	WARN_ON(atomic_read(&eb->refs) == 0);
 	if (atomic_dec_and_test(&eb->refs)) {
 		if (test_and_clear_bit(EXTENT_BUFFER_IN_TREE, &eb->bflags)) {
-- 
2.7.4

[PATCH 3/4] btrfs: Rename EXTENT_BUFFER_DUMMY to EXTENT_BUFFER_PRIVATE

[Nikolay Borisov]

EXTENT_BUFFER_DUMMY is an awful name for this flag. Buffers which have
this flag set are not in any way dummy. Rather, they are private in
the sense that are not linked to the global buffer tree. This flag has
subtle implications to the way free_extent_buffer work for example, as
well as controls whether page->mapping->private_lock is held during
extent_buffer release. Pages for a private buffer cannot be under io,
nor can they be written by a 3rd party so taking the lock is
unnecessary.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c   |  2 +-
 fs/btrfs/extent_io.c | 10 +++++-----
 fs/btrfs/extent_io.h |  2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 8a469f70d5ee..1c655be92690 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4093,7 +4093,7 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
 	 * enabled.  Normal people shouldn't be marking dummy buffers as dirty
 	 * outside of the sanity tests.
 	 */
-	if (unlikely(test_bit(EXTENT_BUFFER_DUMMY, &buf->bflags)))
+	if (unlikely(test_bit(EXTENT_BUFFER_PRIVATE, &buf->bflags)))
 		return;
 #endif
 	root = BTRFS_I(buf->pages[0]->mapping->host)->root;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 6ac15804bab1..6611207e8e1f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4642,7 +4642,7 @@ int extent_buffer_under_io(struct extent_buffer *eb)
 static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
 {
 	int i;
-	int mapped = !test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
+	int mapped = !test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags);
 
 	BUG_ON(extent_buffer_under_io(eb));
 
@@ -4755,7 +4755,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
 	}
 
 	set_bit(EXTENT_BUFFER_UPTODATE, &new->bflags);
-	set_bit(EXTENT_BUFFER_DUMMY, &new->bflags);
+	set_bit(EXTENT_BUFFER_PRIVATE, &new->bflags);
 
 	return new;
 }
@@ -4780,7 +4780,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
 	}
 	set_extent_buffer_uptodate(eb);
 	btrfs_set_header_nritems(eb, 0);
-	set_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
+	set_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags);
 
 	return eb;
 err:
@@ -5086,7 +5086,7 @@ static int release_extent_buffer(struct extent_buffer *eb)
 		/* Should be safe to release our pages at this point */
 		btrfs_release_extent_buffer_page(eb);
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-		if (unlikely(test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags))) {
+		if (unlikely(test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags))) {
 			__free_extent_buffer(eb);
 			return 1;
 		}
@@ -5117,7 +5117,7 @@ void free_extent_buffer(struct extent_buffer *eb)
 
 	spin_lock(&eb->refs_lock);
 	if (atomic_read(&eb->refs) == 2 &&
-	    test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags))
+	    test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags))
 		atomic_dec(&eb->refs);
 
 	if (atomic_read(&eb->refs) == 2 &&
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 0bfd4aeb822d..bfccaec2c89a 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -46,7 +46,7 @@
 #define EXTENT_BUFFER_STALE 6
 #define EXTENT_BUFFER_WRITEBACK 7
 #define EXTENT_BUFFER_READ_ERR 8        /* read IO error */
-#define EXTENT_BUFFER_DUMMY 9
+#define EXTENT_BUFFER_PRIVATE 9
 #define EXTENT_BUFFER_IN_TREE 10
 #define EXTENT_BUFFER_WRITE_ERR 11    /* write IO error */
 
-- 
2.7.4

[PATCH 4/4] btrfs: Remove unnecessary locking code in qgroup_rescan_leaf

[Nikolay Borisov]

In qgroup_rescan_leaf a copy is made of the target leaf by calling
btrfs_clone_extent_buffer. The latter allocates a new buffer and
attaches a new set of pages and copies the content of the source
buffer. The new scratch buffer is only used to iterate it's items, it's
not published anywhere and cannot be accessed by a third party. Hence,
it's not necessary to perform any locking on it whatsoever. Furthermore,
remove the extra extent_buffer_get call since the new buffer is always
allocated with a reference count of 1 which is sufficient here.
No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/qgroup.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 1874a6d2e6f5..3b57dc247fa2 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2647,9 +2647,6 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 		mutex_unlock(&fs_info->qgroup_rescan_lock);
 		goto out;
 	}
-	extent_buffer_get(scratch_leaf);
-	btrfs_tree_read_lock(scratch_leaf);
-	btrfs_set_lock_blocking_rw(scratch_leaf, BTRFS_READ_LOCK);
 	slot = path->slots[0];
 	btrfs_release_path(path);
 	mutex_unlock(&fs_info->qgroup_rescan_lock);
@@ -2675,10 +2672,8 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 			goto out;
 	}
 out:
-	if (scratch_leaf) {
-		btrfs_tree_read_unlock_blocking(scratch_leaf);
+	if (scratch_leaf)
 		free_extent_buffer(scratch_leaf);
-	}
 
 	if (done && !ret)
 		ret = 1;
-- 
2.7.4

Re: [PATCH 1/4] btrfs: Refactor loop in btrfs_release_extent_buffer_page

[David Sterba]

On Wed, Jun 27, 2018 at 04:38:22PM +0300, Nikolay Borisov wrote:
> The purpose of the function is to free all the pages comprising an
> extent buffer. This can be achieved with a simple for loop rather than
> the slitghly more involved 'do {} while' construct. So rewrite the
> loop using a 'for' construct. Additionally we can never have an
> extent_buffer that is 0 pages so remove the check for index == 0. No
> functional changes.

The reversed loop makes sense, the first page is special and used for
locking the whole extent buffer's pages, as can be seen eg.
897ca6e9b4fef86d5dfb6b31 or 4f2de97acee6532b36dd6e99 . Also see current
alloc_extent_buffer for the locking and managing the private bit.

So you can still rewrite it as a for loop but would have to preserve the
logic or provide the reason that it's correct to iterate from 0 to
num_pages. There are some subltle races regarding pages and extents and
their presence in various trees, so I'd rather be careful here.

> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/extent_io.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index cce6087d6880..4180a3b7e725 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4641,19 +4641,14 @@ int extent_buffer_under_io(struct extent_buffer *eb)
>   */
>  static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
>  {
> -	unsigned long index;
> -	struct page *page;
> +	int i;
>  	int mapped = !test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
>  
>  	BUG_ON(extent_buffer_under_io(eb));
>  
> -	index = num_extent_pages(eb->start, eb->len);
> -	if (index == 0)
> -		return;

This check does seem to be redundant.

> +	for (i = 0; i < num_extent_pages(eb->start, eb->len); i++) {

I think that the num_extent_pages(...) would be better put to a
temporary variable so it's not evaluated each time the loop termination
condition is checked.

> +		struct page *page = eb->pages[i];
>  
> -	do {
> -		index--;
> -		page = eb->pages[index];
>  		if (!page)
>  			continue;
>  		if (mapped)

Re: [PATCH 3/4] btrfs: Rename EXTENT_BUFFER_DUMMY to EXTENT_BUFFER_PRIVATE

[David Sterba]

On Wed, Jun 27, 2018 at 04:38:24PM +0300, Nikolay Borisov wrote:
> EXTENT_BUFFER_DUMMY is an awful name for this flag. Buffers which have
> this flag set are not in any way dummy. Rather, they are private in
> the sense that are not linked to the global buffer tree. This flag has
> subtle implications to the way free_extent_buffer work for example, as
> well as controls whether page->mapping->private_lock is held during
> extent_buffer release. Pages for a private buffer cannot be under io,
> nor can they be written by a 3rd party so taking the lock is
> unnecessary.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/disk-io.c   |  2 +-
>  fs/btrfs/extent_io.c | 10 +++++-----
>  fs/btrfs/extent_io.h |  2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 8a469f70d5ee..1c655be92690 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4093,7 +4093,7 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
>  	 * enabled.  Normal people shouldn't be marking dummy buffers as dirty
>  	 * outside of the sanity tests.
>  	 */
> -	if (unlikely(test_bit(EXTENT_BUFFER_DUMMY, &buf->bflags)))
> +	if (unlikely(test_bit(EXTENT_BUFFER_PRIVATE, &buf->bflags)))

This is going to be confusing. There's page Private bit,
PAGE_SET_PRIVATE2 and EXTENT_PAGE_PRIVATE, that are somehow logically
connected.

I'd suggest EXTENT_BUFFER_CLONED or _UNMAPPED as it's created by
btrfs_clone_extent_buffer or used in the disconnected way (ie. without
the mapping).

>  		return;
>  #endif
>  	root = BTRFS_I(buf->pages[0]->mapping->host)->root;
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 6ac15804bab1..6611207e8e1f 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4642,7 +4642,7 @@ int extent_buffer_under_io(struct extent_buffer *eb)
>  static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
>  {
>  	int i;
> -	int mapped = !test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
> +	int mapped = !test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags);
>  
>  	BUG_ON(extent_buffer_under_io(eb));
>  
> @@ -4755,7 +4755,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
>  	}
>  
>  	set_bit(EXTENT_BUFFER_UPTODATE, &new->bflags);
> -	set_bit(EXTENT_BUFFER_DUMMY, &new->bflags);
> +	set_bit(EXTENT_BUFFER_PRIVATE, &new->bflags);
>  
>  	return new;
>  }
> @@ -4780,7 +4780,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,

Would be good to sync the new name with the helpers:
__alloc_dummy_extent_buffer and alloc_dummy_extent_buffer then.

>  	}
>  	set_extent_buffer_uptodate(eb);
>  	btrfs_set_header_nritems(eb, 0);
> -	set_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
> +	set_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags);
>  
>  	return eb;
>  err:
> @@ -5086,7 +5086,7 @@ static int release_extent_buffer(struct extent_buffer *eb)
>  		/* Should be safe to release our pages at this point */
>  		btrfs_release_extent_buffer_page(eb);
>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> -		if (unlikely(test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags))) {
> +		if (unlikely(test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags))) {
>  			__free_extent_buffer(eb);
>  			return 1;
>  		}
> @@ -5117,7 +5117,7 @@ void free_extent_buffer(struct extent_buffer *eb)
>  
>  	spin_lock(&eb->refs_lock);
>  	if (atomic_read(&eb->refs) == 2 &&
> -	    test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags))
> +	    test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags))
>  		atomic_dec(&eb->refs);
>  
>  	if (atomic_read(&eb->refs) == 2 &&
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 0bfd4aeb822d..bfccaec2c89a 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -46,7 +46,7 @@
>  #define EXTENT_BUFFER_STALE 6
>  #define EXTENT_BUFFER_WRITEBACK 7
>  #define EXTENT_BUFFER_READ_ERR 8        /* read IO error */
> -#define EXTENT_BUFFER_DUMMY 9
> +#define EXTENT_BUFFER_PRIVATE 9
>  #define EXTENT_BUFFER_IN_TREE 10
>  #define EXTENT_BUFFER_WRITE_ERR 11    /* write IO error */
>  
> -- 
> 2.7.4
> 
> --
> 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 3/4] btrfs: Rename EXTENT_BUFFER_DUMMY to EXTENT_BUFFER_PRIVATE

[Qu Wenruo]

On 2018年06月29日 20:46, David Sterba wrote:
> On Wed, Jun 27, 2018 at 04:38:24PM +0300, Nikolay Borisov wrote:
>> EXTENT_BUFFER_DUMMY is an awful name for this flag. Buffers which have
>> this flag set are not in any way dummy. Rather, they are private in
>> the sense that are not linked to the global buffer tree. This flag has
>> subtle implications to the way free_extent_buffer work for example, as
>> well as controls whether page->mapping->private_lock is held during
>> extent_buffer release. Pages for a private buffer cannot be under io,
>> nor can they be written by a 3rd party so taking the lock is
>> unnecessary.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/disk-io.c   |  2 +-
>>  fs/btrfs/extent_io.c | 10 +++++-----
>>  fs/btrfs/extent_io.h |  2 +-
>>  3 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 8a469f70d5ee..1c655be92690 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -4093,7 +4093,7 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
>>  	 * enabled.  Normal people shouldn't be marking dummy buffers as dirty
>>  	 * outside of the sanity tests.
>>  	 */
>> -	if (unlikely(test_bit(EXTENT_BUFFER_DUMMY, &buf->bflags)))
>> +	if (unlikely(test_bit(EXTENT_BUFFER_PRIVATE, &buf->bflags)))
> 
> This is going to be confusing. There's page Private bit,
> PAGE_SET_PRIVATE2 and EXTENT_PAGE_PRIVATE, that are somehow logically
> connected.
> 
> I'd suggest EXTENT_BUFFER_CLONED or _UNMAPPED as it's created by
> btrfs_clone_extent_buffer or used in the disconnected way (ie. without
> the mapping).

UNMAPPED looks good to me.
(Or ANONYMOUS?)

> 
>>  		return;
>>  #endif
>>  	root = BTRFS_I(buf->pages[0]->mapping->host)->root;
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 6ac15804bab1..6611207e8e1f 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -4642,7 +4642,7 @@ int extent_buffer_under_io(struct extent_buffer *eb)
>>  static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
>>  {
>>  	int i;
>> -	int mapped = !test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
>> +	int mapped = !test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags);
>>  
>>  	BUG_ON(extent_buffer_under_io(eb));
>>  
>> @@ -4755,7 +4755,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
>>  	}
>>  
>>  	set_bit(EXTENT_BUFFER_UPTODATE, &new->bflags);
>> -	set_bit(EXTENT_BUFFER_DUMMY, &new->bflags);
>> +	set_bit(EXTENT_BUFFER_PRIVATE, &new->bflags);
>>  
>>  	return new;
>>  }
>> @@ -4780,7 +4780,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
> 
> Would be good to sync the new name with the helpers:
> __alloc_dummy_extent_buffer and alloc_dummy_extent_buffer then.
> 
>>  	}
>>  	set_extent_buffer_uptodate(eb);
>>  	btrfs_set_header_nritems(eb, 0);
>> -	set_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
>> +	set_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags);
>>  
>>  	return eb;
>>  err:
>> @@ -5086,7 +5086,7 @@ static int release_extent_buffer(struct extent_buffer *eb)
>>  		/* Should be safe to release our pages at this point */
>>  		btrfs_release_extent_buffer_page(eb);
>>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>> -		if (unlikely(test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags))) {
>> +		if (unlikely(test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags))) {
>>  			__free_extent_buffer(eb);
>>  			return 1;
>>  		}
>> @@ -5117,7 +5117,7 @@ void free_extent_buffer(struct extent_buffer *eb)
>>  
>>  	spin_lock(&eb->refs_lock);
>>  	if (atomic_read(&eb->refs) == 2 &&
>> -	    test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags))
>> +	    test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags))
>>  		atomic_dec(&eb->refs);

Also discussed in off list mail, this extra atomic_dec for cloned eb
looks confusing.
(That also explains why after cloning the eb, we call
extent_buffer_get() and only frees it once, and still no eb leaking)
What about just removing such special handling?

Thanks,
Qu

>>  
>>  	if (atomic_read(&eb->refs) == 2 &&
>> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
>> index 0bfd4aeb822d..bfccaec2c89a 100644
>> --- a/fs/btrfs/extent_io.h
>> +++ b/fs/btrfs/extent_io.h
>> @@ -46,7 +46,7 @@
>>  #define EXTENT_BUFFER_STALE 6
>>  #define EXTENT_BUFFER_WRITEBACK 7
>>  #define EXTENT_BUFFER_READ_ERR 8        /* read IO error */
>> -#define EXTENT_BUFFER_DUMMY 9
>> +#define EXTENT_BUFFER_PRIVATE 9
>>  #define EXTENT_BUFFER_IN_TREE 10
>>  #define EXTENT_BUFFER_WRITE_ERR 11    /* write IO error */
>>  
>> -- 
>> 2.7.4
>>
>> --
>> 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
> --
> 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 3/4] btrfs: Rename EXTENT_BUFFER_DUMMY to EXTENT_BUFFER_PRIVATE

[Nikolay Borisov]

On 29.06.2018 16:07, Qu Wenruo wrote:
> 
> 
> On 2018年06月29日 20:46, David Sterba wrote:
>> On Wed, Jun 27, 2018 at 04:38:24PM +0300, Nikolay Borisov wrote:
>>> EXTENT_BUFFER_DUMMY is an awful name for this flag. Buffers which have
>>> this flag set are not in any way dummy. Rather, they are private in
>>> the sense that are not linked to the global buffer tree. This flag has
>>> subtle implications to the way free_extent_buffer work for example, as
>>> well as controls whether page->mapping->private_lock is held during
>>> extent_buffer release. Pages for a private buffer cannot be under io,
>>> nor can they be written by a 3rd party so taking the lock is
>>> unnecessary.
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>>  fs/btrfs/disk-io.c   |  2 +-
>>>  fs/btrfs/extent_io.c | 10 +++++-----
>>>  fs/btrfs/extent_io.h |  2 +-
>>>  3 files changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index 8a469f70d5ee..1c655be92690 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -4093,7 +4093,7 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
>>>  	 * enabled.  Normal people shouldn't be marking dummy buffers as dirty
>>>  	 * outside of the sanity tests.
>>>  	 */
>>> -	if (unlikely(test_bit(EXTENT_BUFFER_DUMMY, &buf->bflags)))
>>> +	if (unlikely(test_bit(EXTENT_BUFFER_PRIVATE, &buf->bflags)))
>>
>> This is going to be confusing. There's page Private bit,
>> PAGE_SET_PRIVATE2 and EXTENT_PAGE_PRIVATE, that are somehow logically
>> connected.
>>
>> I'd suggest EXTENT_BUFFER_CLONED or _UNMAPPED as it's created by
>> btrfs_clone_extent_buffer or used in the disconnected way (ie. without
>> the mapping).
> 
> UNMAPPED looks good to me.
> (Or ANONYMOUS?)

I'm more leaning towards UNMAPPED at this point.

> 
>>
>>>  		return;
>>>  #endif
>>>  	root = BTRFS_I(buf->pages[0]->mapping->host)->root;
>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>> index 6ac15804bab1..6611207e8e1f 100644
>>> --- a/fs/btrfs/extent_io.c
>>> +++ b/fs/btrfs/extent_io.c
>>> @@ -4642,7 +4642,7 @@ int extent_buffer_under_io(struct extent_buffer *eb)
>>>  static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
>>>  {
>>>  	int i;
>>> -	int mapped = !test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
>>> +	int mapped = !test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags);
>>>  
>>>  	BUG_ON(extent_buffer_under_io(eb));
>>>  
>>> @@ -4755,7 +4755,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
>>>  	}
>>>  
>>>  	set_bit(EXTENT_BUFFER_UPTODATE, &new->bflags);
>>> -	set_bit(EXTENT_BUFFER_DUMMY, &new->bflags);
>>> +	set_bit(EXTENT_BUFFER_PRIVATE, &new->bflags);
>>>  
>>>  	return new;
>>>  }
>>> @@ -4780,7 +4780,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
>>
>> Would be good to sync the new name with the helpers:
>> __alloc_dummy_extent_buffer and alloc_dummy_extent_buffer then.
>>
>>>  	}
>>>  	set_extent_buffer_uptodate(eb);
>>>  	btrfs_set_header_nritems(eb, 0);
>>> -	set_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
>>> +	set_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags);
>>>  
>>>  	return eb;
>>>  err:
>>> @@ -5086,7 +5086,7 @@ static int release_extent_buffer(struct extent_buffer *eb)
>>>  		/* Should be safe to release our pages at this point */
>>>  		btrfs_release_extent_buffer_page(eb);
>>>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>>> -		if (unlikely(test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags))) {
>>> +		if (unlikely(test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags))) {
>>>  			__free_extent_buffer(eb);
>>>  			return 1;
>>>  		}
>>> @@ -5117,7 +5117,7 @@ void free_extent_buffer(struct extent_buffer *eb)
>>>  
>>>  	spin_lock(&eb->refs_lock);
>>>  	if (atomic_read(&eb->refs) == 2 &&
>>> -	    test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags))
>>> +	    test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags))
>>>  		atomic_dec(&eb->refs);
> 
> Also discussed in off list mail, this extra atomic_dec for cloned eb
> looks confusing.
> (That also explains why after cloning the eb, we call
> extent_buffer_get() and only frees it once, and still no eb leaking)
> What about just removing such special handling?

I think this special handling is not needed if we consider the fact that
allocating a new eb already has ref1. In this case the code that
allocated the buffer really "hands over" the reference when putting it
on a btrfs_path struct.

Let's take btrfs_search_old_slot as an example. It calls
tree_mod_log_rewind which rewinds the passed in extent buffer by cloning
it and doing an extra extent_buffer_get and "publishing" it to the given
btrfs_path struct. But really this buffer should have only a single
reference since it's only in the btrfs_path. Then this buffer should be
released when btrfs_release_path is called on the path.

Again, in btrfs_search_old_slot we have the same usage scenario in
get_old_root.

> 
> Thanks,
> Qu
> 
>>>  
>>>  	if (atomic_read(&eb->refs) == 2 &&
>>> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
>>> index 0bfd4aeb822d..bfccaec2c89a 100644
>>> --- a/fs/btrfs/extent_io.h
>>> +++ b/fs/btrfs/extent_io.h
>>> @@ -46,7 +46,7 @@
>>>  #define EXTENT_BUFFER_STALE 6
>>>  #define EXTENT_BUFFER_WRITEBACK 7
>>>  #define EXTENT_BUFFER_READ_ERR 8        /* read IO error */
>>> -#define EXTENT_BUFFER_DUMMY 9
>>> +#define EXTENT_BUFFER_PRIVATE 9
>>>  #define EXTENT_BUFFER_IN_TREE 10
>>>  #define EXTENT_BUFFER_WRITE_ERR 11    /* write IO error */
>>>  
>>> -- 
>>> 2.7.4
>>>
>>> --
>>> 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
>> --
>> 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
>>
> --
> 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 3/4] btrfs: Rename EXTENT_BUFFER_DUMMY to EXTENT_BUFFER_PRIVATE

[David Sterba]

On Fri, Jun 29, 2018 at 09:07:12PM +0800, Qu Wenruo wrote:
> >> @@ -5117,7 +5117,7 @@ void free_extent_buffer(struct extent_buffer *eb)
> >>  
> >>  	spin_lock(&eb->refs_lock);
> >>  	if (atomic_read(&eb->refs) == 2 &&
> >> -	    test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags))
> >> +	    test_bit(EXTENT_BUFFER_PRIVATE, &eb->bflags))
> >>  		atomic_dec(&eb->refs);
> 
> Also discussed in off list mail, this extra atomic_dec for cloned eb
> looks confusing.
> (That also explains why after cloning the eb, we call
> extent_buffer_get() and only frees it once, and still no eb leaking)
> What about just removing such special handling?

The extent_buffer_get can be moved to the cloning function, all callers
increase the reference but the missing dec is indeed confusing. However
I don't think we can remove it completely. We need to keep the eb::refs
at the same level for all eb types (regular, STALE, DUMMY), so we can
use eg. free_extent_buffer.

There may be other way how to clean it that I don't see now, so if you
think you can improve the reference handling, then go for it.

Re: [PATCH 1/4] btrfs: Refactor loop in btrfs_release_extent_buffer_page

[Nikolay Borisov]

On 29.06.2018 15:35, David Sterba wrote:
> On Wed, Jun 27, 2018 at 04:38:22PM +0300, Nikolay Borisov wrote:
>> The purpose of the function is to free all the pages comprising an
>> extent buffer. This can be achieved with a simple for loop rather than
>> the slitghly more involved 'do {} while' construct. So rewrite the
>> loop using a 'for' construct. Additionally we can never have an
>> extent_buffer that is 0 pages so remove the check for index == 0. No
>> functional changes.
> 
> The reversed loop makes sense, the first page is special and used for
> locking the whole extent buffer's pages, as can be seen eg.
> 897ca6e9b4fef86d5dfb6b31 or 4f2de97acee6532b36dd6e99 . Also see current
> alloc_extent_buffer for the locking and managing the private bit.

Turns out  page[0] is not special and it's not used for any special
locking whatsoever. In csum_dirty_buffer this page is used so that when
we submit a multipage eb then we only perform csum calculation once, i.e
when the first page (page[0]) is submitted.
btrfs_release_extent_buffer_page OTOH no longer takes an index but just
an EB and iterates all the pages.

The fact that page0 is unlocked last in alloc_extent_buffer seems to be
an artefact of the code refactoring rather than a deliberate behavior.
Furthermore I've been running tests with sequential unlocking (and
complete removal of the SetPageChecked/ClearPageChecked function from
alloc_extent_buffer) of the pages and didn't observe any problems
whatsoever on both 4g and 1g configuration ( the latter is more likely
to trigger premature page eviction hence trigger any lurking races
between alloc_extent_buffer and btree_releasepage. I will be sending a
patch after I do a bit more testing but so far the results are good.

> 
> So you can still rewrite it as a for loop but would have to preserve the
> logic or provide the reason that it's correct to iterate from 0 to
> num_pages. There are some subltle races regarding pages and extents and
> their presence in various trees, so I'd rather be careful here.
> 
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/extent_io.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index cce6087d6880..4180a3b7e725 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -4641,19 +4641,14 @@ int extent_buffer_under_io(struct extent_buffer *eb)
>>   */
>>  static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
>>  {
>> -	unsigned long index;
>> -	struct page *page;
>> +	int i;
>>  	int mapped = !test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
>>  
>>  	BUG_ON(extent_buffer_under_io(eb));
>>  
>> -	index = num_extent_pages(eb->start, eb->len);
>> -	if (index == 0)
>> -		return;
> 
> This check does seem to be redundant.
> 
>> +	for (i = 0; i < num_extent_pages(eb->start, eb->len); i++) {
> 
> I think that the num_extent_pages(...) would be better put to a
> temporary variable so it's not evaluated each time the loop termination
> condition is checked.
> 
>> +		struct page *page = eb->pages[i];
>>  
>> -	do {
>> -		index--;
>> -		page = eb->pages[index];
>>  		if (!page)
>>  			continue;
>>  		if (mapped)
> --
> 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/4] btrfs: Remove unnecessary locking code in qgroup_rescan_leaf

[Nikolay Borisov]

On 27.06.2018 16:38, Nikolay Borisov wrote:
> In qgroup_rescan_leaf a copy is made of the target leaf by calling
> btrfs_clone_extent_buffer. The latter allocates a new buffer and
> attaches a new set of pages and copies the content of the source
> buffer. The new scratch buffer is only used to iterate it's items, it's
> not published anywhere and cannot be accessed by a third party. Hence,
> it's not necessary to perform any locking on it whatsoever. Furthermore,
> remove the extra extent_buffer_get call since the new buffer is always
> allocated with a reference count of 1 which is sufficient here.
> No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

I'm going to be sending this patch as part of a larger series so you can
ignore it for now.

> ---
>  fs/btrfs/qgroup.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 1874a6d2e6f5..3b57dc247fa2 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2647,9 +2647,6 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
>  		mutex_unlock(&fs_info->qgroup_rescan_lock);
>  		goto out;
>  	}
> -	extent_buffer_get(scratch_leaf);
> -	btrfs_tree_read_lock(scratch_leaf);
> -	btrfs_set_lock_blocking_rw(scratch_leaf, BTRFS_READ_LOCK);
>  	slot = path->slots[0];
>  	btrfs_release_path(path);
>  	mutex_unlock(&fs_info->qgroup_rescan_lock);
> @@ -2675,10 +2672,8 @@ qgroup_rescan_leaf(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
>  			goto out;
>  	}
>  out:
> -	if (scratch_leaf) {
> -		btrfs_tree_read_unlock_blocking(scratch_leaf);
> +	if (scratch_leaf)
>  		free_extent_buffer(scratch_leaf);
> -	}
>  
>  	if (done && !ret)
>  		ret = 1;
> 

Re: [PATCH 1/4] btrfs: Refactor loop in btrfs_release_extent_buffer_page

[David Sterba]

On Mon, Jul 02, 2018 at 01:03:41PM +0300, Nikolay Borisov wrote:
> 
> 
> On 29.06.2018 15:35, David Sterba wrote:
> > On Wed, Jun 27, 2018 at 04:38:22PM +0300, Nikolay Borisov wrote:
> >> The purpose of the function is to free all the pages comprising an
> >> extent buffer. This can be achieved with a simple for loop rather than
> >> the slitghly more involved 'do {} while' construct. So rewrite the
> >> loop using a 'for' construct. Additionally we can never have an
> >> extent_buffer that is 0 pages so remove the check for index == 0. No
> >> functional changes.
> > 
> > The reversed loop makes sense, the first page is special and used for
> > locking the whole extent buffer's pages, as can be seen eg.
> > 897ca6e9b4fef86d5dfb6b31 or 4f2de97acee6532b36dd6e99 . Also see current
> > alloc_extent_buffer for the locking and managing the private bit.
> 
> Turns out  page[0] is not special and it's not used for any special
> locking whatsoever. In csum_dirty_buffer this page is used so that when
> we submit a multipage eb then we only perform csum calculation once, i.e
> when the first page (page[0]) is submitted.
> btrfs_release_extent_buffer_page OTOH no longer takes an index but just
> an EB and iterates all the pages.
> 
> The fact that page0 is unlocked last in alloc_extent_buffer seems to be
> an artefact of the code refactoring rather than a deliberate behavior.
> Furthermore I've been running tests with sequential unlocking (and
> complete removal of the SetPageChecked/ClearPageChecked function from
> alloc_extent_buffer) of the pages and didn't observe any problems
> whatsoever on both 4g and 1g configuration ( the latter is more likely
> to trigger premature page eviction hence trigger any lurking races
> between alloc_extent_buffer and btree_releasepage. I will be sending a
> patch after I do a bit more testing but so far the results are good.

With more patch archeology I agree that the page zero is not used in any
sense that would require the reversed loop. Patch added to misc-next.

Re: [PATCH 3/4] btrfs: Rename EXTENT_BUFFER_DUMMY to EXTENT_BUFFER_PRIVATE

[David Sterba]

On Fri, Jun 29, 2018 at 04:42:41PM +0300, Nikolay Borisov wrote:
> 
> 
> On 29.06.2018 16:07, Qu Wenruo wrote:
> > 
> > 
> > On 2018年06月29日 20:46, David Sterba wrote:
> >> On Wed, Jun 27, 2018 at 04:38:24PM +0300, Nikolay Borisov wrote:
> >>> EXTENT_BUFFER_DUMMY is an awful name for this flag. Buffers which have
> >>> this flag set are not in any way dummy. Rather, they are private in
> >>> the sense that are not linked to the global buffer tree. This flag has
> >>> subtle implications to the way free_extent_buffer work for example, as
> >>> well as controls whether page->mapping->private_lock is held during
> >>> extent_buffer release. Pages for a private buffer cannot be under io,
> >>> nor can they be written by a 3rd party so taking the lock is
> >>> unnecessary.
> >>>
> >>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> >>> ---
> >>>  fs/btrfs/disk-io.c   |  2 +-
> >>>  fs/btrfs/extent_io.c | 10 +++++-----
> >>>  fs/btrfs/extent_io.h |  2 +-
> >>>  3 files changed, 7 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >>> index 8a469f70d5ee..1c655be92690 100644
> >>> --- a/fs/btrfs/disk-io.c
> >>> +++ b/fs/btrfs/disk-io.c
> >>> @@ -4093,7 +4093,7 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
> >>>  	 * enabled.  Normal people shouldn't be marking dummy buffers as dirty
> >>>  	 * outside of the sanity tests.
> >>>  	 */
> >>> -	if (unlikely(test_bit(EXTENT_BUFFER_DUMMY, &buf->bflags)))
> >>> +	if (unlikely(test_bit(EXTENT_BUFFER_PRIVATE, &buf->bflags)))
> >>
> >> This is going to be confusing. There's page Private bit,
> >> PAGE_SET_PRIVATE2 and EXTENT_PAGE_PRIVATE, that are somehow logically
> >> connected.
> >>
> >> I'd suggest EXTENT_BUFFER_CLONED or _UNMAPPED as it's created by
> >> btrfs_clone_extent_buffer or used in the disconnected way (ie. without
> >> the mapping).
> > 
> > UNMAPPED looks good to me.
> > (Or ANONYMOUS?)
> 
> I'm more leaning towards UNMAPPED at this point.

Ok, renamed it to EXTENT_BUFFER_UNMAPPED.