Re: [PATCH 09/16] btrfs: lock/unlock extents while creation/end of async_chunk

[Josef Bacik <josef@toxicpanda.com>]

On Tue, Nov 15, 2022 at 12:00:27PM -0600, Goldwyn Rodrigues wrote:
> writepages() writebacks the unwritten pages the synchronously. So we
> know that once writepages returns, the pages are "done" and can be
> safely unlocked. However, with async writes, this is done over a thread.
> So, for async writeback, perform this within the async thread.
> 
> Locking is performed at origin of async_chunk and unlocked when
> async_chunk completes.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/inode.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 92726831dd5d..aa393219019b 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -528,6 +528,7 @@ struct async_chunk {
>  	struct list_head extents;
>  	struct cgroup_subsys_state *blkcg_css;
>  	struct btrfs_work work;
> +	struct extent_state *cached_state;

I'd rather this be separated into it's own patch, I definitely got confused for
a second.

>  	struct async_cow *async_cow;
>  };
>  
> @@ -1491,6 +1492,9 @@ static noinline void async_cow_start(struct btrfs_work *work)
>  
>  	compressed_extents = compress_file_range(async_chunk);
>  	if (compressed_extents == 0) {
> +		unlock_extent(&async_chunk->inode->io_tree,
> +				async_chunk->start, async_chunk->end,
> +				&async_chunk->cached_state);
>  		btrfs_add_delayed_iput(async_chunk->inode);
>  		async_chunk->inode = NULL;
>  	}
> @@ -1530,11 +1534,16 @@ static noinline void async_cow_free(struct btrfs_work *work)
>  	struct async_cow *async_cow;
>  
>  	async_chunk = container_of(work, struct async_chunk, work);
> -	if (async_chunk->inode)
> +	if (async_chunk->inode) {
> +		unlock_extent(&async_chunk->inode->io_tree,
> +				async_chunk->start, async_chunk->end,
> +				&async_chunk->cached_state);
>  		btrfs_add_delayed_iput(async_chunk->inode);
> +	}
>  	if (async_chunk->blkcg_css)
>  		css_put(async_chunk->blkcg_css);
>  
> +

Extra whitespace.

>  	async_cow = async_chunk->async_cow;
>  	if (atomic_dec_and_test(&async_cow->num_chunks))
>  		kvfree(async_cow);
> @@ -1558,7 +1567,6 @@ static int cow_file_range_async(struct btrfs_inode *inode,
>  	unsigned nofs_flag;
>  	const blk_opf_t write_flags = wbc_to_write_flags(wbc);
>  
> -	unlock_extent(&inode->io_tree, start, end, NULL);
>  
>  	if (inode->flags & BTRFS_INODE_NOCOMPRESS &&
>  	    !btrfs_test_opt(fs_info, FORCE_COMPRESS)) {
> @@ -1600,6 +1608,9 @@ static int cow_file_range_async(struct btrfs_inode *inode,
>  		ihold(&inode->vfs_inode);
>  		async_chunk[i].async_cow = ctx;
>  		async_chunk[i].inode = inode;
> +		async_chunk[i].cached_state = NULL;
> +		btrfs_lock_and_flush_ordered_range(inode, start, cur_end,
> +				&async_chunk[i].cached_state);
>  		async_chunk[i].start = start;
>  		async_chunk[i].end = cur_end;
>  		async_chunk[i].write_flags = write_flags;
> @@ -8222,10 +8233,11 @@ static int btrfs_writepages(struct address_space *mapping,
>  			    struct writeback_control *wbc)
>  {
>  	u64 start, end;
> -	struct inode *inode = mapping->host;
> +	struct btrfs_inode *inode = BTRFS_I(mapping->host);
>  	struct extent_state *cached = NULL;
> +	bool async_wb;
>  	int ret;
> -	u64 isize = round_up(i_size_read(inode), PAGE_SIZE) - 1;
> +	u64 isize = round_up(i_size_read(&inode->vfs_inode), PAGE_SIZE) - 1;
>  
>  	if (wbc->range_cyclic) {
>  		start = mapping->writeback_index << PAGE_SHIFT;
> @@ -8239,9 +8251,18 @@ static int btrfs_writepages(struct address_space *mapping,
>  	if (start >= end)
>  		return 0;
>  
> -	lock_extent(&BTRFS_I(inode)->io_tree, start, end, &cached);
> +	/*
> +	 * For async I/O, locking and unlocking is performed with the
> +	 * allocation and completion of async_chunk.
> +	 */
> +	async_wb = btrfs_inode_can_compress(inode) &&
> +		   inode_need_compress(inode, start, end);

These can change their answer arbitrarily and randomly, which means we could end
up doing an async extent when we didn't think we would, and then unpleasantness
happens.  Thanks,

Josef