Re: [PATCH 1/7] btrfs: Preallocate chunks in cow_file_range_async

[David Sterba <dsterba@suse.cz>]

On Thu, Mar 28, 2019 at 02:49:30PM +0200, Nikolay Borisov wrote:
> 
> 
> On 27.03.19 г. 19:23 ч., David Sterba wrote:
> > On Tue, Mar 12, 2019 at 05:20:24PM +0200, Nikolay Borisov wrote:
> >> @@ -1190,45 +1201,71 @@ 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_chunk;
> >>  	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 */
> >> +
> >> +	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(struct_size(ctx, chunks, num_chunks), GFP_NOFS);
> > 
> > This leads to OOM due to high order allocation. And this is worse than
> > the previous state, where there are many small allocation that could
> > potentially fail (but most likely will not due to GFP_NOSF and size <
> > PAGE_SIZE).
> > 
> > So this needs to be reworked to avoid the costly allocations or reverted
> > to the previous state.
> 
> Right, makes sense. In order to have a simplified submission logic I
> think to rework the allocation to have a loop that allocates a single
> item for every chunk or alternatively switch to using kvmalloc? I think
> the fact that vmalloced memory might not be contiguous is not critical
> for the metadata structures in this case?

kvmalloc would work here, though there is some overhead involved
compared to bare kmalloc (the mappings). As the call happens on writeout
path, I'd be cautious about unnecessary overhead.

If looping over the range works, then we can allocate the largest size
that does not require kvmalloc (PAGE_ALLOC_COSTLY_ORDER) and then reuse
it for each iteration.