[PATCH v3 0/6] Compress path cleanups

[PATCH v3 0/6] Compress path cleanups

[Nikolay Borisov]

Another day, another revision, hopefully this is the last one. 

 Changes since v2: 

 * Reworked patch 1 and made async_cow be the top-level context struct and each
 chunk is tracked by async_chunk. This cleansup the pointer gymnastics I was 
 doing in the previous version (Johannes)
 
 * Reworded changelogs in various patches to reflect the usage of async_chunk.

 * Fixed changelog in patch2 to properly reflect fs_info is referenced from 
 btrfs_work (Johannes)
  
 * Added Reviwed-by to patches 4/5/6

 Changes since v1:

 * Fixed error handling in patch 1 - now properly cleanup on failure.
 * Fixed subject of patch 3
 * Added patches 4/5 as minor cleanups to the code.

Nikolay Borisov (6):
  btrfs: Refactor cow_file_range_async
  btrfs: Remove fs_info from struct async_chunk
  btrfs: Make compress_file_range take only struct async_chunk
  btrfs: Replace clear_extent_bit with unlock_extent
  btrfs: Set iotree only once in submit_compressed_extents
  btrfs: Factor out common extent locking code in
    submit_compressed_extents

 fs/btrfs/inode.c | 148 +++++++++++++++++++++++++++--------------------
 1 file changed, 84 insertions(+), 64 deletions(-)

-- 
2.17.1

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

The idea is to make async_cow be a top-level structured, shared
amongst all chunks being sent for compression. This allows to perform
one memory allocation at the beginning and gracefully fail the IO if
there isn't enough memory. Now, each chunk is going to be described
by an async_chunk struct. It's the responsibility of the final chunk
to actually free the memory.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 05bbfd02ea49..0f82ea348164 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -366,7 +366,7 @@ struct async_extent {
 	struct list_head list;
 };
 
-struct async_cow {
+struct async_chunk {
 	struct inode *inode;
 	struct btrfs_fs_info *fs_info;
 	struct page *locked_page;
@@ -375,9 +375,15 @@ struct async_cow {
 	unsigned int write_flags;
 	struct list_head extents;
 	struct btrfs_work work;
+	atomic_t *pending;
+};
+
+struct async_cow {
+	atomic_t num_chunks; /* Number of chunks in flight */
+	struct async_chunk chunks[];
 };
 
-static noinline int add_async_extent(struct async_cow *cow,
+static noinline int add_async_extent(struct async_chunk *cow,
 				     u64 start, u64 ram_size,
 				     u64 compressed_size,
 				     struct page **pages,
@@ -447,7 +453,7 @@ static inline void inode_should_defrag(struct btrfs_inode *inode,
 static noinline void compress_file_range(struct inode *inode,
 					struct page *locked_page,
 					u64 start, u64 end,
-					struct async_cow *async_cow,
+					struct async_chunk *async_cow,
 					int *num_added)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
@@ -713,7 +719,7 @@ 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 async_cow *async_cow)
+static noinline void submit_compressed_extents(struct async_chunk *async_cow)
 {
 	struct inode *inode = async_cow->inode;
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
@@ -1132,9 +1138,9 @@ static noinline int cow_file_range(struct inode *inode,
  */
 static noinline void async_cow_start(struct btrfs_work *work)
 {
-	struct async_cow *async_cow;
+	struct async_chunk *async_cow;
 	int num_added = 0;
-	async_cow = container_of(work, struct async_cow, work);
+	async_cow = container_of(work, struct async_chunk, work);
 
 	compress_file_range(async_cow->inode, async_cow->locked_page,
 			    async_cow->start, async_cow->end, async_cow,
@@ -1151,10 +1157,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_chunk *async_cow;
 	unsigned long nr_pages;
 
-	async_cow = container_of(work, struct async_cow, work);
+	async_cow = container_of(work, struct async_chunk, work);
 
 	fs_info = async_cow->fs_info;
 	nr_pages = (async_cow->end - async_cow->start + PAGE_SIZE) >>
@@ -1177,11 +1183,16 @@ static noinline void async_cow_submit(struct btrfs_work *work)
 
 static noinline void async_cow_free(struct btrfs_work *work)
 {
-	struct async_cow *async_cow;
-	async_cow = container_of(work, struct async_cow, work);
+	struct async_chunk *async_cow;
+	async_cow = container_of(work, struct async_chunk, 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,
@@ -1190,45 +1201,68 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 				unsigned int write_flags)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-	struct async_cow *async_cow;
+	struct async_cow *ctx;
+	struct async_chunk *async_cow;
 	unsigned long nr_pages;
 	u64 cur_end;
+	u64 num_chunks = DIV_ROUND_UP(end - start, SZ_512K);
+	int i;
+	bool should_compress;
 
 	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;
+	}
+
+	ctx = kmalloc(sizeof(struct async_cow) + num_chunks*sizeof(*async_cow),
+		      GFP_NOFS);
+	if (!ctx) {
+		unsigned clear_bits = EXTENT_LOCKED | EXTENT_DELALLOC |
+			EXTENT_DELALLOC_NEW | EXTENT_DEFRAG |
+			EXTENT_DO_ACCOUNTING;
+		unsigned long page_ops = PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
+			PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK |
+			PAGE_SET_ERROR;
+		extent_clear_unlock_delalloc(inode, start, end, 0, locked_page,
+					     clear_bits, page_ops);
+		return -ENOMEM;
+	}
+
+	async_cow = ctx->chunks;
+	atomic_set(&ctx->num_chunks, 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= &ctx->num_chunks;
+		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 v3 2/6] btrfs: Remove fs_info from struct async_chunk

[Nikolay Borisov]

The associated btrfs_work already contains a reference to the fs_info so
use that instead of passing it via async_chunk. No functional changes.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0f82ea348164..d61dd538d2b4 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -368,7 +368,6 @@ struct async_extent {
 
 struct async_chunk {
 	struct inode *inode;
-	struct btrfs_fs_info *fs_info;
 	struct page *locked_page;
 	u64 start;
 	u64 end;
@@ -1156,13 +1155,11 @@ 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_chunk *async_cow;
+	struct async_chunk *async_cow = container_of(work, struct async_chunk,
+						     work);
+	struct btrfs_fs_info *fs_info = btrfs_work_owner(work);
 	unsigned long nr_pages;
 
-	async_cow = container_of(work, struct async_chunk, work);
-
-	fs_info = async_cow->fs_info;
 	nr_pages = (async_cow->end - async_cow->start + PAGE_SIZE) >>
 		PAGE_SHIFT;
 
@@ -1249,7 +1246,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 v3 3/6] btrfs: Make compress_file_range take only struct async_chunk

[Nikolay Borisov]

All context this function needs is held within struct async_chunk.
Currently we not only pass the struct but also every individual member.
This is redundant, simplify it by only passing struct async_chunk 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 d61dd538d2b4..d566e15f8c58 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -449,14 +449,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_chunk *async_cow,
-					int *num_added)
+static noinline void compress_file_range(struct async_chunk *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;
@@ -675,9 +675,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)
@@ -1141,9 +1141,7 @@ static noinline void async_cow_start(struct btrfs_work *work)
 	int num_added = 0;
 	async_cow = container_of(work, struct async_chunk, 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 v3 4/6] btrfs: Replace clear_extent_bit with unlock_extent

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 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 d566e15f8c58..c857f3c2bfc4 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1204,8 +1204,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 	int i;
 	bool should_compress;
 
-	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 v3 5/6] btrfs: Set iotree only once in submit_compressed_extents

[Nikolay Borisov]

The inode never changes so it's sufficient to dereference it and get
the iotree only once, before the execution of the main loop. No
functional changes, only the size of the function is decreased:

add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-44 (-44)
Function                                     old     new   delta
submit_compressed_extents                   1240    1196     -44
Total: Before=88476, After=88432, chg -0.05%

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/inode.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c857f3c2bfc4..bfd0e934a098 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -727,7 +727,7 @@ static noinline void submit_compressed_extents(struct async_chunk *async_cow)
 	struct btrfs_key ins;
 	struct extent_map *em;
 	struct btrfs_root *root = BTRFS_I(inode)->root;
-	struct extent_io_tree *io_tree;
+	struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
 	int ret = 0;
 
 again:
@@ -735,9 +735,6 @@ static noinline void submit_compressed_extents(struct async_chunk *async_cow)
 		async_extent = list_entry(async_cow->extents.next,
 					  struct async_extent, list);
 		list_del(&async_extent->list);
-
-		io_tree = &BTRFS_I(inode)->io_tree;
-
 retry:
 		/* did the compression code fall back to uncompressed IO? */
 		if (!async_extent->pages) {
-- 
2.17.1

[PATCH v3 6/6] btrfs: Factor out common extent locking code in submit_compressed_extents

[Nikolay Borisov]

Irrespective of whether the compress code fell back to uncompressed or
a compressed extent has to be submitted, the extent range is always
locked. So factor out the common lock_extent call at the beginning of
the loop. No functional changes just removes one duplicate lock_extent
call.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/inode.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index bfd0e934a098..edeb9da6b27e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -736,15 +736,14 @@ static noinline void submit_compressed_extents(struct async_chunk *async_cow)
 					  struct async_extent, list);
 		list_del(&async_extent->list);
 retry:
+
+		lock_extent(io_tree, async_extent->start,
+			    async_extent->start + async_extent->ram_size - 1);
 		/* did the compression code fall back to uncompressed IO? */
 		if (!async_extent->pages) {
 			int page_started = 0;
 			unsigned long nr_written = 0;
 
-			lock_extent(io_tree, async_extent->start,
-					 async_extent->start +
-					 async_extent->ram_size - 1);
-
 			/* allocate blocks */
 			ret = cow_file_range(inode, async_cow->locked_page,
 					     async_extent->start,
@@ -776,9 +775,6 @@ static noinline void submit_compressed_extents(struct async_chunk *async_cow)
 			continue;
 		}
 
-		lock_extent(io_tree, async_extent->start,
-			    async_extent->start + async_extent->ram_size - 1);
-
 		ret = btrfs_reserve_extent(root, async_extent->ram_size,
 					   async_extent->compressed_size,
 					   async_extent->compressed_size,
-- 
2.17.1

Re: [PATCH v3 3/6] btrfs: Make compress_file_range take only struct async_chunk

[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 v3 2/6] btrfs: Remove fs_info from struct async_chunk

[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 v3 1/6] btrfs: Refactor cow_file_range_async

[Johannes Thumshirn]

On 21/02/2019 12:57, Nikolay Borisov wrote:
>  
>  static int cow_file_range_async(struct inode *inode, struct page *locked_page,
> @@ -1190,45 +1201,68 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>  				unsigned int write_flags)
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> -	struct async_cow *async_cow;
> +	struct async_cow *ctx;
> +	struct async_chunk *async_cow;

In case you have to re-send the patch you could maybe rename the
async_cow variable to async_chunk or sth like that. Would make the
resulting code a little bit clearer but no strong opinions here.

Anyways,
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 v3 1/6] btrfs: Refactor cow_file_range_async

[Nikolay Borisov]

On 21.02.19 г. 15:15 ч., Johannes Thumshirn wrote:
> On 21/02/2019 12:57, Nikolay Borisov wrote:
>>  
>>  static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>> @@ -1190,45 +1201,68 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>>  				unsigned int write_flags)
>>  {
>>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>> -	struct async_cow *async_cow;
>> +	struct async_cow *ctx;
>> +	struct async_chunk *async_cow;
> 
> In case you have to re-send the patch you could maybe rename the
> async_cow variable to async_chunk or sth like that. Would make the
> resulting code a little bit clearer but no strong opinions here.

The reason I left it like that is to minimize the resulting diff.

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

Re: [PATCH v3 1/6] btrfs: Refactor cow_file_range_async

[Johannes Thumshirn]

On 21/02/2019 14:25, Nikolay Borisov wrote:
> 
> 
> On 21.02.19 г. 15:15 ч., Johannes Thumshirn wrote:
>> On 21/02/2019 12:57, Nikolay Borisov wrote:
>>>  
>>>  static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>>> @@ -1190,45 +1201,68 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>>>  				unsigned int write_flags)
>>>  {
>>>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>>> -	struct async_cow *async_cow;
>>> +	struct async_cow *ctx;
>>> +	struct async_chunk *async_cow;
>>
>> In case you have to re-send the patch you could maybe rename the
>> async_cow variable to async_chunk or sth like that. Would make the
>> resulting code a little bit clearer but no strong opinions here.
> 
> The reason I left it like that is to minimize the resulting diff.

Understood, but now we have a 'ctx' of type 'struct async_cow' and a
'*async_cow' of type 'struct asyn_chunk'. This is the type of code that
will make people go WTF when they read it in 3-4 cycles.


-- 
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 v3 1/6] btrfs: Refactor cow_file_range_async

[Nikolay Borisov]

On 21.02.19 г. 17:07 ч., Johannes Thumshirn wrote:
> On 21/02/2019 14:25, Nikolay Borisov wrote:
>>
>>
>> On 21.02.19 г. 15:15 ч., Johannes Thumshirn wrote:
>>> On 21/02/2019 12:57, Nikolay Borisov wrote:
>>>>  
>>>>  static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>>>> @@ -1190,45 +1201,68 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>>>>  				unsigned int write_flags)
>>>>  {
>>>>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>>>> -	struct async_cow *async_cow;
>>>> +	struct async_cow *ctx;
>>>> +	struct async_chunk *async_cow;
>>>
>>> In case you have to re-send the patch you could maybe rename the
>>> async_cow variable to async_chunk or sth like that. Would make the
>>> resulting code a little bit clearer but no strong opinions here.
>>
>> The reason I left it like that is to minimize the resulting diff.
> 
> Understood, but now we have a 'ctx' of type 'struct async_cow' and a
> '*async_cow' of type 'struct asyn_chunk'. This is the type of code that
> will make people go WTF when they read it in 3-4 cycles.

I also thought of that but then sometimes David has strange tastes re.
code cleanups that add noise. If he is fine with doing the rename in
this patch then I'm happy to do it and resend.

> 
> 

Re: [PATCH v3 1/6] btrfs: Refactor cow_file_range_async

[David Sterba]

On Thu, Feb 21, 2019 at 05:09:01PM +0200, Nikolay Borisov wrote:
> On 21.02.19 г. 17:07 ч., Johannes Thumshirn wrote:
> > On 21/02/2019 14:25, Nikolay Borisov wrote:
> >> On 21.02.19 г. 15:15 ч., Johannes Thumshirn wrote:
> >>> On 21/02/2019 12:57, Nikolay Borisov wrote:
> >>>>  
> >>>>  static int cow_file_range_async(struct inode *inode, struct page *locked_page,
> >>>> @@ -1190,45 +1201,68 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
> >>>>  				unsigned int write_flags)
> >>>>  {
> >>>>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> >>>> -	struct async_cow *async_cow;
> >>>> +	struct async_cow *ctx;
> >>>> +	struct async_chunk *async_cow;
> >>>
> >>> In case you have to re-send the patch you could maybe rename the
> >>> async_cow variable to async_chunk or sth like that. Would make the
> >>> resulting code a little bit clearer but no strong opinions here.
> >>
> >> The reason I left it like that is to minimize the resulting diff.
> > 
> > Understood, but now we have a 'ctx' of type 'struct async_cow' and a
> > '*async_cow' of type 'struct asyn_chunk'. This is the type of code that
> > will make people go WTF when they read it in 3-4 cycles.
> 
> I also thought of that but then sometimes David has strange tastes re.
> code cleanups that add noise. If he is fine with doing the rename in
> this patch then I'm happy to do it and resend.

You must be joking, the naming you chose is quite confusing exactly as
Johannes points out.

I'm not sure what noise patches or changes you mean. There are commits
that just rename something for better readability or free the
type/variable name for another purpose, similar what you do.

Your concern about diff size is ok, though it should result in a
separate patch doing just the rename and another patch using the new
async_cow.

Re: [PATCH v3 1/6] btrfs: Refactor cow_file_range_async

[David Sterba]

On Thu, Feb 21, 2019 at 01:57:12PM +0200, 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.
> 
> The idea is to make async_cow be a top-level structured, shared
> amongst all chunks being sent for compression. This allows to perform
> one memory allocation at the beginning and gracefully fail the IO if
> there isn't enough memory. Now, each chunk is going to be described
> by an async_chunk struct. It's the responsibility of the final chunk
> to actually free the memory.

Right, preallocation is the way to fix it. Please pick a more
descriptive subject, that one is too generic.

> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/inode.c | 108 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 71 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 05bbfd02ea49..0f82ea348164 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -366,7 +366,7 @@ struct async_extent {
>  	struct list_head list;
>  };
>  
> -struct async_cow {
> +struct async_chunk {
>  	struct inode *inode;
>  	struct btrfs_fs_info *fs_info;
>  	struct page *locked_page;
> @@ -375,9 +375,15 @@ struct async_cow {
>  	unsigned int write_flags;
>  	struct list_head extents;
>  	struct btrfs_work work;
> +	atomic_t *pending;
> +};
> +
> +struct async_cow {
> +	atomic_t num_chunks; /* Number of chunks in flight */
> +	struct async_chunk chunks[];
>  };
>  
> -static noinline int add_async_extent(struct async_cow *cow,
> +static noinline int add_async_extent(struct async_chunk *cow,
>  				     u64 start, u64 ram_size,
>  				     u64 compressed_size,
>  				     struct page **pages,
> @@ -447,7 +453,7 @@ static inline void inode_should_defrag(struct btrfs_inode *inode,
>  static noinline void compress_file_range(struct inode *inode,
>  					struct page *locked_page,
>  					u64 start, u64 end,
> -					struct async_cow *async_cow,
> +					struct async_chunk *async_cow,
>  					int *num_added)
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> @@ -713,7 +719,7 @@ 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 async_cow *async_cow)
> +static noinline void submit_compressed_extents(struct async_chunk *async_cow)
>  {
>  	struct inode *inode = async_cow->inode;
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> @@ -1132,9 +1138,9 @@ static noinline int cow_file_range(struct inode *inode,
>   */
>  static noinline void async_cow_start(struct btrfs_work *work)
>  {
> -	struct async_cow *async_cow;
> +	struct async_chunk *async_cow;
>  	int num_added = 0;
> -	async_cow = container_of(work, struct async_cow, work);
> +	async_cow = container_of(work, struct async_chunk, work);
>  
>  	compress_file_range(async_cow->inode, async_cow->locked_page,
>  			    async_cow->start, async_cow->end, async_cow,
> @@ -1151,10 +1157,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_chunk *async_cow;
>  	unsigned long nr_pages;
>  
> -	async_cow = container_of(work, struct async_cow, work);
> +	async_cow = container_of(work, struct async_chunk, work);
>  
>  	fs_info = async_cow->fs_info;
>  	nr_pages = (async_cow->end - async_cow->start + PAGE_SIZE) >>
> @@ -1177,11 +1183,16 @@ static noinline void async_cow_submit(struct btrfs_work *work)
>  
>  static noinline void async_cow_free(struct btrfs_work *work)
>  {
> -	struct async_cow *async_cow;
> -	async_cow = container_of(work, struct async_cow, work);
> +	struct async_chunk *async_cow;
> +	async_cow = container_of(work, struct async_chunk, 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,
> @@ -1190,45 +1201,68 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>  				unsigned int write_flags)
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> -	struct async_cow *async_cow;
> +	struct async_cow *ctx;
> +	struct async_chunk *async_cow;
>  	unsigned long nr_pages;
>  	u64 cur_end;
> +	u64 num_chunks = DIV_ROUND_UP(end - start, SZ_512K);
> +	int i;
> +	bool should_compress;
>  
>  	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;
> +	}
> +
> +	ctx = kmalloc(sizeof(struct async_cow) + num_chunks*sizeof(*async_cow),
> +		      GFP_NOFS);

There's a recently introduced macro for calculating the size of struct
with a fixed header followed by array 'struct_size', please use it.

> +	if (!ctx) {
> +		unsigned clear_bits = EXTENT_LOCKED | EXTENT_DELALLOC |
> +			EXTENT_DELALLOC_NEW | EXTENT_DEFRAG |
> +			EXTENT_DO_ACCOUNTING;
> +		unsigned long page_ops = PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
> +			PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK |
> +			PAGE_SET_ERROR;
> +		extent_clear_unlock_delalloc(inode, start, end, 0, locked_page,
> +					     clear_bits, page_ops);
> +		return -ENOMEM;
> +	}
> +
> +	async_cow = ctx->chunks;
> +	atomic_set(&ctx->num_chunks, 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);

You should preserve the comment, the refactoring does not make it
obsolete or wrong.

> +		async_cow[i].pending= &ctx->num_chunks;
> +		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