Re: [PATCH v2] Btrfs: use generic_remap_file_range_prep() for cloning and deduplication

[Nikolay Borisov <nborisov@suse.com>]

On 7.12.18 г. 17:25 ч., fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Since cloning and deduplication are no longer Btrfs specific operations, we
> now have generic code to handle parameter validation, compare file ranges
> used for deduplication, clear capabilities when cloning, etc. This change
> makes Btrfs use it, eliminating a lot of code in Btrfs and also fixing a
> few bugs, such as:
> 
> 1) When cloning, the destination file's capabilities were not dropped
>    (the fstest generic/513 tests this);
> 
> 2) We were not checking if the destination file is immutable;
> 
> 3) Not checking if either the source or destination files are swap
>    files (swap file support is coming soon for Btrfs);
> 
> 4) System limits were not checked (resource limits and O_LARGEFILE).
> 
> Note that the generic helper generic_remap_file_range_prep() does start
> and waits for writeback by calling filemap_write_and_wait_range(), however
> that is not enough for Btrfs for two reasons:
> 
> 1) With compression, we need to start writeback twice in order to get the
>    pages marked for writeback and ordered extents created;
> 
> 2) filemap_write_and_wait_range() (and all its other variants) only waits
>    for the IO to complete, but we need to wait for the ordered extents to
>    finish, so that when we do the actual reflinking operations the file
>    extent items are in the fs tree. This is also important due to the fact
>    that the generic helper, for the deduplication case, compares the
>    contents of the pages in the requested range, which might require
>    reading extents from disk in the very unlikely case that pages get
>    invalidated after writeback finishes (so the file extent items must be
>    up to date in the fs tree).
> 
> Since these reasons are specific to Btrfs we have to do it in the Btrfs
> code before calling generic_remap_file_range_prep(). This also results in
> a more simple way of dealing with existing delalloc in the source/target
> ranges, specially for the deduplication case where we used to lock all the
> pages first and then if we found any dealloc for the range, or ordered
> extent, we would unlock the pages trigger writeback and wait for ordered
> extents to complete, then lock all the pages again and check if
> deduplication can be done. So now we get a simpler approach: lock the
> inodes, then trigger writeback and then wait for ordered extents to
> complete.
> 
> So make btrfs use generic_remap_file_range_prep() (XFS and OCFS2 use it)
> to eliminate duplicated code, fix a few bugs and benefit from future bug
> fixes done there - for example the recent clone and dedupe bugs involving
> reflinking a partial EOF block got a counterpart fix in the generic helpe,
> since it affected all filesystems supporting these operations, so we no
> longer need special checks in Btrfs for them.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Two minor suggestions but other LGTM so:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> V2: Removed check that verifies if either of the inodes is a directory,
>     as it is done by generic_remap_file_range_prep(). Oddly in btrfs was being
>     done only for cloning but not for dedupe.
> 
>  fs/btrfs/ioctl.c | 612 ++++++++++++-------------------------------------------
>  1 file changed, 129 insertions(+), 483 deletions(-)
> 

<snip>

> @@ -4213,11 +3854,9 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
>  	struct inode *inode = file_inode(file);
>  	struct inode *src = file_inode(file_src);
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);

nit: This variable can be removed  and btrfs_sb...

> -	struct btrfs_root *root = BTRFS_I(inode)->root;
>  	int ret;
>  	u64 len = olen;
>  	u64 bs = fs_info->sb->s_blocksize;

can be used directly here, since we only care about the blocksize, saves
sizeof(void *) worth of bytes on the stack.

> -	int same_inode = src == inode;
>  
>  	/*
>  	 * TODO:
> @@ -4230,101 +3869,32 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
>  	 *   be either compressed or non-compressed.
>  	 */
>  
> -	if (btrfs_root_readonly(root))
> -		return -EROFS;
> -
> -	if (file_src->f_path.mnt != file->f_path.mnt ||
> -	    src->i_sb != inode->i_sb)
> -		return -EXDEV;
> -
> -	if (S_ISDIR(src->i_mode) || S_ISDIR(inode->i_mode))
> -		return -EISDIR;
> -
> -	if (!same_inode) {
> -		btrfs_double_inode_lock(src, inode);
> -	} else {
> -		inode_lock(src);
> -	}
> -
>  	/* don't make the dst file partly checksummed */
>  	if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) !=
> -	    (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) {
> -		ret = -EINVAL;
> -		goto out_unlock;
> -	}
> +	    (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM))
> +		return -EINVAL;

This common check between btrfs_extent_same and btrfs_clone_files can be
factored out to btrfs_remap_file_range_prep or btrfs_remap_file_range

>  
> -	/* determine range to clone */
> -	ret = -EINVAL;
> -	if (off + len > src->i_size || off + len < off)
> -		goto out_unlock;
> -	if (len == 0)
> -		olen = len = src->i_size - off;

<snip>
>  }
>  
>