[PATCH 0/7] More misc fixes

[PATCH 0/7] More misc fixes

[Nikolay Borisov]

Here is an assortment of fixes, mainly around the async (compressed) cow path. 
Just removes some redundant arguments/local variables, makes shrink_delalloc a
bit more readable by simplifying variable usage and giving more appropriate names. 
Also documents the ->inode null check in async_cow_submit and cleans up open-coded
usage of DIV_ROUND_UP. No functional changes to any of the patches. 

Nikolay Borisov (7):
  btrfs: Remove inode argument from async_cow_submit
  btrfs: Remove isize local variable in compress_file_range
  btrfs: Use ihold instead of igrab in cow_file_range_async
  btrfs: Remove WARN_ON in btrfs_alloc_delalloc_work
  btrfs: Document logic in async_cow_submit
  btrfs: Replace open-coded maths with DIV_ROUND_UP
  btrfs: Refactor shrink_delalloc

 fs/btrfs/extent-tree.c | 33 ++++++++++++++++++++++-----------
 fs/btrfs/extent_io.c   |  7 +++----
 fs/btrfs/inode.c       | 27 +++++++++++++++------------
 3 files changed, 40 insertions(+), 27 deletions(-)

-- 
2.17.1

[PATCH 1/7] btrfs: Remove inode argument from async_cow_submit

[Nikolay Borisov]

We already pass the async_cow struct that holds a reference to the
inode. Exploit this fact and remove the extra inode argument. No
functional changes.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 38c0f385a617..43baca50fba5 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -714,9 +714,9 @@ static void free_async_extent_pages(struct async_extent *async_extent)
  * queued.  We walk all the async extents created by compress_file_range
  * and send them down to the disk.
  */
-static noinline void submit_compressed_extents(struct inode *inode,
-					      struct async_cow *async_cow)
+static noinline void submit_compressed_extents(struct async_cow *async_cow)
 {
+	struct inode *inode = async_cow->inode;
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct async_extent *async_extent;
 	u64 alloc_hint = 0;
@@ -1167,7 +1167,7 @@ static noinline void async_cow_submit(struct btrfs_work *work)
 		cond_wake_up_nomb(&fs_info->async_submit_wait);
 
 	if (async_cow->inode)
-		submit_compressed_extents(async_cow->inode, async_cow);
+		submit_compressed_extents(async_cow);
 }
 
 static noinline void async_cow_free(struct btrfs_work *work)
-- 
2.17.1

Re: [PATCH 1/7] btrfs: Remove inode argument from async_cow_submit

[Anand Jain]

On 01/03/2019 04:49 PM, Nikolay Borisov wrote:
> We already pass the async_cow struct that holds a reference to the
> inode. Exploit this fact and remove the extra inode argument. No
> functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

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


> ---
>   fs/btrfs/inode.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 38c0f385a617..43baca50fba5 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -714,9 +714,9 @@ static void free_async_extent_pages(struct async_extent *async_extent)
>    * queued.  We walk all the async extents created by compress_file_range
>    * and send them down to the disk.
>    */
> -static noinline void submit_compressed_extents(struct inode *inode,
> -					      struct async_cow *async_cow)
> +static noinline void submit_compressed_extents(struct async_cow *async_cow)
>   {
> +	struct inode *inode = async_cow->inode;
>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>   	struct async_extent *async_extent;
>   	u64 alloc_hint = 0;
> @@ -1167,7 +1167,7 @@ static noinline void async_cow_submit(struct btrfs_work *work)
>   		cond_wake_up_nomb(&fs_info->async_submit_wait);
>   
>   	if (async_cow->inode)
> -		submit_compressed_extents(async_cow->inode, async_cow);
> +		submit_compressed_extents(async_cow);
>   }
>   
>   static noinline void async_cow_free(struct btrfs_work *work)
> 

Re: [PATCH 1/7] btrfs: Remove inode argument from async_cow_submit

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 2/7] btrfs: Remove isize local variable in compress_file_range

[Nikolay Borisov]

It's used only once so just inline the call to i_size_read. 

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 43baca50fba5..97ad494d0b3c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -453,7 +453,6 @@ static noinline void compress_file_range(struct inode *inode,
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	u64 blocksize = fs_info->sectorsize;
 	u64 actual_end;
-	u64 isize = i_size_read(inode);
 	int ret = 0;
 	struct page **pages = NULL;
 	unsigned long nr_pages;
@@ -467,7 +466,7 @@ static noinline void compress_file_range(struct inode *inode,
 	inode_should_defrag(BTRFS_I(inode), start, end, end - start + 1,
 			SZ_16K);
 
-	actual_end = min_t(u64, isize, end + 1);
+	actual_end = min_t(u64, i_size_read(inode), end + 1);
 again:
 	will_compress = 0;
 	nr_pages = (end >> PAGE_SHIFT) - (start >> PAGE_SHIFT) + 1;
-- 
2.17.1

Re: [PATCH 2/7] btrfs: Remove isize local variable in compress_file_range

[Anand Jain]

On 01/03/2019 04:50 PM, Nikolay Borisov wrote:
> It's used only once so just inline the call to i_size_read.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

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

> ---
>   fs/btrfs/inode.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 43baca50fba5..97ad494d0b3c 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -453,7 +453,6 @@ static noinline void compress_file_range(struct inode *inode,
>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>   	u64 blocksize = fs_info->sectorsize;
>   	u64 actual_end;
> -	u64 isize = i_size_read(inode);
>   	int ret = 0;
>   	struct page **pages = NULL;
>   	unsigned long nr_pages;
> @@ -467,7 +466,7 @@ static noinline void compress_file_range(struct inode *inode,
>   	inode_should_defrag(BTRFS_I(inode), start, end, end - start + 1,
>   			SZ_16K);
>   
> -	actual_end = min_t(u64, isize, end + 1);
> +	actual_end = min_t(u64, i_size_read(inode), end + 1);
>   again:
>   	will_compress = 0;
>   	nr_pages = (end >> PAGE_SHIFT) - (start >> PAGE_SHIFT) + 1;
> 

Re: [PATCH 2/7] btrfs: Remove isize local variable in compress_file_range

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 2/7] btrfs: Remove isize local variable in compress_file_range

[David Sterba]

On Thu, Jan 03, 2019 at 10:50:00AM +0200, Nikolay Borisov wrote:
> It's used only once so just inline the call to i_size_read. 
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

I'll update changelog to note that the semantics of i_size has not
changed.

[PATCH 3/7] btrfs: Use ihold instead of igrab in cow_file_range_async

[Nikolay Borisov]

ihold is supposed to be used when the caller already has a reference to
the inode. In the case of cow_file_range_async this invariants holds,
since the 3 call chains leading to this function all take a reference:

btrfs_writepage  <--- does igrab
 extent_write_full_page
  __extent_writepage
   writepage_delalloc
     btrfs_run_delalloc_range
      cow_file_range_async

extent_write_cache_pages <--- does igrab
 __extent_writepage (same callchain as above)

and

submit_compressed_extents <-- already called from async CoW submit path,
			      which would have done ihold.
 extent_write_locked_range
  __extent_writepage

No functional changes.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 97ad494d0b3c..f023d2b5ef49 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1194,7 +1194,8 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 		async_cow = kmalloc(sizeof(*async_cow), GFP_NOFS);
 		if (!async_cow)
 			return -ENOMEM;
-		async_cow->inode = igrab(inode);
+		ihold(inode);
+		async_cow->inode = inode;
 		async_cow->fs_info = fs_info;
 		async_cow->locked_page = locked_page;
 		async_cow->start = start;
-- 
2.17.1

Re: [PATCH 3/7] btrfs: Use ihold instead of igrab in cow_file_range_async

[David Sterba]

On Thu, Jan 03, 2019 at 10:50:01AM +0200, Nikolay Borisov wrote:
> ihold is supposed to be used when the caller already has a reference to
> the inode. In the case of cow_file_range_async this invariants holds,
> since the 3 call chains leading to this function all take a reference:
> 
> btrfs_writepage  <--- does igrab
>  extent_write_full_page
>   __extent_writepage
>    writepage_delalloc
>      btrfs_run_delalloc_range
>       cow_file_range_async
> 
> extent_write_cache_pages <--- does igrab
>  __extent_writepage (same callchain as above)
> 
> and
> 
> submit_compressed_extents <-- already called from async CoW submit path,
> 			      which would have done ihold.
>  extent_write_locked_range
>   __extent_writepage

So by the logic the inode reference needs to be taken early, eg. in
btrfs_writepage and all low-level callbacks or helpers are fine with
ihold.

Reviewed-by: David Sterba <dsterba@suse.com>

[PATCH 4/7] btrfs: Remove WARN_ON in btrfs_alloc_delalloc_work

[Nikolay Borisov]

It can never trigger since before calling alloc_delalloc_work we have
called igrab in start_delalloc_inodes.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index f023d2b5ef49..1a7f790b68e8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9943,7 +9943,6 @@ static struct btrfs_delalloc_work *btrfs_alloc_delalloc_work(struct inode *inode
 	init_completion(&work->completion);
 	INIT_LIST_HEAD(&work->list);
 	work->inode = inode;
-	WARN_ON_ONCE(!inode);
 	btrfs_init_work(&work->work, btrfs_flush_delalloc_helper,
 			btrfs_run_delalloc_work, NULL, NULL);
 
-- 
2.17.1

Re: [PATCH 4/7] btrfs: Remove WARN_ON in btrfs_alloc_delalloc_work

[David Sterba]

On Thu, Jan 03, 2019 at 10:50:02AM +0200, Nikolay Borisov wrote:
> It can never trigger since before calling alloc_delalloc_work we have
> called igrab in start_delalloc_inodes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 4/7] btrfs: Remove WARN_ON in btrfs_alloc_delalloc_work

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 5/7] btrfs: Document logic in async_cow_submit

[Nikolay Borisov]

Add a comment explaining when ->inode could be null and why we always
perform the ->async_delalloc_pages modification.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 1a7f790b68e8..41ad0d06b3d4 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1165,6 +1165,12 @@ static noinline void async_cow_submit(struct btrfs_work *work)
 	    5 * SZ_1M)
 		cond_wake_up_nomb(&fs_info->async_submit_wait);
 
+	/*
+	 * ->inode could be NULL if async_cow_start has failed to compress,
+	 * in which case we don't have anything to submit, yet we need to
+	 * adjust ->async_delalloc_pages always as its paired to the init
+	 * happening in cow_file_range_async
+	 */
 	if (async_cow->inode)
 		submit_compressed_extents(async_cow);
 }
-- 
2.17.1

[PATCH 6/7] btrfs: Replace open-coded maths with DIV_ROUND_UP

[Nikolay Borisov]

In a couple of places it's required to calculate the number of pages
given a start and end offsets. Currently this is opencoded, unify the
code base by replacing all such sites with the DIV_ROUND_UP macro. Also,
current open-coded sites were buggy in that they were adding
'PAGE_SIZE', rather than 'PAGE_SIZE - 1'.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent_io.c | 7 +++----
 fs/btrfs/inode.c     | 8 +++-----
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b9b30e618378..0cd41ec8b698 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3189,8 +3189,8 @@ static noinline_for_stack int writepage_delalloc(struct inode *inode,
 		 * delalloc_end is already one less than the total length, so
 		 * we don't subtract one from PAGE_SIZE
 		 */
-		delalloc_to_write += (delalloc_end - delalloc_start +
-				      PAGE_SIZE) >> PAGE_SHIFT;
+		delalloc_to_write += DIV_ROUND_UP(delalloc_end - delalloc_start,
+						  PAGE_SIZE);
 		delalloc_start = delalloc_end + 1;
 	}
 	if (wbc->nr_to_write < delalloc_to_write) {
@@ -4001,8 +4001,7 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
 	struct address_space *mapping = inode->i_mapping;
 	struct extent_io_tree *tree = &BTRFS_I(inode)->io_tree;
 	struct page *page;
-	unsigned long nr_pages = (end - start + PAGE_SIZE) >>
-		PAGE_SHIFT;
+	unsigned long nr_pages = DIV_ROUND_UP(end - start, PAGE_SIZE);
 
 	struct extent_page_data epd = {
 		.bio = NULL,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 41ad0d06b3d4..32977174826b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -469,7 +469,7 @@ static noinline void compress_file_range(struct inode *inode,
 	actual_end = min_t(u64, i_size_read(inode), end + 1);
 again:
 	will_compress = 0;
-	nr_pages = (end >> PAGE_SHIFT) - (start >> PAGE_SHIFT) + 1;
+	nr_pages = DIV_ROUND_UP(end - start, PAGE_SIZE);
 	BUILD_BUG_ON((BTRFS_MAX_COMPRESSED % PAGE_SIZE) != 0);
 	nr_pages = min_t(unsigned long, nr_pages,
 			BTRFS_MAX_COMPRESSED / PAGE_SIZE);
@@ -1157,8 +1157,7 @@ static noinline void async_cow_submit(struct btrfs_work *work)
 	async_cow = container_of(work, struct async_cow, work);
 
 	fs_info = async_cow->fs_info;
-	nr_pages = (async_cow->end - async_cow->start + PAGE_SIZE) >>
-		PAGE_SHIFT;
+	nr_pages = DIV_ROUND_UP(async_cow->end - async_cow->start, PAGE_SIZE);
 
 	/* atomic_sub_return implies a barrier */
 	if (atomic_sub_return(nr_pages, &fs_info->async_delalloc_pages) <
@@ -1221,8 +1220,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 				async_cow_start, async_cow_submit,
 				async_cow_free);
 
-		nr_pages = (cur_end - start + PAGE_SIZE) >>
-			PAGE_SHIFT;
+		nr_pages = DIV_ROUND_UP(cur_end - start, PAGE_SIZE);
 		atomic_add(nr_pages, &fs_info->async_delalloc_pages);
 
 		btrfs_queue_work(fs_info->delalloc_workers, &async_cow->work);
-- 
2.17.1

Re: [PATCH 6/7] btrfs: Replace open-coded maths with DIV_ROUND_UP

[David Sterba]

On Thu, Jan 03, 2019 at 10:50:04AM +0200, Nikolay Borisov wrote:
> In a couple of places it's required to calculate the number of pages
> given a start and end offsets. Currently this is opencoded, unify the
> code base by replacing all such sites with the DIV_ROUND_UP macro. Also,
> current open-coded sites were buggy in that they were adding
> 'PAGE_SIZE', rather than 'PAGE_SIZE - 1'.

Didn't you find it strange that it's so consistently wrong? After a
closer inspection, you'd find that the end of the range is inclusive. So
the math is correct and your patch introduces a bug.

end - start + PAGE_SIZE = end + 1 - start + PAGE_SIZE - 1

Check eg. writepage_delalloc how it sets up page_end.

The correct use in DIV_ROUND_UP needs +1 adjustment.

Re: [PATCH 6/7] btrfs: Replace open-coded maths with DIV_ROUND_UP

[Nikolay Borisov]

On 3.01.19 г. 16:44 ч., David Sterba wrote:
> On Thu, Jan 03, 2019 at 10:50:04AM +0200, Nikolay Borisov wrote:
>> In a couple of places it's required to calculate the number of pages
>> given a start and end offsets. Currently this is opencoded, unify the
>> code base by replacing all such sites with the DIV_ROUND_UP macro. Also,
>> current open-coded sites were buggy in that they were adding
>> 'PAGE_SIZE', rather than 'PAGE_SIZE - 1'.
> 
> Didn't you find it strange that it's so consistently wrong? After a
> closer inspection, you'd find that the end of the range is inclusive. So
> the math is correct and your patch introduces a bug.

But since we are talking about number of pages, why does the range need
to be inclusive. Indeed, let's take delalloc_to_write in
writepage_delalloc. Say we have a 1mb range, 0-1m - that's 256 pages
right so with DIV_ROUND_UP we'll have:

(1048576 + 4096 - 1) / 4096 = 256

with the old version we'll have:

1048576 + 4096 / 4096 = 257

Since delalloc_to_write is used in a context where we care about the
number of pages written I think using DIV_ROUND_UP is correct and it
fixes an off-by-one bug, no ?

Let's take extent_write_locked_range, it's called only from
submit_compressed_extents with the range for the async (compressed) extent:

Say we have an extent which is 2k in ram size and starts at 1m offset.
We'll have:
start = 1048576
end = 1048576 + 2048 - 1 = 1050623

nr_pages in this case should be 1, with DIV_ROUND_UP:

nr_pages = 2047 + 4096 -1 / 4096 = 1 (1,4995 actually but due to integer
math we don't care about floating point part)

with old formula:

nr_pages = (2047 + 4096) / 4096 = 1 (1,4998 but ditto as above).


> 
> end - start + PAGE_SIZE = end + 1 - start + PAGE_SIZE - 1
> 
> Check eg. writepage_delalloc how it sets up page_end.
> 
> The correct use in DIV_ROUND_UP needs +1 adjustment.
> 

Re: [PATCH 6/7] btrfs: Replace open-coded maths with DIV_ROUND_UP

[David Sterba]

On Thu, Jan 03, 2019 at 05:33:26PM +0200, Nikolay Borisov wrote:
> 
> 
> On 3.01.19 г. 16:44 ч., David Sterba wrote:
> > On Thu, Jan 03, 2019 at 10:50:04AM +0200, Nikolay Borisov wrote:
> >> In a couple of places it's required to calculate the number of pages
> >> given a start and end offsets. Currently this is opencoded, unify the
> >> code base by replacing all such sites with the DIV_ROUND_UP macro. Also,
> >> current open-coded sites were buggy in that they were adding
> >> 'PAGE_SIZE', rather than 'PAGE_SIZE - 1'.
> > 
> > Didn't you find it strange that it's so consistently wrong? After a
> > closer inspection, you'd find that the end of the range is inclusive. So
> > the math is correct and your patch introduces a bug.
> 
> But since we are talking about number of pages, why does the range need
> to be inclusive. Indeed, let's take delalloc_to_write in
> writepage_delalloc. Say we have a 1mb range, 0-1m - that's 256 pages
> right so with DIV_ROUND_UP we'll have:
> 
> (1048576 + 4096 - 1) / 4096 = 256
> 
> with the old version we'll have:
> 
> 1048576 + 4096 / 4096 = 257

No, it's not 1048576 but 1048575. The end of the range is inclusive, ie.
the value in end is the end of the interval. Not one byte after.

It's even written in the comment in writepage_delalloc just above the
line where you switch to DIV_ROUND_UP.

[PATCH 7/7] btrfs: Refactor shrink_delalloc

[Nikolay Borisov]

Add a couple of comments regarding the logic flow in shrink_delalloc.
Then, cease using max_reclaim as a temporary variable when calculating
nr_pages. Finally give max_reclaim a more becoming name, which
uneqivocally shows at what this variable really holds. No functional
changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index b15afeae16df..615441f4d458 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4743,7 +4743,7 @@ static void shrink_delalloc(struct btrfs_fs_info *fs_info, u64 to_reclaim,
 	struct btrfs_space_info *space_info;
 	struct btrfs_trans_handle *trans;
 	u64 delalloc_bytes;
-	u64 max_reclaim;
+	u64 async_pages;
 	u64 items;
 	long time_left;
 	unsigned long nr_pages;
@@ -4768,25 +4768,36 @@ static void shrink_delalloc(struct btrfs_fs_info *fs_info, u64 to_reclaim,
 
 	loops = 0;
 	while (delalloc_bytes && loops < 3) {
-		max_reclaim = min(delalloc_bytes, to_reclaim);
-		nr_pages = max_reclaim >> PAGE_SHIFT;
+		nr_pages = min(delalloc_bytes, to_reclaim) >> PAGE_SHIFT;
+
+		/*
+		 * Triggers inode writeback for up to nr_pages. This will invoke
+		 * ->writepages callback and trigger delalloc filling
+		 *  (btrfs_run_delalloc_range()).
+		 */
 		btrfs_writeback_inodes_sb_nr(fs_info, nr_pages, items);
+
 		/*
-		 * We need to wait for the async pages to actually start before
-		 * we do anything.
+		 * We need to wait for the compressed pages to start before
+		 * we continue.
 		 */
-		max_reclaim = atomic_read(&fs_info->async_delalloc_pages);
-		if (!max_reclaim)
+		async_pages = atomic_read(&fs_info->async_delalloc_pages);
+		if (!async_pages)
 			goto skip_async;
 
-		if (max_reclaim <= nr_pages)
-			max_reclaim = 0;
+		/*
+		 * Calculate how many compressed pages we want to be written
+		 * before we continue. I.e if there are more async pages than we
+		 * require wait_event will wait until nr_pages are written.
+		 */
+		if (async_pages <= nr_pages)
+			async_pages = 0;
 		else
-			max_reclaim -= nr_pages;
+			async_pages -= nr_pages;
 
 		wait_event(fs_info->async_submit_wait,
 			   atomic_read(&fs_info->async_delalloc_pages) <=
-			   (int)max_reclaim);
+			   (int)async_pages);
 skip_async:
 		spin_lock(&space_info->lock);
 		if (list_empty(&space_info->tickets) &&
-- 
2.17.1

Re: [PATCH 7/7] btrfs: Refactor shrink_delalloc

[David Sterba]

On Thu, Jan 03, 2019 at 10:50:05AM +0200, Nikolay Borisov wrote:
> Add a couple of comments regarding the logic flow in shrink_delalloc.
> Then, cease using max_reclaim as a temporary variable when calculating
> nr_pages. Finally give max_reclaim a more becoming name, which
> uneqivocally shows at what this variable really holds. No functional
> changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/extent-tree.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index b15afeae16df..615441f4d458 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4743,7 +4743,7 @@ static void shrink_delalloc(struct btrfs_fs_info *fs_info, u64 to_reclaim,
>  	struct btrfs_space_info *space_info;
>  	struct btrfs_trans_handle *trans;
>  	u64 delalloc_bytes;
> -	u64 max_reclaim;
> +	u64 async_pages;

This could be int, as it's compared to an atomic_t, otherwise ok.

>  	u64 items;
>  	long time_left;
>  	unsigned long nr_pages;
> @@ -4768,25 +4768,36 @@ static void shrink_delalloc(struct btrfs_fs_info *fs_info, u64 to_reclaim,
>  
>  	loops = 0;
>  	while (delalloc_bytes && loops < 3) {
> -		max_reclaim = min(delalloc_bytes, to_reclaim);
> -		nr_pages = max_reclaim >> PAGE_SHIFT;
> +		nr_pages = min(delalloc_bytes, to_reclaim) >> PAGE_SHIFT;
> +
> +		/*
> +		 * Triggers inode writeback for up to nr_pages. This will invoke
> +		 * ->writepages callback and trigger delalloc filling
> +		 *  (btrfs_run_delalloc_range()).
> +		 */
>  		btrfs_writeback_inodes_sb_nr(fs_info, nr_pages, items);
> +
>  		/*
> -		 * We need to wait for the async pages to actually start before
> -		 * we do anything.
> +		 * We need to wait for the compressed pages to start before
> +		 * we continue.
>  		 */
> -		max_reclaim = atomic_read(&fs_info->async_delalloc_pages);
> -		if (!max_reclaim)
> +		async_pages = atomic_read(&fs_info->async_delalloc_pages);
> +		if (!async_pages)
>  			goto skip_async;
>  
> -		if (max_reclaim <= nr_pages)
> -			max_reclaim = 0;
> +		/*
> +		 * Calculate how many compressed pages we want to be written
> +		 * before we continue. I.e if there are more async pages than we
> +		 * require wait_event will wait until nr_pages are written.
> +		 */
> +		if (async_pages <= nr_pages)
> +			async_pages = 0;
>  		else
> -			max_reclaim -= nr_pages;
> +			async_pages -= nr_pages;
>  
>  		wait_event(fs_info->async_submit_wait,
>  			   atomic_read(&fs_info->async_delalloc_pages) <=
> -			   (int)max_reclaim);
> +			   (int)async_pages);
>  skip_async:
>  		spin_lock(&space_info->lock);
>  		if (list_empty(&space_info->tickets) &&
> -- 
> 2.17.1

Re: [PATCH 7/7] btrfs: Refactor shrink_delalloc

[David Sterba]

On Fri, Jan 04, 2019 at 04:35:43PM +0100, David Sterba wrote:
> > @@ -4743,7 +4743,7 @@ static void shrink_delalloc(struct btrfs_fs_info *fs_info, u64 to_reclaim,
> >  	struct btrfs_space_info *space_info;
> >  	struct btrfs_trans_handle *trans;
> >  	u64 delalloc_bytes;
> > -	u64 max_reclaim;
> > +	u64 async_pages;
> 
> This could be int, as it's compared to an atomic_t, otherwise ok.

As this is a different change, please send a separate patch, I've
applied the rename to misc-next.

Re: [PATCH 0/7] More misc fixes

[David Sterba]

On Thu, Jan 03, 2019 at 10:49:58AM +0200, Nikolay Borisov wrote:
> Here is an assortment of fixes, mainly around the async (compressed) cow path. 
> Just removes some redundant arguments/local variables, makes shrink_delalloc a
> bit more readable by simplifying variable usage and giving more appropriate names. 
> Also documents the ->inode null check in async_cow_submit and cleans up open-coded
> usage of DIV_ROUND_UP. No functional changes to any of the patches. 
> 
> Nikolay Borisov (7):
>   btrfs: Remove inode argument from async_cow_submit
>   btrfs: Remove isize local variable in compress_file_range
>   btrfs: Use ihold instead of igrab in cow_file_range_async
>   btrfs: Remove WARN_ON in btrfs_alloc_delalloc_work
>   btrfs: Document logic in async_cow_submit
>   btrfs: Replace open-coded maths with DIV_ROUND_UP
>   btrfs: Refactor shrink_delalloc

All but 6/7 applied, the one still needs to be clarified.