On 7.12.18 г. 17:25 ч., fdman...@kernel.org wrote:
> From: Filipe Manana <fdman...@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 <fdman...@suse.com>
Two minor suggestions but other LGTM so:
Reviewed-by: Nikolay Borisov <nbori...@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>
> }
>
>
