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

[Johannes Thumshirn <jthumshirn@suse.de>]

On 20/02/2019 16:11, 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 | 84 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 58 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 05bbfd02ea49..546000779310 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);
>  }

[...]

> +	/* Layout is [atomic_t][async_cow1][async_cowN].... */
> +	async_cow = kmalloc(sizeof(atomic_t) + num_chunks*sizeof(*async_cow),
> +			    GFP_NOFS);
> +	if (!async_cow) {
> +		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;
> +	}
> +
> +	p = (atomic_t *)async_cow;
> +	async_cow = (struct async_cow *)((char *)async_cow + sizeof(atomic_t));
> +	atomic_set(p, num_chunks);

Ugh! You're abusing an atomic_t * for something you want to actually
make this:

@@ -375,6 +375,7 @@ struct async_cow {
 	unsigned int write_flags;
 	struct list_head extents;
 	struct btrfs_work work;
+	atomic_t pending;
+	struct async_cow list[0];
};

and then have:

tmp = krealloc(async_cow, num_chunks * sizeof(*async_cow), GFP_NOFS);
if (!tmp) {
[...]
}
kfree(async_cow);
async_cow = tmp;


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