On Wed, 2017-02-08 at 15:44 -0800, Pavel Shilovsky wrote:
> 2017-02-07 3:33 GMT-08:00 Sachin Prabhu <sprabhu@xxxxxxxxxx>:
> > The patch introduces the file_operations helper
> > cifs_copy_file_range().
> >
> > The vfs helper vfs_copy_file_range() first calls clone_file_range
> > which for cifs uses
> > SMB2_ioctl(.., FSCTL_DUPLICATE_EXTENTS_TO_FILE, ..)
> > to do a server side copy of the file. This ioctl is only supported
> > on
> > SMB3 and later versions.
> >
> > Once this call fails, vfs_copy_file_range() calls copy_file_range()
> > corresponding to the cifs filesystem which is introduced by this
> > patch.
> > This calls the more widely available
> > SMB2_ioctl(.., FSCTL_SRV_COPYCHUNK_WRITE, ..)
> >
> > The upstream changes in this area was first made by the commit
> > 04b38d601239 ("vfs: pull btrfs clone API to vfs layer")
> > This commit also introduces the ioctls FICLONE and FICLONERANGE
> > which
> > removes the need for separate cifs ioctls to perform server side
> > copies
> > and hence can be removed.
> >
>
> ioctls FICLONE and FICLONERANGE perform a file clone operation which
> for cifs is implemented through FSCTL_DUPLICATE_EXTENTS_TO_FILE for
> SMB3 only. ioctl CIFS_IOC_COPYCHUNK_FILE performs a file copy
> operation which is implemented through FSCTL_SRV_COPYCHUNK_WRITE. So,
> this generic ioctls can't substitute the existing cifs one.
>
> Also there is a copy_file_range() system call that calls
> vfs_copy_file_range(). With this patch applied it tries to clone
> first
> and then copy. It means that for SMB3 it will still call
> DUPLICATE_EXTENTS_TO_FILE and we end up with two calls issuing
> DUPLICATE_EXTENTS_TO_FILE and zero -SRV_COPYCHUNK_WRITE.
>
> Thus I suggest not to remove a separate cifs ioctl that always calls
> COPYCHUNK_WRITE for possible scenarios where
> DUPLICATE_EXTENTS_TO_FILE
> is not suitable.
I'll post another patch which keeps the ioctl intact.
Sachin Prabhu
>
> > Signed-off-by: Sachin Prabhu <sprabhu@xxxxxxxxxx>
> > ---
> > fs/cifs/cifsfs.c | 72 +++++++++++++++++++++++++++++++++++
> > fs/cifs/cifsglob.h | 2 +-
> > fs/cifs/ioctl.c | 107 ---------------------------------------
> > --------------
> > fs/cifs/smb2ops.c | 16 +++++---
> > 4 files changed, 84 insertions(+), 113 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 70f4e65..33bc53d 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -972,6 +972,72 @@ static int cifs_clone_file_range(struct file
> > *src_file, loff_t off,
> > return rc;
> > }
> >
> > +ssize_t cifs_copy_file_range(struct file *src_file, loff_t off,
> > + struct file *dst_file, loff_t destoff,
> > + size_t len, unsigned int flags)
> > +{
> > + struct inode *src_inode = file_inode(src_file);
> > + struct inode *target_inode = file_inode(dst_file);
> > + struct cifsFileInfo *smb_file_src = src_file->private_data;
> > + struct cifsFileInfo *smb_file_target = dst_file-
> > >private_data;
> > + struct cifs_tcon *src_tcon;
> > + struct cifs_tcon *target_tcon;
> > + unsigned int xid;
> > + int rc;
> > +
> > + xid = get_xid();
> > +
> > + cifs_dbg(FYI, "copy file range\n");
> > +
> > + if (src_inode == target_inode) {
> > + rc = -EINVAL;
> > + goto out;
> > + }
> > +
> > + if (!src_file->private_data || !dst_file->private_data) {
> > + rc = -EBADF;
> > + cifs_dbg(VFS, "missing cifsFileInfo on copy range
> > src file\n");
> > + goto out;
> > + }
> > +
> > + rc = -EXDEV;
> > + src_tcon = tlink_tcon(smb_file_src->tlink);
> > + target_tcon = tlink_tcon(smb_file_target->tlink);
> > +
> > + if (src_tcon->ses != target_tcon->ses) {
> > + cifs_dbg(VFS, "source and target of copy not on
> > same server\n");
> > + goto out;
> > + }
> > +
> > + /*
> > + * Note: cifs case is easier than btrfs since server
> > responsible for
> > + * checks for proper open modes and file type and if it
> > wants
> > + * server could even support copy of range where source =
> > target
> > + */
> > + lock_two_nondirectories(target_inode, src_inode);
> > +
> > + cifs_dbg(FYI, "about to flush pages\n");
> > + /* should we flush first and last page first */
> > + truncate_inode_pages(&target_inode->i_data, 0);
> > +
> > + if (target_tcon->ses->server->ops->clone_range)
> > + rc = target_tcon->ses->server->ops-
> > >clone_range(xid,
> > + smb_file_src, smb_file_target, off, len,
> > destoff);
>
> I think there is a naming confusion here. I suggest to rename
> clone_range() callback to copy_range() to reflect its actual logic.
>
> > + else
> > + rc = -EOPNOTSUPP;
> > +
> > + /* force revalidate of size and timestamps of target file
> > now
> > + that target is updated on the server */
> > + CIFS_I(target_inode)->time = 0;
> > + /* although unlocking in the reverse order from locking is
> > not
> > + strictly necessary here it is a little cleaner to be
> > consistent */
> > + unlock_two_nondirectories(src_inode, target_inode);
> > +
> > +out:
> > + free_xid(xid);
> > + return rc;
> > +}
> > +
> > const struct file_operations cifs_file_ops = {
> > .read_iter = cifs_loose_read_iter,
> > .write_iter = cifs_file_write_iter,
> > @@ -984,6 +1050,7 @@ const struct file_operations cifs_file_ops = {
> > .splice_read = generic_file_splice_read,
> > .llseek = cifs_llseek,
> > .unlocked_ioctl = cifs_ioctl,
> > + .copy_file_range = cifs_copy_file_range,
> > .clone_file_range = cifs_clone_file_range,
> > .setlease = cifs_setlease,
> > .fallocate = cifs_fallocate,
> > @@ -1001,6 +1068,7 @@ const struct file_operations
> > cifs_file_strict_ops = {
> > .splice_read = generic_file_splice_read,
> > .llseek = cifs_llseek,
> > .unlocked_ioctl = cifs_ioctl,
> > + .copy_file_range = cifs_copy_file_range,
> > .clone_file_range = cifs_clone_file_range,
> > .setlease = cifs_setlease,
> > .fallocate = cifs_fallocate,
> > @@ -1018,6 +1086,7 @@ const struct file_operations
> > cifs_file_direct_ops = {
> > .mmap = cifs_file_mmap,
> > .splice_read = generic_file_splice_read,
> > .unlocked_ioctl = cifs_ioctl,
> > + .copy_file_range = cifs_copy_file_range,
> > .clone_file_range = cifs_clone_file_range,
> > .llseek = cifs_llseek,
> > .setlease = cifs_setlease,
> > @@ -1035,6 +1104,7 @@ const struct file_operations
> > cifs_file_nobrl_ops = {
> > .splice_read = generic_file_splice_read,
> > .llseek = cifs_llseek,
> > .unlocked_ioctl = cifs_ioctl,
> > + .copy_file_range = cifs_copy_file_range,
> > .clone_file_range = cifs_clone_file_range,
> > .setlease = cifs_setlease,
> > .fallocate = cifs_fallocate,
> > @@ -1051,6 +1121,7 @@ const struct file_operations
> > cifs_file_strict_nobrl_ops = {
> > .splice_read = generic_file_splice_read,
> > .llseek = cifs_llseek,
> > .unlocked_ioctl = cifs_ioctl,
> > + .copy_file_range = cifs_copy_file_range,
> > .clone_file_range = cifs_clone_file_range,
> > .setlease = cifs_setlease,
> > .fallocate = cifs_fallocate,
> > @@ -1067,6 +1138,7 @@ const struct file_operations
> > cifs_file_direct_nobrl_ops = {
> > .mmap = cifs_file_mmap,
> > .splice_read = generic_file_splice_read,
> > .unlocked_ioctl = cifs_ioctl,
> > + .copy_file_range = cifs_copy_file_range,
> > .clone_file_range = cifs_clone_file_range,
> > .llseek = cifs_llseek,
> > .setlease = cifs_setlease,
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 7ea8a33..ae0c095 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -405,7 +405,7 @@ struct smb_version_operations {
> > char * (*create_lease_buf)(u8 *, u8);
> > /* parse lease context buffer and return oplock/epoch info
> > */
> > __u8 (*parse_lease_buf)(void *, unsigned int *);
> > - int (*clone_range)(const unsigned int, struct cifsFileInfo
> > *src_file,
> > + ssize_t (*clone_range)(const unsigned int, struct
> > cifsFileInfo *src_file,
> > struct cifsFileInfo *target_file, u64
> > src_off, u64 len,
> > u64 dest_off);
> > int (*duplicate_extents)(const unsigned int, struct
> > cifsFileInfo *src,
> > diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c
> > index 0015287..14975b7 100644
> > --- a/fs/cifs/ioctl.c
> > +++ b/fs/cifs/ioctl.c
> > @@ -34,110 +34,6 @@
> > #include "cifs_ioctl.h"
> > #include <linux/btrfs.h>
> >
> > -static int cifs_file_clone_range(unsigned int xid, struct file
> > *src_file,
> > - struct file *dst_file)
> > -{
> > - struct inode *src_inode = file_inode(src_file);
> > - struct inode *target_inode = file_inode(dst_file);
> > - struct cifsFileInfo *smb_file_src;
> > - struct cifsFileInfo *smb_file_target;
> > - struct cifs_tcon *src_tcon;
> > - struct cifs_tcon *target_tcon;
> > - int rc;
> > -
> > - cifs_dbg(FYI, "ioctl clone range\n");
> > -
> > - if (!src_file->private_data || !dst_file->private_data) {
> > - rc = -EBADF;
> > - cifs_dbg(VFS, "missing cifsFileInfo on copy range
> > src file\n");
> > - goto out;
> > - }
> > -
> > - rc = -EXDEV;
> > - smb_file_target = dst_file->private_data;
> > - smb_file_src = src_file->private_data;
> > - src_tcon = tlink_tcon(smb_file_src->tlink);
> > - target_tcon = tlink_tcon(smb_file_target->tlink);
> > -
> > - if (src_tcon->ses != target_tcon->ses) {
> > - cifs_dbg(VFS, "source and target of copy not on
> > same server\n");
> > - goto out;
> > - }
> > -
> > - /*
> > - * Note: cifs case is easier than btrfs since server
> > responsible for
> > - * checks for proper open modes and file type and if it
> > wants
> > - * server could even support copy of range where source =
> > target
> > - */
> > - lock_two_nondirectories(target_inode, src_inode);
> > -
> > - cifs_dbg(FYI, "about to flush pages\n");
> > - /* should we flush first and last page first */
> > - truncate_inode_pages(&target_inode->i_data, 0);
> > -
> > - if (target_tcon->ses->server->ops->clone_range)
> > - rc = target_tcon->ses->server->ops-
> > >clone_range(xid,
> > - smb_file_src, smb_file_target, 0,
> > src_inode->i_size, 0);
> > - else
> > - rc = -EOPNOTSUPP;
> > -
> > - /* force revalidate of size and timestamps of target file
> > now
> > - that target is updated on the server */
> > - CIFS_I(target_inode)->time = 0;
> > - /* although unlocking in the reverse order from locking is
> > not
> > - strictly necessary here it is a little cleaner to be
> > consistent */
> > - unlock_two_nondirectories(src_inode, target_inode);
> > -out:
> > - return rc;
> > -}
> > -
> > -static long cifs_ioctl_clone(unsigned int xid, struct file
> > *dst_file,
> > - unsigned long srcfd)
> > -{
> > - int rc;
> > - struct fd src_file;
> > - struct inode *src_inode;
> > -
> > - cifs_dbg(FYI, "ioctl clone range\n");
> > - /* the destination must be opened for writing */
> > - if (!(dst_file->f_mode & FMODE_WRITE)) {
> > - cifs_dbg(FYI, "file target not open for write\n");
> > - return -EINVAL;
> > - }
> > -
> > - /* check if target volume is readonly and take reference */
> > - rc = mnt_want_write_file(dst_file);
> > - if (rc) {
> > - cifs_dbg(FYI, "mnt_want_write failed with rc %d\n",
> > rc);
> > - return rc;
> > - }
> > -
> > - src_file = fdget(srcfd);
> > - if (!src_file.file) {
> > - rc = -EBADF;
> > - goto out_drop_write;
> > - }
> > -
> > - if (src_file.file->f_op->unlocked_ioctl != cifs_ioctl) {
> > - rc = -EBADF;
> > - cifs_dbg(VFS, "src file seems to be from a
> > different filesystem type\n");
> > - goto out_fput;
> > - }
> > -
> > - src_inode = file_inode(src_file.file);
> > - rc = -EINVAL;
> > - if (S_ISDIR(src_inode->i_mode))
> > - goto out_fput;
> > -
> > - rc = cifs_file_clone_range(xid, src_file.file, dst_file);
> > -
> > -out_fput:
> > - fdput(src_file);
> > -out_drop_write:
> > - mnt_drop_write_file(dst_file);
> > - return rc;
> > -}
> > -
> > static long smb_mnt_get_fsinfo(unsigned int xid, struct cifs_tcon
> > *tcon,
> > void __user *arg)
> > {
> > @@ -250,9 +146,6 @@ long cifs_ioctl(struct file *filep, unsigned
> > int command, unsigned long arg)
> > cifs_dbg(FYI, "set compress flag rc
> > %d\n", rc);
> > }
> > break;
> > - case CIFS_IOC_COPYCHUNK_FILE:
> > - rc = cifs_ioctl_clone(xid, filep, arg);
> > - break;
> > case CIFS_IOC_SET_INTEGRITY:
> > if (pSMBFile == NULL)
> > break;
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index 5d456eb..b7aa0926 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -586,7 +586,7 @@ SMB2_request_res_key(const unsigned int xid,
> > struct cifs_tcon *tcon,
> > return rc;
> > }
> >
> > -static int
> > +static ssize_t
> > smb2_clone_range(const unsigned int xid,
> > struct cifsFileInfo *srcfile,
> > struct cifsFileInfo *trgtfile, u64 src_off,
> > @@ -599,6 +599,7 @@ smb2_clone_range(const unsigned int xid,
> > struct cifs_tcon *tcon;
> > int chunks_copied = 0;
> > bool chunk_sizes_updated = false;
> > + ssize_t bytes_written, total_bytes_written = 0;
> >
> > pcchunk = kmalloc(sizeof(struct copychunk_ioctl),
> > GFP_KERNEL);
> >
> > @@ -662,14 +663,16 @@ smb2_clone_range(const unsigned int xid,
> > }
> > chunks_copied++;
> >
> > + bytes_written = le32_to_cpu(retbuf-
> > >TotalBytesWritten);
> > src_off += le32_to_cpu(retbuf-
> > >TotalBytesWritten);
> > dest_off += le32_to_cpu(retbuf-
> > >TotalBytesWritten);
> > - len -= le32_to_cpu(retbuf-
> > >TotalBytesWritten);
> > + len -= bytes_written;
> > + total_bytes_written += bytes_written;
> >
> > - cifs_dbg(FYI, "Chunks %d PartialChunk %d
> > Total %d\n",
> > + cifs_dbg(FYI, "Chunks %d PartialChunk %d
> > Total %zu\n",
> > le32_to_cpu(retbuf->ChunksWritten),
> > le32_to_cpu(retbuf-
> > >ChunkBytesWritten),
> > - le32_to_cpu(retbuf-
> > >TotalBytesWritten));
> > + bytes_written);
> > } else if (rc == -EINVAL) {
> > if (ret_data_len != sizeof(struct
> > copychunk_ioctl_rsp))
> > goto cchunk_out;
> > @@ -706,7 +709,10 @@ smb2_clone_range(const unsigned int xid,
> > cchunk_out:
> > kfree(pcchunk);
> > kfree(retbuf);
> > - return rc;
> > + if (rc)
> > + return rc;
> > + else
> > + return total_bytes_written;
> > }
> >
> > static int
> > --
> > 2.9.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > cifs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Other than the comments above the patch looks good.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
