Re: [PATCH] Btrfs: fix performance regression of writing to prealloc/nocow file

[Roman Mamedov <rm@romanrm.net>]

[-- Attachment #1: Type: text/plain, Size: 2922 bytes --]

On Thu, 17 Mar 2016 15:16:15 -0700
Liu Bo <bo.li.liu@oracle.com> wrote:

> For nocow/prealloc files, we try our best to not allocate space, however,
> this ends up a huge performance regression since it's expensive to check
> if data is shared.
> 
> Let's go back to only go check shared data once we're not able to allocate
> space.

As mentioned by Holger there's another patch modifying the same function:
https://git.kernel.org/cgit/linux/kernel/git/mason/linux-btrfs.git/commit/fs/btrfs?h=integration-4.6&id=4da2e26a2a32b174878744bd0f07db180c875f26
and that one fixes a serious regression in the 4.4 series.
I did not try yours, but could you please ensure that you do not hit the
described problem with your version of the patch, or perhaps even better,
consider re-testing performance and then basing any further performance
optimizations upon a state with no known grave functionality bugs (i.e. with
the above patch applied).

Thanks

> The test was made against a tmpfs backed loop device:
> 
> xfs_io -f -c "falloc 0 400M" foobar
> xfs_io -c "pwrite -W -b 4096 0 400M" foobar
> 
> Vanilla:
> wrote 419430400/419430400 bytes at offset 0
> 400 MiB, 102400 ops; 0:00:01.00 (200.015 MiB/sec and 51203.8403 ops/sec)
> 
> Patched kernel:
> wrote 419430400/419430400 bytes at offset 0
> 400 MiB, 102400 ops; 0:00:01.00 (346.137 MiB/sec and 88610.9796 ops/sec)
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/file.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 098bb8f..f66c1bc 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1525,16 +1525,12 @@ static noinline ssize_t __btrfs_buffered_write(struct file *file,
>  
>  		reserve_bytes = num_pages << PAGE_CACHE_SHIFT;
>  
> -		if (BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
> -					     BTRFS_INODE_PREALLOC)) {
> +		ret = btrfs_check_data_free_space(inode, pos, write_bytes);
> +		if (ret == -ENOSPC &&
> +		    BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
> +						     BTRFS_INODE_PREALLOC)) {
>  			ret = check_can_nocow(inode, pos, &write_bytes);
> -			if (ret < 0)
> -				break;
>  			if (ret > 0) {
> -				/*
> -				 * For nodata cow case, no need to reserve
> -				 * data space.
> -				 */
>  				only_release_metadata = true;
>  				/*
>  				 * our prealloc extent may be smaller than
> @@ -1543,10 +1539,13 @@ static noinline ssize_t __btrfs_buffered_write(struct file *file,
>  				num_pages = DIV_ROUND_UP(write_bytes + offset,
>  							 PAGE_CACHE_SIZE);
>  				reserve_bytes = num_pages << PAGE_CACHE_SHIFT;
> +
> +				ret = 0;
>  				goto reserve_metadata;
> +			} else {
> +				ret = -ENOSPC;
>  			}
>  		}
> -		ret = btrfs_check_data_free_space(inode, pos, write_bytes);
>  		if (ret < 0)
>  			break;
>  


-- 
With respect,
Roman

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]