On Wed, Oct 10, 2018 at 3:12 AM Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
>
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Combine the clone_file_range and dedupe_file_range operations into a
> single remap_file_range file operation dispatch since they're
> fundamentally the same operation. The differences between the two can
> be made in the prep functions.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
I like this. Nits below.
[...]
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index d60b6caf09e8..e22b294fa25b 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3627,26 +3627,6 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
> return ret;
> }
>
> -int btrfs_dedupe_file_range(struct file *src_file, loff_t src_loff,
> - struct file *dst_file, loff_t dst_loff,
> - u64 olen)
> -{
> - struct inode *src = file_inode(src_file);
> - struct inode *dst = file_inode(dst_file);
> - u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
> -
> - if (WARN_ON_ONCE(bs < PAGE_SIZE)) {
> - /*
> - * Btrfs does not support blocksize < page_size. As a
> - * result, btrfs_cmp_data() won't correctly handle
> - * this situation without an update.
> - */
> - return -EINVAL;
> - }
> -
> - return btrfs_extent_same(src, src_loff, olen, dst, dst_loff);
> -}
> -
> static int clone_finish_inode_update(struct btrfs_trans_handle *trans,
> struct inode *inode,
> u64 endoff,
> @@ -4348,9 +4328,27 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
> return ret;
> }
>
> -int btrfs_clone_file_range(struct file *src_file, loff_t off,
> - struct file *dst_file, loff_t destoff, u64 len)
> +int btrfs_remap_file_range(struct file *src_file, loff_t off,
> + struct file *dst_file, loff_t destoff, u64 len,
> + unsigned int flags)
> {
> + if (flags & RFR_IDENTICAL_DATA) {
> + struct inode *src = file_inode(src_file);
> + struct inode *dst = file_inode(dst_file);
> + u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
> +
> + if (WARN_ON_ONCE(bs < PAGE_SIZE)) {
> + /*
> + * Btrfs does not support blocksize < page_size. As a
> + * result, btrfs_cmp_data() won't correctly handle
> + * this situation without an update.
> + */
> + return -EINVAL;
> + }
> +
> + return btrfs_extent_same(src, off, len, dst, destoff);
> + }
> +
Seems weird that you would do that instead of:
+ if (flags & ~RFR_IDENTICAL_DATA)
+ return -EINVAL;
+ if (flags & RFR_IDENTICAL_DATA)
+ return btrfs_dedupe_file_range(src, off, dst, destoff, len);
> return btrfs_clone_files(dst_file, src_file, off, len, destoff);
> }
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 7065426b3280..bf971fd7cab2 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -975,8 +975,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
> .listxattr = cifs_listxattr,
> };
>
> -static int cifs_clone_file_range(struct file *src_file, loff_t off,
> - struct file *dst_file, loff_t destoff, u64 len)
> +static int cifs_remap_file_range(struct file *src_file, loff_t off,
> + struct file *dst_file, loff_t destoff, u64 len,
> + unsigned int flags)
> {
> struct inode *src_inode = file_inode(src_file);
> struct inode *target_inode = file_inode(dst_file);
> @@ -986,6 +987,9 @@ static int cifs_clone_file_range(struct file *src_file, loff_t off,
> unsigned int xid;
> int rc;
>
> + if (flags & RFR_IDENTICAL_DATA)
> + return -EOPNOTSUPP;
> +
I think everyone would be better off with:
+ if (flags)
+ return -EINVAL;
This way you won't need to change all filesystem implementations
every time that you add a new RFR flag.
Lucky for us, dedup already return -EINVAL if (!f_op->dedupe_file_range)
(and not -EOPNOTSUPP).
[...]
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 986313da0c88..693bd0620a81 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -489,26 +489,28 @@ static ssize_t ovl_copy_file_range(struct file *file_in, loff_t pos_in,
> OVL_COPY);
> }
>
> -static int ovl_clone_file_range(struct file *file_in, loff_t pos_in,
> - struct file *file_out, loff_t pos_out, u64 len)
> +static int ovl_remap_file_range(struct file *file_in, loff_t pos_in,
> + struct file *file_out, loff_t pos_out,
> + u64 len, unsigned int flags)
> {
> - return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, 0,
> - OVL_CLONE);
> -}
> + enum ovl_copyop op;
> +
> + if (flags & RFR_IDENTICAL_DATA)
> + op = OVL_DEDUPE;
> + else
> + op = OVL_CLONE;
>
> -static int ovl_dedupe_file_range(struct file *file_in, loff_t pos_in,
> - struct file *file_out, loff_t pos_out, u64 len)
> -{
> /*
> * Don't copy up because of a dedupe request, this wouldn't make sense
> * most of the time (data would be duplicated instead of deduplicated).
> */
> - if (!ovl_inode_upper(file_inode(file_in)) ||
> - !ovl_inode_upper(file_inode(file_out)))
> + if (op == OVL_DEDUPE &&
> + (!ovl_inode_upper(file_inode(file_in)) ||
> + !ovl_inode_upper(file_inode(file_out))))
> return -EPERM;
>
> return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, 0,
> - OVL_DEDUPE);
> + op);
> }
>
Apart from the generic check invalid flags comment - ACK on ovl part.
Thanks,
Amir.
