[PATCH 0/4] Cleanups around compress path

[PATCH 0/4] Cleanups around compress path

[Nikolay Borisov]

This series aims to streamline the logic in cow_file_range_async which is 
invoked if extents have to be compressed. The main change is that the logic of 
the loop in the function is now centered around the idea of chunks (in case 
compression is enabled). So it's obvious that we will submit as many chunks as 
necessary. The 2nd improvement is to eliminate the BUG_ON that is dependent on the 
return value of kmalloc. Now, an array of struct async_cow is allocated, preceded 
by an atomic_t, shared by all chunks. This allows to bail out before submitting 
any io whatsoever and mark the page as errored out. 

Rest of the patches are pure quality-of-live improvements - simplifying function
signature, removing unused code/redundant data members. 
 
Needless to say this has survived full xfstest run. 

Nikolay Borisov (4):
  btrfs: Refactor cow_file_range_async
  btrfs: Remove fs_info from struct async_cow
  btrfs: Make compress_file_range only struct async_cow
  btrfs: Replace clear_extent_bit with unlock_extent

 fs/btrfs/inode.c | 105 +++++++++++++++++++++++++++--------------------
 1 file changed, 60 insertions(+), 45 deletions(-)

-- 
2.17.1

[PATCH 1/4] btrfs: Refactor cow_file_range_async

[Nikolay Borisov]

This commit changes the implementation of cow_file_range_async in order
to get rid of the BUG_ON in the middle of the loop. Additionally it
reworks the inner loop in the hopes of making it more understandable.

Main change is that the number of chunks required to handle the given
range is calculated before going into the loop and the logic of the loop
just iterates the chunk count. Furthermore, the way memory is allocated
is reworked and now the code does a single kmalloc with enough space to
handle all chunks. Depending on whether compression is enabled or not
chunks are either 1 (in non-compress case) or the range divided by 512k.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 05bbfd02ea49..c2180f0a5f42 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -375,6 +375,7 @@ struct async_cow {
 	unsigned int write_flags;
 	struct list_head extents;
 	struct btrfs_work work;
+	atomic_t *pending;
 };
 
 static noinline int add_async_extent(struct async_cow *cow,
@@ -1181,7 +1182,12 @@ static noinline void async_cow_free(struct btrfs_work *work)
 	async_cow = container_of(work, struct async_cow, work);
 	if (async_cow->inode)
 		btrfs_add_delayed_iput(async_cow->inode);
-	kfree(async_cow);
+	/*
+	 * Since the pointer to 'pending' is at the beginning of the array of
+	 * async_cow's, freeing it ensures the whole array has been freed.
+	 */
+	if (atomic_dec_and_test(async_cow->pending))
+		kfree(async_cow->pending);
 }
 
 static int cow_file_range_async(struct inode *inode, struct page *locked_page,
@@ -1193,42 +1199,59 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 	struct async_cow *async_cow;
 	unsigned long nr_pages;
 	u64 cur_end;
+	u64 num_chunks = DIV_ROUND_UP(end - start, SZ_512K);
+	int i;
+	bool should_compress;
+	atomic_t *p;
+
 
 	clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, EXTENT_LOCKED,
 			 1, 0, NULL);
-	while (start < end) {
-		async_cow = kmalloc(sizeof(*async_cow), GFP_NOFS);
-		BUG_ON(!async_cow); /* -ENOMEM */
-		/*
-		 * igrab is called higher up in the call chain, take only the
-		 * lightweight reference for the callback lifetime
-		 */
-		ihold(inode);
-		async_cow->inode = inode;
-		async_cow->fs_info = fs_info;
-		async_cow->locked_page = locked_page;
-		async_cow->start = start;
-		async_cow->write_flags = write_flags;
-
-		if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
-		    !btrfs_test_opt(fs_info, FORCE_COMPRESS))
-			cur_end = end;
-		else
-			cur_end = min(end, start + SZ_512K - 1);
 
-		async_cow->end = cur_end;
-		INIT_LIST_HEAD(&async_cow->extents);
+	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
+	    !btrfs_test_opt(fs_info, FORCE_COMPRESS)) {
+		num_chunks = 1;
+		should_compress = false;
+	} else {
+		should_compress = true;
+	}
+
+	/* Layout is [atomic_t][async_cow1][async_cowN].... */
+	async_cow = kmalloc(sizeof(atomic_t) + num_chunks*sizeof(*async_cow),
+			    GFP_NOFS);
+	if (!async_cow)
+		return -ENOMEM;
+
+	p = (atomic_t *)async_cow;
+	async_cow = (struct async_cow *)((char *)async_cow + sizeof(atomic_t));
+	atomic_set(p, num_chunks);
+
+	for (i = 0; i < num_chunks; i++) {
 
-		btrfs_init_work(&async_cow->work,
+		if (should_compress)
+			cur_end = min(end, start + SZ_512K - 1);
+		else
+			cur_end = end;
+
+		ihold(inode);
+		async_cow[i].pending= p;
+		async_cow[i].inode = inode;
+		async_cow[i].start = start;
+		async_cow[i].end = cur_end;
+		async_cow[i].fs_info = fs_info;
+		async_cow[i].locked_page = locked_page;
+		async_cow[i].write_flags = write_flags;
+		INIT_LIST_HEAD(&async_cow[i].extents);
+
+		btrfs_init_work(&async_cow[i].work,
 				btrfs_delalloc_helper,
 				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);
+		btrfs_queue_work(fs_info->delalloc_workers, &async_cow[i].work);
 
 		*nr_written += nr_pages;
 		start = cur_end + 1;
-- 
2.17.1

[PATCH 2/4] btrfs: Remove fs_info from struct async_cow

[Nikolay Borisov]

We always pass an inode alongside async_cow. fs_info can be referenced
from this inode so it makes the explicit fs_info member in
struct async_cow redundant, remove it. No functional changes.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c2180f0a5f42..51147f389ebc 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -368,7 +368,6 @@ struct async_extent {
 
 struct async_cow {
 	struct inode *inode;
-	struct btrfs_fs_info *fs_info;
 	struct page *locked_page;
 	u64 start;
 	u64 end;
@@ -1151,13 +1150,10 @@ static noinline void async_cow_start(struct btrfs_work *work)
  */
 static noinline void async_cow_submit(struct btrfs_work *work)
 {
-	struct btrfs_fs_info *fs_info;
-	struct async_cow *async_cow;
+	struct async_cow *async_cow = container_of(work, struct async_cow, work);
+	struct btrfs_fs_info *fs_info = btrfs_work_owner(work);
 	unsigned long nr_pages;
 
-	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;
 
@@ -1238,7 +1234,6 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 		async_cow[i].inode = inode;
 		async_cow[i].start = start;
 		async_cow[i].end = cur_end;
-		async_cow[i].fs_info = fs_info;
 		async_cow[i].locked_page = locked_page;
 		async_cow[i].write_flags = write_flags;
 		INIT_LIST_HEAD(&async_cow[i].extents);
-- 
2.17.1

[PATCH 3/4] btrfs: Make compress_file_range only struct async_cow

[Nikolay Borisov]

All context this function needs is held within struct async_cow.
Currently we not only pass the struct but also every individual member.
This is redundant, simplify it by only passing struct async_cow and
leaving it to compress_file_range to extract the values it requires.
No functional changes.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 51147f389ebc..7a1a4082f8b2 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -444,14 +444,14 @@ static inline void inode_should_defrag(struct btrfs_inode *inode,
  * are written in the same order that the flusher thread sent them
  * down.
  */
-static noinline void compress_file_range(struct inode *inode,
-					struct page *locked_page,
-					u64 start, u64 end,
-					struct async_cow *async_cow,
-					int *num_added)
+static noinline void compress_file_range(struct async_cow *async_cow,
+					 int *num_added)
 {
+	struct inode *inode = async_cow->inode;
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	u64 blocksize = fs_info->sectorsize;
+	u64 start = async_cow->start;
+	u64 end = async_cow->end;
 	u64 actual_end;
 	int ret = 0;
 	struct page **pages = NULL;
@@ -670,9 +670,9 @@ static noinline void compress_file_range(struct inode *inode,
 	 * to our extent and set things up for the async work queue to run
 	 * cow_file_range to do the normal delalloc dance.
 	 */
-	if (page_offset(locked_page) >= start &&
-	    page_offset(locked_page) <= end)
-		__set_page_dirty_nobuffers(locked_page);
+	if (page_offset(async_cow->locked_page) >= start &&
+	    page_offset(async_cow->locked_page) <= end)
+		__set_page_dirty_nobuffers(async_cow->locked_page);
 		/* unlocked later on in the async handlers */
 
 	if (redirty)
@@ -1136,9 +1136,7 @@ static noinline void async_cow_start(struct btrfs_work *work)
 	int num_added = 0;
 	async_cow = container_of(work, struct async_cow, work);
 
-	compress_file_range(async_cow->inode, async_cow->locked_page,
-			    async_cow->start, async_cow->end, async_cow,
-			    &num_added);
+	compress_file_range(async_cow, &num_added);
 	if (num_added == 0) {
 		btrfs_add_delayed_iput(async_cow->inode);
 		async_cow->inode = NULL;
-- 
2.17.1

[PATCH 4/5] btrfs: Remove unused function btrfs_workqueue_owner

[Nikolay Borisov]

It was added as part of cb001095ca70 ("btrfs: plumb fs_info into btrfs_work")
however a user never really materialized so let's just remove it.
No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/async-thread.c | 6 ------
 fs/btrfs/async-thread.h | 1 -
 2 files changed, 7 deletions(-)

diff --git a/fs/btrfs/async-thread.c b/fs/btrfs/async-thread.c
index 122cb97c7909..36775139477a 100644
--- a/fs/btrfs/async-thread.c
+++ b/fs/btrfs/async-thread.c
@@ -61,12 +61,6 @@ noinline_for_stack void btrfs_##name(struct work_struct *arg)		\
 	normal_work_helper(work);					\
 }
 
-struct btrfs_fs_info *
-btrfs_workqueue_owner(const struct __btrfs_workqueue *wq)
-{
-	return wq->fs_info;
-}
-
 struct btrfs_fs_info *
 btrfs_work_owner(const struct btrfs_work *work)
 {
diff --git a/fs/btrfs/async-thread.h b/fs/btrfs/async-thread.h
index 7861c9feba5f..258367051b6d 100644
--- a/fs/btrfs/async-thread.h
+++ b/fs/btrfs/async-thread.h
@@ -71,7 +71,6 @@ void btrfs_destroy_workqueue(struct btrfs_workqueue *wq);
 void btrfs_workqueue_set_max(struct btrfs_workqueue *wq, int max);
 void btrfs_set_work_high_priority(struct btrfs_work *work);
 struct btrfs_fs_info *btrfs_work_owner(const struct btrfs_work *work);
-struct btrfs_fs_info *btrfs_workqueue_owner(const struct __btrfs_workqueue *wq);
 bool btrfs_workqueue_normal_congested(const struct btrfs_workqueue *wq);
 
 #endif
-- 
2.17.1

[PATCH 4/4] btrfs: Replace clear_extent_bit with unlock_extent

[Nikolay Borisov]

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 7a1a4082f8b2..632fdadfb04d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1199,8 +1199,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 	atomic_t *p;
 
 
-	clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, EXTENT_LOCKED,
-			 1, 0, NULL);
+	unlock_extent(&BTRFS_I(inode)->io_tree, start, end);
 
 	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
 	    !btrfs_test_opt(fs_info, FORCE_COMPRESS)) {
-- 
2.17.1

[PATCH 5/5] btrfs: Replace clear_extent_bit with unlock_extent

[Nikolay Borisov]

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 7a1a4082f8b2..632fdadfb04d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1199,8 +1199,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 	atomic_t *p;
 
 
-	clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, EXTENT_LOCKED,
-			 1, 0, NULL);
+	unlock_extent(&BTRFS_I(inode)->io_tree, start, end);
 
 	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
 	    !btrfs_test_opt(fs_info, FORCE_COMPRESS)) {
-- 
2.17.1

Re: [PATCH 0/4] Cleanups around compress path

[Nikolay Borisov]

On 13.02.19 г. 17:53 ч., Nikolay Borisov wrote:
> This series aims to streamline the logic in cow_file_range_async which is 
> invoked if extents have to be compressed. The main change is that the logic of 
> the loop in the function is now centered around the idea of chunks (in case 
> compression is enabled). So it's obvious that we will submit as many chunks as 
> necessary. The 2nd improvement is to eliminate the BUG_ON that is dependent on the 
> return value of kmalloc. Now, an array of struct async_cow is allocated, preceded 
> by an atomic_t, shared by all chunks. This allows to bail out before submitting 
> any io whatsoever and mark the page as errored out. 
> 
> Rest of the patches are pure quality-of-live improvements - simplifying function
> signature, removing unused code/redundant data members. 
>  
> Needless to say this has survived full xfstest run. 
> 
> Nikolay Borisov (4):
>   btrfs: Refactor cow_file_range_async
>   btrfs: Remove fs_info from struct async_cow
>   btrfs: Make compress_file_range only struct async_cow
>   btrfs: Replace clear_extent_bit with unlock_extent
> 
>  fs/btrfs/inode.c | 105 +++++++++++++++++++++++++++--------------------
>  1 file changed, 60 insertions(+), 45 deletions(-)
> 


Disregard this, I will resend.

Re: [PATCH 1/4] btrfs: Refactor cow_file_range_async

[Nikolay Borisov]

On 13.02.19 г. 17:55 ч., Nikolay Borisov wrote:
> This commit changes the implementation of cow_file_range_async in order
> to get rid of the BUG_ON in the middle of the loop. Additionally it
> reworks the inner loop in the hopes of making it more understandable.
> 
> Main change is that the number of chunks required to handle the given
> range is calculated before going into the loop and the logic of the loop
> just iterates the chunk count. Furthermore, the way memory is allocated
> is reworked and now the code does a single kmalloc with enough space to
> handle all chunks. Depending on whether compression is enabled or not
> chunks are either 1 (in non-compress case) or the range divided by 512k.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Scratch that patch since it could lead to deadlocks. It's not enough to
just return an error and hop higher levels will unlock the pages and
whatnot. Other callers (cow_file_range/run_delalloc_nocow) use
extent_clear_unlock_delalloc.
> ---
>  fs/btrfs/inode.c | 75 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 49 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 05bbfd02ea49..c2180f0a5f42 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -375,6 +375,7 @@ struct async_cow {
>  	unsigned int write_flags;
>  	struct list_head extents;
>  	struct btrfs_work work;
> +	atomic_t *pending;
>  };
>  
>  static noinline int add_async_extent(struct async_cow *cow,
> @@ -1181,7 +1182,12 @@ static noinline void async_cow_free(struct btrfs_work *work)
>  	async_cow = container_of(work, struct async_cow, work);
>  	if (async_cow->inode)
>  		btrfs_add_delayed_iput(async_cow->inode);
> -	kfree(async_cow);
> +	/*
> +	 * Since the pointer to 'pending' is at the beginning of the array of
> +	 * async_cow's, freeing it ensures the whole array has been freed.
> +	 */
> +	if (atomic_dec_and_test(async_cow->pending))
> +		kfree(async_cow->pending);
>  }
>  
>  static int cow_file_range_async(struct inode *inode, struct page *locked_page,
> @@ -1193,42 +1199,59 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>  	struct async_cow *async_cow;
>  	unsigned long nr_pages;
>  	u64 cur_end;
> +	u64 num_chunks = DIV_ROUND_UP(end - start, SZ_512K);
> +	int i;
> +	bool should_compress;
> +	atomic_t *p;
> +
>  
>  	clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, EXTENT_LOCKED,
>  			 1, 0, NULL);
> -	while (start < end) {
> -		async_cow = kmalloc(sizeof(*async_cow), GFP_NOFS);
> -		BUG_ON(!async_cow); /* -ENOMEM */
> -		/*
> -		 * igrab is called higher up in the call chain, take only the
> -		 * lightweight reference for the callback lifetime
> -		 */
> -		ihold(inode);
> -		async_cow->inode = inode;
> -		async_cow->fs_info = fs_info;
> -		async_cow->locked_page = locked_page;
> -		async_cow->start = start;
> -		async_cow->write_flags = write_flags;
> -
> -		if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
> -		    !btrfs_test_opt(fs_info, FORCE_COMPRESS))
> -			cur_end = end;
> -		else
> -			cur_end = min(end, start + SZ_512K - 1);
>  
> -		async_cow->end = cur_end;
> -		INIT_LIST_HEAD(&async_cow->extents);
> +	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
> +	    !btrfs_test_opt(fs_info, FORCE_COMPRESS)) {
> +		num_chunks = 1;
> +		should_compress = false;
> +	} else {
> +		should_compress = true;
> +	}
> +
> +	/* Layout is [atomic_t][async_cow1][async_cowN].... */
> +	async_cow = kmalloc(sizeof(atomic_t) + num_chunks*sizeof(*async_cow),
> +			    GFP_NOFS);
> +	if (!async_cow)
> +		return -ENOMEM;
> +
> +	p = (atomic_t *)async_cow;
> +	async_cow = (struct async_cow *)((char *)async_cow + sizeof(atomic_t));
> +	atomic_set(p, num_chunks);
> +
> +	for (i = 0; i < num_chunks; i++) {
>  
> -		btrfs_init_work(&async_cow->work,
> +		if (should_compress)
> +			cur_end = min(end, start + SZ_512K - 1);
> +		else
> +			cur_end = end;
> +
> +		ihold(inode);
> +		async_cow[i].pending= p;
> +		async_cow[i].inode = inode;
> +		async_cow[i].start = start;
> +		async_cow[i].end = cur_end;
> +		async_cow[i].fs_info = fs_info;
> +		async_cow[i].locked_page = locked_page;
> +		async_cow[i].write_flags = write_flags;
> +		INIT_LIST_HEAD(&async_cow[i].extents);
> +
> +		btrfs_init_work(&async_cow[i].work,
>  				btrfs_delalloc_helper,
>  				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);
> +		btrfs_queue_work(fs_info->delalloc_workers, &async_cow[i].work);
>  
>  		*nr_written += nr_pages;
>  		start = cur_end + 1;
> 

Re: [PATCH 1/4] btrfs: Refactor cow_file_range_async

[Nikolay Borisov]

On 13.02.19 г. 17:55 ч., Nikolay Borisov wrote:
> This commit changes the implementation of cow_file_range_async in order
> to get rid of the BUG_ON in the middle of the loop. Additionally it
> reworks the inner loop in the hopes of making it more understandable.
> 
> Main change is that the number of chunks required to handle the given
> range is calculated before going into the loop and the logic of the loop
> just iterates the chunk count. Furthermore, the way memory is allocated
> is reworked and now the code does a single kmalloc with enough space to
> handle all chunks. Depending on whether compression is enabled or not
> chunks are either 1 (in non-compress case) or the range divided by 512k.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/inode.c | 75 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 49 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 05bbfd02ea49..c2180f0a5f42 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -375,6 +375,7 @@ struct async_cow {
>  	unsigned int write_flags;
>  	struct list_head extents;
>  	struct btrfs_work work;
> +	atomic_t *pending;
>  };
>  
>  static noinline int add_async_extent(struct async_cow *cow,
> @@ -1181,7 +1182,12 @@ static noinline void async_cow_free(struct btrfs_work *work)
>  	async_cow = container_of(work, struct async_cow, work);
>  	if (async_cow->inode)
>  		btrfs_add_delayed_iput(async_cow->inode);
> -	kfree(async_cow);
> +	/*
> +	 * Since the pointer to 'pending' is at the beginning of the array of
> +	 * async_cow's, freeing it ensures the whole array has been freed.
> +	 */
> +	if (atomic_dec_and_test(async_cow->pending))
> +		kfree(async_cow->pending);
>  }
>  
>  static int cow_file_range_async(struct inode *inode, struct page *locked_page,
> @@ -1193,42 +1199,59 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>  	struct async_cow *async_cow;
>  	unsigned long nr_pages;
>  	u64 cur_end;
> +	u64 num_chunks = DIV_ROUND_UP(end - start, SZ_512K);
> +	int i;
> +	bool should_compress;
> +	atomic_t *p;
> +
>  
>  	clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, EXTENT_LOCKED,
>  			 1, 0, NULL);

In case of error I wonder shouldn't the cleared range really be within
page_Start-page_end. Because subsequent pages which are being written
back could follow this whole region?

> -	while (start < end) {
> -		async_cow = kmalloc(sizeof(*async_cow), GFP_NOFS);
> -		BUG_ON(!async_cow); /* -ENOMEM */
> -		/*
> -		 * igrab is called higher up in the call chain, take only the
> -		 * lightweight reference for the callback lifetime
> -		 */
> -		ihold(inode);
> -		async_cow->inode = inode;
> -		async_cow->fs_info = fs_info;
> -		async_cow->locked_page = locked_page;
> -		async_cow->start = start;
> -		async_cow->write_flags = write_flags;
> -
> -		if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
> -		    !btrfs_test_opt(fs_info, FORCE_COMPRESS))
> -			cur_end = end;
> -		else
> -			cur_end = min(end, start + SZ_512K - 1);
>  
> -		async_cow->end = cur_end;
> -		INIT_LIST_HEAD(&async_cow->extents);
> +	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
> +	    !btrfs_test_opt(fs_info, FORCE_COMPRESS)) {
> +		num_chunks = 1;
> +		should_compress = false;
> +	} else {
> +		should_compress = true;
> +	}
> +
> +	/* Layout is [atomic_t][async_cow1][async_cowN].... */
> +	async_cow = kmalloc(sizeof(atomic_t) + num_chunks*sizeof(*async_cow),
> +			    GFP_NOFS);
> +	if (!async_cow)
> +		return -ENOMEM;
> +
> +	p = (atomic_t *)async_cow;
> +	async_cow = (struct async_cow *)((char *)async_cow + sizeof(atomic_t));
> +	atomic_set(p, num_chunks);
> +
> +	for (i = 0; i < num_chunks; i++) {
>  
> -		btrfs_init_work(&async_cow->work,
> +		if (should_compress)
> +			cur_end = min(end, start + SZ_512K - 1);
> +		else
> +			cur_end = end;
> +
> +		ihold(inode);
> +		async_cow[i].pending= p;
> +		async_cow[i].inode = inode;
> +		async_cow[i].start = start;
> +		async_cow[i].end = cur_end;
> +		async_cow[i].fs_info = fs_info;
> +		async_cow[i].locked_page = locked_page;
> +		async_cow[i].write_flags = write_flags;
> +		INIT_LIST_HEAD(&async_cow[i].extents);
> +
> +		btrfs_init_work(&async_cow[i].work,
>  				btrfs_delalloc_helper,
>  				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);
> +		btrfs_queue_work(fs_info->delalloc_workers, &async_cow[i].work);
>  
>  		*nr_written += nr_pages;
>  		start = cur_end + 1;
> 

[PATCH 1/4] btrfs: Refactor cow_file_range_async

[Nikolay Borisov]

This commit changes the implementation of cow_file_range_async in order
to get rid of the BUG_ON in the middle of the loop. Additionally it
reworks the inner loop in the hopes of making it more understandable.

Main change is that the number of chunks required to handle the given
range is calculated before going into the loop and the logic of the loop
just iterates the chunk count. Furthermore, the way memory is allocated
is reworked and now the code does a single kmalloc with enough space to
handle all chunks. Depending on whether compression is enabled or not
chunks are either 1 (in non-compress case) or the range divided by 512k.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 05bbfd02ea49..c2180f0a5f42 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -375,6 +375,7 @@ struct async_cow {
 	unsigned int write_flags;
 	struct list_head extents;
 	struct btrfs_work work;
+	atomic_t *pending;
 };
 
 static noinline int add_async_extent(struct async_cow *cow,
@@ -1181,7 +1182,12 @@ static noinline void async_cow_free(struct btrfs_work *work)
 	async_cow = container_of(work, struct async_cow, work);
 	if (async_cow->inode)
 		btrfs_add_delayed_iput(async_cow->inode);
-	kfree(async_cow);
+	/*
+	 * Since the pointer to 'pending' is at the beginning of the array of
+	 * async_cow's, freeing it ensures the whole array has been freed.
+	 */
+	if (atomic_dec_and_test(async_cow->pending))
+		kfree(async_cow->pending);
 }
 
 static int cow_file_range_async(struct inode *inode, struct page *locked_page,
@@ -1193,42 +1199,59 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 	struct async_cow *async_cow;
 	unsigned long nr_pages;
 	u64 cur_end;
+	u64 num_chunks = DIV_ROUND_UP(end - start, SZ_512K);
+	int i;
+	bool should_compress;
+	atomic_t *p;
+
 
 	clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, EXTENT_LOCKED,
 			 1, 0, NULL);
-	while (start < end) {
-		async_cow = kmalloc(sizeof(*async_cow), GFP_NOFS);
-		BUG_ON(!async_cow); /* -ENOMEM */
-		/*
-		 * igrab is called higher up in the call chain, take only the
-		 * lightweight reference for the callback lifetime
-		 */
-		ihold(inode);
-		async_cow->inode = inode;
-		async_cow->fs_info = fs_info;
-		async_cow->locked_page = locked_page;
-		async_cow->start = start;
-		async_cow->write_flags = write_flags;
-
-		if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
-		    !btrfs_test_opt(fs_info, FORCE_COMPRESS))
-			cur_end = end;
-		else
-			cur_end = min(end, start + SZ_512K - 1);
 
-		async_cow->end = cur_end;
-		INIT_LIST_HEAD(&async_cow->extents);
+	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
+	    !btrfs_test_opt(fs_info, FORCE_COMPRESS)) {
+		num_chunks = 1;
+		should_compress = false;
+	} else {
+		should_compress = true;
+	}
+
+	/* Layout is [atomic_t][async_cow1][async_cowN].... */
+	async_cow = kmalloc(sizeof(atomic_t) + num_chunks*sizeof(*async_cow),
+			    GFP_NOFS);
+	if (!async_cow)
+		return -ENOMEM;
+
+	p = (atomic_t *)async_cow;
+	async_cow = (struct async_cow *)((char *)async_cow + sizeof(atomic_t));
+	atomic_set(p, num_chunks);
+
+	for (i = 0; i < num_chunks; i++) {
 
-		btrfs_init_work(&async_cow->work,
+		if (should_compress)
+			cur_end = min(end, start + SZ_512K - 1);
+		else
+			cur_end = end;
+
+		ihold(inode);
+		async_cow[i].pending= p;
+		async_cow[i].inode = inode;
+		async_cow[i].start = start;
+		async_cow[i].end = cur_end;
+		async_cow[i].fs_info = fs_info;
+		async_cow[i].locked_page = locked_page;
+		async_cow[i].write_flags = write_flags;
+		INIT_LIST_HEAD(&async_cow[i].extents);
+
+		btrfs_init_work(&async_cow[i].work,
 				btrfs_delalloc_helper,
 				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);
+		btrfs_queue_work(fs_info->delalloc_workers, &async_cow[i].work);
 
 		*nr_written += nr_pages;
 		start = cur_end + 1;
-- 
2.17.1