diff mbox

[2/9] cifs: add .copy_file_range file operation

Message ID 1445728636-10109-3-git-send-email-tao.peng@primarydata.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peng Tao Oct. 24, 2015, 11:17 p.m. UTC 
Signed-off-by: Peng Tao <tao.peng@primarydata.com>
---
 fs/cifs/cifsfs.c |  22 ++++++++++++
 fs/cifs/cifsfs.h |   4 ++-
 fs/cifs/ioctl.c  | 100 +++++++++++++++++++++++++++++++------------------------
 3 files changed, 82 insertions(+), 44 deletions(-)

Comments

Steve French Oct. 27, 2015, 5:13 p.m. UTC | #1 
Patch looks fine, but it points out a Kconfig problem with cifs.ko.
Need to remove the

         #ifdef CONFIG_CIFS_POSIX

around the statements like:

         .unlocked_ioctl = cifs_ioctl,
         .copy_file_range = cifs_file_copy_range,

Copy offload does not require support for the old CIFS POSIX
extensions (or even cifs at all) - it works fine over SMB3.
Alternatively you can move the copy_file_range to the end of the
struct file_operations in your patch, and I can remove the
CONFIG_CIFS_POSIX around cifs_ioctl in a different patch that I put in
my tree - whichever you prefer.

Originally the first ioctl that cifs supported did require the CIFS
POSIX extensions because it used an info level defined in the CIFS
UNIX/POSIX extensions.  With later additions to cifs_ioctl that
restriction is now obsolete.  Later ioctls have been added that work
over SMB3, and do not require the CIFS POSIX extensions (such as copy
offload).   The patch that originally introduced the CIFS_POSIX ifdef
around this ioctl was

commit c67593a03129967eae8939c4899767182eb6d6cd
Author: Steve French <smfrench@austin.rr.com>
Date:   Thu Apr 28 22:41:04 2005 -0700

    [PATCH] cifs: Enable ioctl support in POSIX extensions to handle lsattr

but obviously now we don't need the CONFIG_CIFS_POSIX ifdef around the
ioctl functions.

On Sat, Oct 24, 2015 at 6:17 PM, Peng Tao <tao.peng@primarydata.com> wrote:
> Signed-off-by: Peng Tao <tao.peng@primarydata.com>
> ---
>  fs/cifs/cifsfs.c |  22 ++++++++++++
>  fs/cifs/cifsfs.h |   4 ++-
>  fs/cifs/ioctl.c  | 100 +++++++++++++++++++++++++++++++------------------------
>  3 files changed, 82 insertions(+), 44 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index e739950..6ef7c3c 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -910,6 +910,22 @@ const struct inode_operations cifs_symlink_inode_ops = {
>  #endif
>  };
>
> +#ifdef CONFIG_CIFS_POSIX
> +ssize_t cifs_file_copy_range(struct file *file_in, loff_t pos_in,
> +                            struct file *file_out, loff_t pos_out,
> +                            size_t len, unsigned int flags)
> +{
> +       unsigned int xid;
> +       int rc;
> +
> +       xid = get_xid();
> +       rc = cifs_file_clone_range(xid, file_in, file_out, pos_in,
> +                                  len, pos_out, true);
> +       free_xid(xid);
> +       return rc < 0 ? rc : len;
> +}
> +#endif
> +
>  const struct file_operations cifs_file_ops = {
>         .read_iter = cifs_loose_read_iter,
>         .write_iter = cifs_file_write_iter,
> @@ -923,6 +939,7 @@ const struct file_operations cifs_file_ops = {
>         .llseek = cifs_llseek,
>  #ifdef CONFIG_CIFS_POSIX
>         .unlocked_ioctl = cifs_ioctl,
> +       .copy_file_range = cifs_file_copy_range,
>  #endif /* CONFIG_CIFS_POSIX */
>         .setlease = cifs_setlease,
>         .fallocate = cifs_fallocate,
> @@ -941,6 +958,7 @@ const struct file_operations cifs_file_strict_ops = {
>         .llseek = cifs_llseek,
>  #ifdef CONFIG_CIFS_POSIX
>         .unlocked_ioctl = cifs_ioctl,
> +       .copy_file_range = cifs_file_copy_range,
>  #endif /* CONFIG_CIFS_POSIX */
>         .setlease = cifs_setlease,
>         .fallocate = cifs_fallocate,
> @@ -959,6 +977,7 @@ const struct file_operations cifs_file_direct_ops = {
>         .splice_read = generic_file_splice_read,
>  #ifdef CONFIG_CIFS_POSIX
>         .unlocked_ioctl  = cifs_ioctl,
> +       .copy_file_range = cifs_file_copy_range,
>  #endif /* CONFIG_CIFS_POSIX */
>         .llseek = cifs_llseek,
>         .setlease = cifs_setlease,
> @@ -977,6 +996,7 @@ const struct file_operations cifs_file_nobrl_ops = {
>         .llseek = cifs_llseek,
>  #ifdef CONFIG_CIFS_POSIX
>         .unlocked_ioctl = cifs_ioctl,
> +       .copy_file_range = cifs_file_copy_range,
>  #endif /* CONFIG_CIFS_POSIX */
>         .setlease = cifs_setlease,
>         .fallocate = cifs_fallocate,
> @@ -994,6 +1014,7 @@ const struct file_operations cifs_file_strict_nobrl_ops = {
>         .llseek = cifs_llseek,
>  #ifdef CONFIG_CIFS_POSIX
>         .unlocked_ioctl = cifs_ioctl,
> +       .copy_file_range = cifs_file_copy_range,
>  #endif /* CONFIG_CIFS_POSIX */
>         .setlease = cifs_setlease,
>         .fallocate = cifs_fallocate,
> @@ -1011,6 +1032,7 @@ const struct file_operations cifs_file_direct_nobrl_ops = {
>         .splice_read = generic_file_splice_read,
>  #ifdef CONFIG_CIFS_POSIX
>         .unlocked_ioctl  = cifs_ioctl,
> +       .copy_file_range = cifs_file_copy_range,
>  #endif /* CONFIG_CIFS_POSIX */
>         .llseek = cifs_llseek,
>         .setlease = cifs_setlease,


Patch looks fine, but it points out a Kconfig problem with cifs.ko.
Need to remove the

#ifdef CONFIG_CIFS_POSIX

around the

         .unlocked_ioctl = cifs_ioctl,
         .copy_file_range = cifs_file_copy_range,

statements.  Alternatively you can move the copy_file_range to the end
of the struct file_operations in your patch, and I can remove the
CONFIG_CIFS_POSIX around cifs_ioctl in a different patch that I put in
my tree - whichever you prefer.

Originally the first ioctl that cifs supported did require the CIFS
POSIX extensions because it used an info level defined in the CIFS
UNIX/POSIX extensions.  With later additions to cifs_ioctl that
restriction is now obsolete.  Later ioctls have been added that work
over SMB3, and do not require the CIFS POSIX extensions (such as copy
offload).  See patch
diff mbox

Patch

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index e739950..6ef7c3c 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -910,6 +910,22 @@  const struct inode_operations cifs_symlink_inode_ops = {
 #endif
 };
 
+#ifdef CONFIG_CIFS_POSIX
+ssize_t cifs_file_copy_range(struct file *file_in, loff_t pos_in,
+			     struct file *file_out, loff_t pos_out,
+			     size_t len, unsigned int flags)
+{
+	unsigned int xid;
+	int rc;
+
+	xid = get_xid();
+	rc = cifs_file_clone_range(xid, file_in, file_out, pos_in,
+				   len, pos_out, true);
+	free_xid(xid);
+	return rc < 0 ? rc : len;
+}
+#endif
+
 const struct file_operations cifs_file_ops = {
 	.read_iter = cifs_loose_read_iter,
 	.write_iter = cifs_file_write_iter,
@@ -923,6 +939,7 @@  const struct file_operations cifs_file_ops = {
 	.llseek = cifs_llseek,
 #ifdef CONFIG_CIFS_POSIX
 	.unlocked_ioctl	= cifs_ioctl,
+	.copy_file_range = cifs_file_copy_range,
 #endif /* CONFIG_CIFS_POSIX */
 	.setlease = cifs_setlease,
 	.fallocate = cifs_fallocate,
@@ -941,6 +958,7 @@  const struct file_operations cifs_file_strict_ops = {
 	.llseek = cifs_llseek,
 #ifdef CONFIG_CIFS_POSIX
 	.unlocked_ioctl	= cifs_ioctl,
+	.copy_file_range = cifs_file_copy_range,
 #endif /* CONFIG_CIFS_POSIX */
 	.setlease = cifs_setlease,
 	.fallocate = cifs_fallocate,
@@ -959,6 +977,7 @@  const struct file_operations cifs_file_direct_ops = {
 	.splice_read = generic_file_splice_read,
 #ifdef CONFIG_CIFS_POSIX
 	.unlocked_ioctl  = cifs_ioctl,
+	.copy_file_range = cifs_file_copy_range,
 #endif /* CONFIG_CIFS_POSIX */
 	.llseek = cifs_llseek,
 	.setlease = cifs_setlease,
@@ -977,6 +996,7 @@  const struct file_operations cifs_file_nobrl_ops = {
 	.llseek = cifs_llseek,
 #ifdef CONFIG_CIFS_POSIX
 	.unlocked_ioctl	= cifs_ioctl,
+	.copy_file_range = cifs_file_copy_range,
 #endif /* CONFIG_CIFS_POSIX */
 	.setlease = cifs_setlease,
 	.fallocate = cifs_fallocate,
@@ -994,6 +1014,7 @@  const struct file_operations cifs_file_strict_nobrl_ops = {
 	.llseek = cifs_llseek,
 #ifdef CONFIG_CIFS_POSIX
 	.unlocked_ioctl	= cifs_ioctl,
+	.copy_file_range = cifs_file_copy_range,
 #endif /* CONFIG_CIFS_POSIX */
 	.setlease = cifs_setlease,
 	.fallocate = cifs_fallocate,
@@ -1011,6 +1032,7 @@  const struct file_operations cifs_file_direct_nobrl_ops = {
 	.splice_read = generic_file_splice_read,
 #ifdef CONFIG_CIFS_POSIX
 	.unlocked_ioctl  = cifs_ioctl,
+	.copy_file_range = cifs_file_copy_range,
 #endif /* CONFIG_CIFS_POSIX */
 	.llseek = cifs_llseek,
 	.setlease = cifs_setlease,
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index c3cc160..797439b 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -131,7 +131,9 @@  extern int	cifs_setxattr(struct dentry *, const char *, const void *,
 extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
 extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
 extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
-
+extern int cifs_file_clone_range(unsigned int xid, struct file *src_file,
+				 struct file *dst_file, u64 off, u64 len,
+				 u64 destoff, bool dup_extents);
 #ifdef CONFIG_CIFS_NFSD_EXPORT
 extern const struct export_operations cifs_export_ops;
 #endif /* CONFIG_CIFS_NFSD_EXPORT */
diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c
index 28a77bf..bbab940 100644
--- a/fs/cifs/ioctl.c
+++ b/fs/cifs/ioctl.c
@@ -34,68 +34,36 @@ 
 #include "cifs_ioctl.h"
 #include <linux/btrfs.h>
 
-static long cifs_ioctl_clone(unsigned int xid, struct file *dst_file,
-			unsigned long srcfd, u64 off, u64 len, u64 destoff,
-			bool dup_extents)
+int cifs_file_clone_range(unsigned int xid, struct file *src_file,
+			  struct file *dst_file, u64 off, u64 len,
+			  u64 destoff, bool dup_extents)
 {
-	int rc;
-	struct cifsFileInfo *smb_file_target = dst_file->private_data;
+	struct inode *src_inode = file_inode(src_file);
 	struct inode *target_inode = file_inode(dst_file);
-	struct cifs_tcon *target_tcon;
-	struct fd src_file;
 	struct cifsFileInfo *smb_file_src;
-	struct inode *src_inode;
+	struct cifsFileInfo *smb_file_target;
 	struct cifs_tcon *src_tcon;
+	struct cifs_tcon *target_tcon;
+	int rc;
 
-	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;
-	}
-
-	if ((!src_file.file->private_data) || (!dst_file->private_data)) {
+	if ((!src_file->private_data) || (!dst_file->private_data)) {
 		rc = -EBADF;
 		cifs_dbg(VFS, "missing cifsFileInfo on copy range src file\n");
-		goto out_fput;
+		goto out;
 	}
 
 	rc = -EXDEV;
 	smb_file_target = dst_file->private_data;
-	smb_file_src = src_file.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);
 
 	/* check if source and target are on same tree connection */
 	if (src_tcon != target_tcon) {
 		cifs_dbg(VFS, "file copy src and target on different volume\n");
-		goto out_fput;
+		goto out;
 	}
 
-	src_inode = file_inode(src_file.file);
-	rc = -EINVAL;
-	if (S_ISDIR(src_inode->i_mode))
-		goto out_fput;
-
 	/*
 	 * Note: cifs case is easier than btrfs since server responsible for
 	 * checks for proper open modes and file type and if it wants
@@ -131,6 +99,52 @@  out_unlock:
 	/* 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, u64 off, u64 len, u64 destoff,
+			bool dup_extents)
+{
+	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, off, len,
+				   destoff, dup_extents);
+
 out_fput:
 	fdput(src_file);
 out_drop_write: