Re: [PATCH 2/2] Btrfs: make deduplication with range including the last block work

[Zygo Blaxell <ce3g8jdj@umail.furryterror.org>]

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

On Mon, Dec 16, 2019 at 06:26:56PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Since btrfs was migrated to use the generic VFS helpers for clone and
> deduplication, it stopped allowing for the last block of a file to be
> deduplicated when the source file size is not sector size aligned (when
> eof is somewhere in the middle of the last block). There are two reasons
> for that:
> 
> 1) The generic code always rounds down, to a multiple of the block size,
>    the range's length for deduplications. This means we end up never
>    deduplicating the last block when the eof is not block size aligned,
>    even for the safe case where the destination range's end offset matches
>    the destination file's size. That rounding down operation is done at
>    generic_remap_check_len();
> 
> 2) Because of that, the btrfs specific code does not expect anymore any
>    non-aligned range length's for deduplication and therefore does not
>    work if such nona-aligned length is given.
> 
> This patch addresses that second part, and it depends on a patch that
> fixes generic_remap_check_len(), in the VFS, which was submitted ealier
> and has the following subject:
> 
>   "fs: allow deduplication of eof block into the end of the destination file"
> 
> These two patches address reports from users that started seeing lower
> deduplication rates due to the last block never being deduplicated when
> the file size is not aligned to the filesystem's block size.
> 
> Link: https://lore.kernel.org/linux-btrfs/2019-1576167349.500456@svIo.N5dq.dFFD/
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Should these patches be marked for stable (5.0+, but see below for
caveats about 5.0)?  The bug affects 5.3 and 5.4 which are still active,
and dedupe is an important feature for some users.

> ---
>  fs/btrfs/ioctl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 3418decb9e61..c41c276ff272 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3237,6 +3237,7 @@ static void btrfs_double_extent_lock(struct inode *inode1, u64 loff1,
>  static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 len,
>  				   struct inode *dst, u64 dst_loff)
>  {
> +	const u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
>  	int ret;
>  
>  	/*
> @@ -3244,7 +3245,7 @@ static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 len,
>  	 * source range to serialize with relocation.
>  	 */
>  	btrfs_double_extent_lock(src, loff, dst, dst_loff, len);
> -	ret = btrfs_clone(src, dst, loff, len, len, dst_loff, 1);
> +	ret = btrfs_clone(src, dst, loff, len, ALIGN(len, bs), dst_loff, 1);

A heads-up for anyone backporting this to 5.0:  this patch depends on

	57a50e2506df Btrfs: remove no longer needed range length checks for deduplication

Simply resolving the git conflict without including 57a50e2506df produces
a kernel where dedupe rounds the size of the dst file up to the next
block boundary.  This is because 57a50e2506df changes the value of
"len".  Before 57a50e2506df, "len" is equivalent to "ALIGN(len, bs)"
at the btrfs_clone line; after 57a50e2506df, "len" is the unaligned
dedupe request length passed to the btrfs_extent_same_range function.
This changes the semantics of the btrfs_clone line significantly.

57a50e2506df is in 5.1, so 5.1+ kernels do not require any additional
patches.

4.20 and earlier don't have the bug, so don't need a fix.

>  	btrfs_double_extent_unlock(src, loff, dst, dst_loff, len);
>  
>  	return ret;
> -- 
> 2.11.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]