nfs: add support for splice writes

PATCH Superseded Suresh Jayaraman 6 participants 14 messages Apr 2, 2009 – Apr 22, 2009

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Suresh Jayaraman

author

2009-04-02 06:21 UTC #0 lore raw

PATCH 1/1 nfs: add support for splice writes

This patch attempts to add splice writes support. In essence, it just calls generic_file_splice_write() after doing a little sanity check. This would allow LTTng users that are using NFS to store trace data. There could be more applications that could be benefitted too.

I have tested this using the Jens' test program and have found no real issues. The test program is inlined below:

/*
* splice-out.c: Splice stdout to file
*/
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>

#define SPLICE_SIZE (64*1024)

int main(int argc, char *argv[])

{
        int fd;

        if (argc < 2) {
                printf("%s: outfile\n", argv[0]);
                return 1;
        }

        fd = open(argv[1], O_WRONLY | O_CREAT | O_TRUNC, 0644);
        if (fd < 0) {
                perror("open");
                return 1;
        }

        do {
                int ret = splice(STDIN_FILENO, NULL, fd, NULL, SPLICE_SIZE, 0);

                if (ret < 0) {
                        perror("splice");
                        break;
                } else if (ret < SPLICE_SIZE)
                        break;
        } while (1);

        close(fd);
        return 0;
}

Compile with -D _GNU_SOURCE and do something like:
echo "some stuff" | ./splice-out <outfile>

Signed-off-by Suresh Jayaraman <[email protected]>

---
 fs/nfs/file.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

fs/nfs/file.c +24 -0

index 90f292b..13d6a00 100644
@@ -47,6 +47,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
 					size_t count, unsigned int flags);
 static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+					struct file *filp, loff_t *ppos,
+					size_t count, unsigned int flags);
 static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
 static int  nfs_file_flush(struct file *, fl_owner_t id);

@@ -76,6 +79,7 @@ const struct file_operations nfs_file_operations = {
 	.lock		= nfs_lock,
 	.flock		= nfs_flock,
 	.splice_read	= nfs_file_splice_read,
+	.splice_write	= nfs_file_splice_write,
 	.check_flags	= nfs_check_flags,
 	.setlease	= nfs_setlease,
 };

@@ -550,6 +554,26 @@ out_swapfile:
 	goto out;
 }
 
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+				     struct file *filp, loff_t *ppos,
+				     size_t count, unsigned int flags)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+
+	dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+		dentry->d_parent->d_name.name, dentry->d_name.name,
+		(unsigned long) count, (unsigned long long) *ppos);
+
+	if (IS_SWAPFILE(inode)) {
+		printk(KERN_INFO "NFS: attempt to write to active swap"
+		       "file!\n");
+		return -EBUSY;
+	}
+
+	return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+
 static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = filp->f_mapping->host;

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Mathieu Desnoyers

2009-04-02 06:29 UTC #1 lore raw

Quoted 8 lines · * Suresh Jayaraman ([email protected]) wrote:

This patch attempts to add splice writes support. In essence, it just calls generic_file_splice_write() after doing a little sanity check. This would allow LTTng users that are using NFS to store trace data. There could be more applications that could be benefitted too.
I have tested this using the Jens' test program and have found no real issues. The test program is inlined below:

That's great ! I'll pull it in the LTTng tree so my users can try it. They currently cannot write LTTng traces to NFS mounts anyway.

Thanks !

Mathieu

Quoted 97 lines

/*
* splice-out.c: Splice stdout to file
*/
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>

#define SPLICE_SIZE (64*1024)

int main(int argc, char *argv[])

{
        int fd;

        if (argc < 2) {
                printf("%s: outfile\n", argv[0]);
                return 1;
        }

        fd = open(argv[1], O_WRONLY | O_CREAT | O_TRUNC, 0644);
        if (fd < 0) {
                perror("open");
                return 1;
        }

        do {
                int ret = splice(STDIN_FILENO, NULL, fd, NULL, SPLICE_SIZE, 0);

                if (ret < 0) {
                        perror("splice");
                        break;
                } else if (ret < SPLICE_SIZE)
                        break;
        } while (1);

        close(fd);
        return 0;
}

Compile with -D _GNU_SOURCE and do something like:
echo "some stuff" | ./splice-out <outfile>

Signed-off-by Suresh Jayaraman <[email protected]>

---
 fs/nfs/file.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

fs/nfs/file.c +24 -0

index 90f292b..13d6a00 100644
@@ -47,6 +47,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
 					size_t count, unsigned int flags);
 static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+					struct file *filp, loff_t *ppos,
+					size_t count, unsigned int flags);
 static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
 static int  nfs_file_flush(struct file *, fl_owner_t id);

@@ -76,6 +79,7 @@ const struct file_operations nfs_file_operations = {
 	.lock		= nfs_lock,
 	.flock		= nfs_flock,
 	.splice_read	= nfs_file_splice_read,
+	.splice_write	= nfs_file_splice_write,
 	.check_flags	= nfs_check_flags,
 	.setlease	= nfs_setlease,
 };

@@ -550,6 +554,26 @@ out_swapfile:
 	goto out;
 }
 
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+				     struct file *filp, loff_t *ppos,
+				     size_t count, unsigned int flags)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+
+	dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+		dentry->d_parent->d_name.name, dentry->d_name.name,
+		(unsigned long) count, (unsigned long long) *ppos);
+
+	if (IS_SWAPFILE(inode)) {
+		printk(KERN_INFO "NFS: attempt to write to active swap"
+		       "file!\n");
+		return -EBUSY;
+	}
+
+	return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+
 static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = filp->f_mapping->host;

Signature · 6 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mathieu Desnoyers

2009-04-02 06:32 UTC #2 lore raw

In reply to Suresh Jayaraman

Quoted 8 lines · * Suresh Jayaraman ([email protected]) wrote:

This patch attempts to add splice writes support. In essence, it just calls generic_file_splice_write() after doing a little sanity check. This would allow LTTng users that are using NFS to store trace data. There could be more applications that could be benefitted too.
I have tested this using the Jens' test program and have found no real issues. The test program is inlined below:

There is just a small checkpatch nit that I'll fix directly in place in the LTTng tree.

WARNING: %Ld/%Lu are not-standard C, use %lld/%llu #93: FILE: fs/nfs/file.c:564:
+ dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",

total: 0 errors, 1 warnings, 42 lines checked

Mathieu

Quoted 97 lines

/*
* splice-out.c: Splice stdout to file
*/
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>

#define SPLICE_SIZE (64*1024)

int main(int argc, char *argv[])

{
        int fd;

        if (argc < 2) {
                printf("%s: outfile\n", argv[0]);
                return 1;
        }

        fd = open(argv[1], O_WRONLY | O_CREAT | O_TRUNC, 0644);
        if (fd < 0) {
                perror("open");
                return 1;
        }

        do {
                int ret = splice(STDIN_FILENO, NULL, fd, NULL, SPLICE_SIZE, 0);

                if (ret < 0) {
                        perror("splice");
                        break;
                } else if (ret < SPLICE_SIZE)
                        break;
        } while (1);

        close(fd);
        return 0;
}

Compile with -D _GNU_SOURCE and do something like:
echo "some stuff" | ./splice-out <outfile>

Signed-off-by Suresh Jayaraman <[email protected]>

---
 fs/nfs/file.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

fs/nfs/file.c +24 -0

index 90f292b..13d6a00 100644
@@ -47,6 +47,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
 					size_t count, unsigned int flags);
 static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+					struct file *filp, loff_t *ppos,
+					size_t count, unsigned int flags);
 static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
 static int  nfs_file_flush(struct file *, fl_owner_t id);

@@ -76,6 +79,7 @@ const struct file_operations nfs_file_operations = {
 	.lock		= nfs_lock,
 	.flock		= nfs_flock,
 	.splice_read	= nfs_file_splice_read,
+	.splice_write	= nfs_file_splice_write,
 	.check_flags	= nfs_check_flags,
 	.setlease	= nfs_setlease,
 };

@@ -550,6 +554,26 @@ out_swapfile:
 	goto out;
 }
 
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+				     struct file *filp, loff_t *ppos,
+				     size_t count, unsigned int flags)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+
+	dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+		dentry->d_parent->d_name.name, dentry->d_name.name,
+		(unsigned long) count, (unsigned long long) *ppos);
+
+	if (IS_SWAPFILE(inode)) {
+		printk(KERN_INFO "NFS: attempt to write to active swap"
+		       "file!\n");
+		return -EBUSY;
+	}
+
+	return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+
 static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = filp->f_mapping->host;

Signature · 6 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Suresh Jayaraman

author

2009-04-02 06:42 UTC #3 lore raw

Quoted 17 lines · Mathieu Desnoyers

Quoted 8 lines · * Suresh Jayaraman ([email protected]) wrote:
This patch attempts to add splice writes support. In essence, it just calls generic_file_splice_write() after doing a little sanity check. This would allow LTTng users that are using NFS to store trace data. There could be more applications that could be benefitted too.
I have tested this using the Jens' test program and have found no real issues. The test program is inlined below:
There is just a small checkpatch nit that I'll fix directly in place in the LTTng tree.
WARNING: %Ld/%Lu are not-standard C, use %lld/%llu #93: FILE: fs/nfs/file.c:564:
fs/nfs/file.c +1 -0
+	dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",

Quoted 2 lines

total: 0 errors, 1 warnings, 42 lines checked

Yes, I noticed it. There are quite a few places in nfs code where we happened to use that (that doesn't imply that it shouldn't be fixed), so I thought it's OK.

Quoted 2 lines

That's great ! I'll pull it in the LTTng tree so my users can try it. They currently cannot write LTTng traces to NFS mounts anyway.

Yes, please report the test results. Would appreciate it very much.

Thanks,

Quoted 52 lines

Quoted 51 lines

fs/nfs/file.c +24 -0

index 90f292b..13d6a00 100644
@@ -47,6 +47,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
 					size_t count, unsigned int flags);
 static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+					struct file *filp, loff_t *ppos,
+					size_t count, unsigned int flags);
 static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
 static int  nfs_file_flush(struct file *, fl_owner_t id);

@@ -76,6 +79,7 @@ const struct file_operations nfs_file_operations = {
 	.lock		= nfs_lock,
 	.flock		= nfs_flock,
 	.splice_read	= nfs_file_splice_read,
+	.splice_write	= nfs_file_splice_write,
 	.check_flags	= nfs_check_flags,
 	.setlease	= nfs_setlease,
 };

@@ -550,6 +554,26 @@ out_swapfile:
 	goto out;
 }
 
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+				     struct file *filp, loff_t *ppos,
+				     size_t count, unsigned int flags)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+
+	dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+		dentry->d_parent->d_name.name, dentry->d_name.name,
+		(unsigned long) count, (unsigned long long) *ppos);
+
+	if (IS_SWAPFILE(inode)) {
+		printk(KERN_INFO "NFS: attempt to write to active swap"
+		       "file!\n");
+		return -EBUSY;
+	}
+
+	return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+
 static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = filp->f_mapping->host;

Signature · 5 lines

Suresh Jayaraman
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Masahiro Tamori

2009-04-02 12:42 UTC #4 lore raw

Hello Suresh and Mathieu,

2009/4/2 Suresh Jayaraman <[email protected]>:

Quoted 25 lines

Quoted 10 lines · Mathieu Desnoyers
Quoted 8 lines · * Suresh Jayaraman ([email protected]) wrote:
This patch attempts to add splice writes support. In essence, it just calls generic_file_splice_write() after doing a little sanity check. This would allow LTTng users that are using NFS to store trace data. There could be more applications that could be benefitted too.
I have tested this using the Jens' test program and have found no real issues. The test program is inlined below:
Quoted 9 lines
There is just a small checkpatch nit that I'll fix directly in place in the LTTng tree.
WARNING: %Ld/%Lu are not-standard C, use %lld/%llu #93: FILE: fs/nfs/file.c:564:
+ dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
total: 0 errors, 1 warnings, 42 lines checked
Yes, I noticed it. There are quite a few places in nfs code where we happened to use that (that doesn't imply that it shouldn't be fixed), so I thought it's OK.

Quoted 7 lines

Quoted 2 lines
That's great ! I'll pull it in the LTTng tree so my users can try it. They currently cannot write LTTng traces to NFS mounts anyway.
Yes, please report the test results. Would appreciate it very much.
Thanks,

Thank you for creating a patch !!

I tested it, and it works fine.

My environment is following,
LTTV 0.12.10
LTTng 0.100
LTT Control 0.65
Kernles 2.6.29

LTTng version is old now, but the test is passed on ARM11 target.

Furthermore, I run the splice test and the test is passed too. The test code is copied from
http://kzk9.net/blog/2007/05/splice2.html
(Japanese page)

Thanks all people who commented this issue.

Best regards,
Masahiro Tamori

Quoted 63 lines

Quoted 44 lines

fs/nfs/file.c +20 -0

index 90f292b..13d6a00 100644
@@ -47,6 +47,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
                  size_t count, unsigned int flags);
 static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
              unsigned long nr_segs, loff_t pos);
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+                  struct file *filp, loff_t *ppos,
+                  size_t count, unsigned int flags);
 static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
              unsigned long nr_segs, loff_t pos);
 static int nfs_file_flush(struct file *, fl_owner_t id);

@@ -76,6 +79,7 @@ const struct file_operations nfs_file_operations = {
  .lock      = nfs_lock,
  .flock     = nfs_flock,
  .splice_read  = nfs_file_splice_read,
+  .splice_write  = nfs_file_splice_write,
  .check_flags  = nfs_check_flags,
  .setlease    = nfs_setlease,
 };

@@ -550,6 +554,26 @@ out_swapfile:
  goto out;
 }
 
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+                 struct file *filp, loff_t *ppos,
+                 size_t count, unsigned int flags)
+{
+  struct dentry *dentry = filp->f_path.dentry;
+  struct inode *inode = dentry->d_inode;
+
+  dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+      dentry->d_parent->d_name.name, dentry->d_name.name,
+      (unsigned long) count, (unsigned long long) *ppos);
+
+  if (IS_SWAPFILE(inode)) {
+      printk(KERN_INFO "NFS: attempt to write to active swap"
+          "file!\n");
+      return -EBUSY;
+  }

Quoted 9 lines

fs/nfs/file.c out_swapfile: · line 579

Quoted 8 lines

fs/nfs/file.c +4 -0

+
+  return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+
 static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
 {
  struct inode *inode = filp->f_mapping->host;

--
Suresh Jayaraman

_______________________________________________
ltt-dev mailing list
[email protected]
http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev

Suresh Jayaraman

author

2009-04-20 05:39 UTC #5 lore raw

Hi Trond,

Do you think this patch is OK? Can this be considered for merging?

Thanks,

Quoted 118 lines · Masahiro Tamori

Hello Suresh and Mathieu,
2009/4/2 Suresh Jayaraman <[email protected]>:
Quoted 23 lines
Quoted 9 lines · Mathieu Desnoyers
Quoted 8 lines · * Suresh Jayaraman ([email protected]) wrote:
This patch attempts to add splice writes support. In essence, it just calls generic_file_splice_write() after doing a little sanity check. This would allow LTTng users that are using NFS to store trace data. There could be more applications that could be benefitted too.
I have tested this using the Jens' test program and have found no real issues. The test program is inlined below:
Quoted 9 lines
There is just a small checkpatch nit that I'll fix directly in place in the LTTng tree.
WARNING: %Ld/%Lu are not-standard C, use %lld/%llu #93: FILE: fs/nfs/file.c:564:
+ dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
total: 0 errors, 1 warnings, 42 lines checked
Yes, I noticed it. There are quite a few places in nfs code where we happened to use that (that doesn't imply that it shouldn't be fixed), so I thought it's OK.
Quoted 6 lines
Quoted 2 lines
That's great ! I'll pull it in the LTTng tree so my users can try it. They currently cannot write LTTng traces to NFS mounts anyway.
Yes, please report the test results. Would appreciate it very much.
Thanks,
Thank you for creating a patch !!
I tested it, and it works fine.
My environment is following,
LTTV 0.12.10
LTTng 0.100
LTT Control 0.65
Kernles 2.6.29
LTTng version is old now, but the test is passed on ARM11 target.
Furthermore, I run the splice test and the test is passed too. The test code is copied from
http://kzk9.net/blog/2007/05/splice2.html
(Japanese page)
Thanks all people who commented this issue.
Best regards,
Masahiro Tamori
Quoted 54 lines
Quoted 42 lines
Quoted 42 lines
fs/nfs/file.c +20 -0
index 90f292b..13d6a00 100644
@@ -47,6 +47,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
                  size_t count, unsigned int flags);
 static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
              unsigned long nr_segs, loff_t pos);
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+                  struct file *filp, loff_t *ppos,
+                  size_t count, unsigned int flags);
 static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
              unsigned long nr_segs, loff_t pos);
 static int nfs_file_flush(struct file *, fl_owner_t id);
@@ -76,6 +79,7 @@ const struct file_operations nfs_file_operations = {
  .lock      = nfs_lock,
  .flock     = nfs_flock,
  .splice_read  = nfs_file_splice_read,
+  .splice_write  = nfs_file_splice_write,
  .check_flags  = nfs_check_flags,
  .setlease    = nfs_setlease,
 };
@@ -550,6 +554,26 @@ out_swapfile:
  goto out;
 }
 
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+                 struct file *filp, loff_t *ppos,
+                 size_t count, unsigned int flags)
+{
+  struct dentry *dentry = filp->f_path.dentry;
+  struct inode *inode = dentry->d_inode;
+
+  dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+      dentry->d_parent->d_name.name, dentry->d_name.name,
+      (unsigned long) count, (unsigned long long) *ppos);
+
+  if (IS_SWAPFILE(inode)) {
+      printk(KERN_INFO "NFS: attempt to write to active swap"
+          "file!\n");
+      return -EBUSY;
+  }
Quoted 8 lines
fs/nfs/file.c out_swapfile: · line 579
Quoted 8 lines
fs/nfs/file.c +4 -0
+
+  return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+
 static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
 {
  struct inode *inode = filp->f_mapping->host;
--
Suresh Jayaraman
Quoted 5 lines
_______________________________________________
ltt-dev mailing list
[email protected]
http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

Suresh Jayaraman

Trond Myklebust

2009-04-20 12:23 UTC #6 lore raw

Quoted 88 lines · Suresh Jayaraman

Hi Trond,
Do you think this patch is OK? Can this be considered for merging?
Thanks,
Quoted 81 lines · Masahiro Tamori
Hello Suresh and Mathieu,
2009/4/2 Suresh Jayaraman <[email protected]>:
Quoted 23 lines
Quoted 9 lines · Mathieu Desnoyers
Quoted 8 lines · * Suresh Jayaraman ([email protected]) wrote:
This patch attempts to add splice writes support. In essence, it just calls generic_file_splice_write() after doing a little sanity check. This would allow LTTng users that are using NFS to store trace data. There could be more applications that could be benefitted too.
I have tested this using the Jens' test program and have found no real issues. The test program is inlined below:
Quoted 9 lines
There is just a small checkpatch nit that I'll fix directly in place in the LTTng tree.
WARNING: %Ld/%Lu are not-standard C, use %lld/%llu #93: FILE: fs/nfs/file.c:564:
+ dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
total: 0 errors, 1 warnings, 42 lines checked
Yes, I noticed it. There are quite a few places in nfs code where we happened to use that (that doesn't imply that it shouldn't be fixed), so I thought it's OK.
Quoted 6 lines
Quoted 2 lines
That's great ! I'll pull it in the LTTng tree so my users can try it. They currently cannot write LTTng traces to NFS mounts anyway.
Yes, please report the test results. Would appreciate it very much.
Thanks,
Thank you for creating a patch !!
I tested it, and it works fine.
My environment is following,
LTTV 0.12.10
LTTng 0.100
LTT Control 0.65
Kernles 2.6.29
LTTng version is old now, but the test is passed on ARM11 target.
Furthermore, I run the splice test and the test is passed too. The test code is copied from
http://kzk9.net/blog/2007/05/splice2.html
(Japanese page)
Thanks all people who commented this issue.
Best regards,
Masahiro Tamori
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
fs/nfs/file.c +4 -0
index 90f292b..13d6a00 100644
@@ -47,6 +47,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
                  size_t count, unsigned int flags);
 static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
              unsigned long nr_segs, loff_t pos);
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+                  struct file *filp, loff_t *ppos,
+                  size_t count, unsigned int flags);
 static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
              unsigned long nr_segs, loff_t pos);
 static int nfs_file_flush(struct file *, fl_owner_t id);
@@ -76,6 +79,7 @@ const struct file_operations nfs_file_operations = {
  .lock      = nfs_lock,
  .flock     = nfs_flock,
  .splice_read  = nfs_file_splice_read,
+  .splice_write  = nfs_file_splice_write,
  .check_flags  = nfs_check_flags,
  .setlease    = nfs_setlease,
 };
@@ -550,6 +554,26 @@ out_swapfile:
  goto out;
 }
 

Quoted 16 lines

Quoted 8 lines

fs/nfs/file.c +8 -0

+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+                 struct file *filp, loff_t *ppos,
+                 size_t count, unsigned int flags)
+{
+  struct dentry *dentry = filp->f_path.dentry;
+  struct inode *inode = dentry->d_inode;
+
+  dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",

Quoted 8 lines

fs/nfs/file.c out_swapfile: · line 572

Quoted 8 lines

fs/nfs/file.c +8 -0

+      dentry->d_parent->d_name.name, dentry->d_name.name,
+      (unsigned long) count, (unsigned long long) *ppos);
+
+  if (IS_SWAPFILE(inode)) {
+      printk(KERN_INFO "NFS: attempt to write to active swap"
+          "file!\n");
+      return -EBUSY;
+  }

I don't know that we really need this. We should sweep through the NFS code and kill all those IS_SWAPFILE() thingys. Or at least #define IS_SWAPFILE(a) (0)
...

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
fs/nfs/file.c out_swapfile: · line 579
Quoted 8 lines
fs/nfs/file.c +4 -0
+
+  return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+
 static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
 {
  struct inode *inode = filp->f_mapping->host;

Otherwise it looks fine...

Suresh Jayaraman

author

2009-04-20 12:38 UTC #7 lore raw

Quoted 46 lines · Trond Myklebust

Quoted 45 lines · Suresh Jayaraman

Hi Trond,

Do you think this patch is OK? Can this be considered for merging?

Thanks,

Quoted 39 lines

fs/nfs/file.c +17 -0

index 90f292b..13d6a00 100644
@@ -47,6 +47,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
                  size_t count, unsigned int flags);
 static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
              unsigned long nr_segs, loff_t pos);
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+                  struct file *filp, loff_t *ppos,
+                  size_t count, unsigned int flags);
 static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
              unsigned long nr_segs, loff_t pos);
 static int nfs_file_flush(struct file *, fl_owner_t id);

@@ -76,6 +79,7 @@ const struct file_operations nfs_file_operations = {
  .lock      = nfs_lock,
  .flock     = nfs_flock,
  .splice_read  = nfs_file_splice_read,
+  .splice_write  = nfs_file_splice_write,
  .check_flags  = nfs_check_flags,
  .setlease    = nfs_setlease,
 };

@@ -550,6 +554,26 @@ out_swapfile:
  goto out;
 }
 
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+                 struct file *filp, loff_t *ppos,
+                 size_t count, unsigned int flags)
+{
+  struct dentry *dentry = filp->f_path.dentry;
+  struct inode *inode = dentry->d_inode;
+
+  dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+      dentry->d_parent->d_name.name, dentry->d_name.name,
+      (unsigned long) count, (unsigned long long) *ppos);
+
+  if (IS_SWAPFILE(inode)) {
+      printk(KERN_INFO "NFS: attempt to write to active swap"

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
fs/nfs/file.c out_swapfile: · line 572
Quoted 3 lines
fs/nfs/file.c +3 -0
+          "file!\n");
+      return -EBUSY;
+  }
I don't know that we really need this. We should sweep through the NFS code and kill all those IS_SWAPFILE() thingys. Or at least #define IS_SWAPFILE(a) (0)
...

Hmm.. I'm not sure whether we should kill them now. I think originally, these were added keeping in mind the future NFS swap support. Given that the recent work from Peterz Zilstra on "Swap over NFS" and multiple iterations/review on the same, I think those patches will eventually get merged sooner or later. Perhaps, it's a good idea to #define IS_SWAPFILE(a) 0 than killing them entirely..?

Thanks,

Quoted 13 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
fs/nfs/file.c out_swapfile: · line 579
Quoted 8 lines
fs/nfs/file.c +4 -0
+
+  return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+
 static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
 {
  struct inode *inode = filp->f_mapping->host;
Otherwise it looks fine...

Signature · 5 lines

Suresh Jayaraman
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Trond Myklebust

2009-04-20 14:21 UTC #8 lore raw

Quoted 56 lines · Suresh Jayaraman

Quoted 46 lines · Trond Myklebust

Quoted 45 lines · Suresh Jayaraman

Hi Trond,

Do you think this patch is OK? Can this be considered for merging?

Thanks,

Quoted 39 lines

fs/nfs/file.c +17 -0

index 90f292b..13d6a00 100644
@@ -47,6 +47,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
                  size_t count, unsigned int flags);
 static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
              unsigned long nr_segs, loff_t pos);
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+                  struct file *filp, loff_t *ppos,
+                  size_t count, unsigned int flags);
 static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
              unsigned long nr_segs, loff_t pos);
 static int nfs_file_flush(struct file *, fl_owner_t id);

@@ -76,6 +79,7 @@ const struct file_operations nfs_file_operations = {
  .lock      = nfs_lock,
  .flock     = nfs_flock,
  .splice_read  = nfs_file_splice_read,
+  .splice_write  = nfs_file_splice_write,
  .check_flags  = nfs_check_flags,
  .setlease    = nfs_setlease,
 };

@@ -550,6 +554,26 @@ out_swapfile:
  goto out;
 }
 
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+                 struct file *filp, loff_t *ppos,
+                 size_t count, unsigned int flags)
+{
+  struct dentry *dentry = filp->f_path.dentry;
+  struct inode *inode = dentry->d_inode;
+
+  dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+      dentry->d_parent->d_name.name, dentry->d_name.name,
+      (unsigned long) count, (unsigned long long) *ppos);
+
+  if (IS_SWAPFILE(inode)) {
+      printk(KERN_INFO "NFS: attempt to write to active swap"

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
fs/nfs/file.c out_swapfile: · line 572
Quoted 3 lines
fs/nfs/file.c +3 -0
+          "file!\n");
+      return -EBUSY;
+  }
I don't know that we really need this. We should sweep through the NFS code and kill all those IS_SWAPFILE() thingys. Or at least #define IS_SWAPFILE(a) (0)
...

Quoted 6 lines

Hmm.. I'm not sure whether we should kill them now. I think originally, these were added keeping in mind the future NFS swap support. Given that the recent work from Peterz Zilstra on "Swap over NFS" and multiple iterations/review on the same, I think those patches will eventually get merged sooner or later. Perhaps, it's a good idea to #define IS_SWAPFILE(a) 0 than killing them entirely..?

Why are they needed at all? AFAICS, other filesystems check IS_SWAPFILE when truncating a file, but don't litter their code with all these weird checks for writing, reading, etc.
It's not as if these checks can stop a determined privileged person from writing to the swapfile anyway. All they have to do is go to another client or write directly to the file on the server...

Trond

Suresh Jayaraman

author

2009-04-20 15:47 UTC #9 lore raw

Quoted 23 lines · Trond Myklebust

Quoted 15 lines · Suresh Jayaraman
Quoted 10 lines · Trond Myklebust
Quoted 9 lines · Suresh Jayaraman
Hi Trond,
Do you think this patch is OK? Can this be considered for merging?
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
(quoted diff) +6 -0
+
+  if (IS_SWAPFILE(inode)) {
+      printk(KERN_INFO "NFS: attempt to write to active swap"
+          "file!\n");
+      return -EBUSY;
+  }
Quoted 4 lines
I don't know that we really need this. We should sweep through the NFS code and kill all those IS_SWAPFILE() thingys. Or at least #define IS_SWAPFILE(a) (0)
...
Quoted 6 lines
Hmm.. I'm not sure whether we should kill them now. I think originally, these were added keeping in mind the future NFS swap support. Given that the recent work from Peterz Zilstra on "Swap over NFS" and multiple iterations/review on the same, I think those patches will eventually get merged sooner or later. Perhaps, it's a good idea to #define IS_SWAPFILE(a) 0 than killing them entirely..?

Quoted 3 lines

Why are they needed at all? AFAICS, other filesystems check IS_SWAPFILE when truncating a file, but don't litter their code with all these weird checks for writing, reading, etc.

Yes, except for afs other filesystems seem to check only during truncation.

Quoted 4 lines

It's not as if these checks can stop a determined privileged person from writing to the swapfile anyway. All they have to do is go to another client or write directly to the file on the server...

Make sense.

Here is the trimmed version. I think the dprink could be retained for debugging issues, if any.

Signed-off-by Suresh Jayaraman <[email protected]>

---
 fs/nfs/file.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

fs/nfs/file.c +17 -0

index 5a97bcf..6dfe7df 100644
@@ -48,6 +48,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
 					size_t count, unsigned int flags);
 static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+					struct file *filp, loff_t *ppos,
+					size_t count, unsigned int flags);
 static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
 static int  nfs_file_flush(struct file *, fl_owner_t id);

@@ -73,6 +76,7 @@ const struct file_operations nfs_file_operations = {
 	.lock		= nfs_lock,
 	.flock		= nfs_flock,
 	.splice_read	= nfs_file_splice_read,
+	.splice_write	= nfs_file_splice_write,
 	.check_flags	= nfs_check_flags,
 	.setlease	= nfs_setlease,
 };

@@ -587,6 +591,19 @@ out_swapfile:
 	goto out;
 }
 
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+				     struct file *filp, loff_t *ppos,
+				     size_t count, unsigned int flags)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+
+	dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+		dentry->d_parent->d_name.name, dentry->d_name.name,
+		(unsigned long) count, (unsigned long long) *ppos);
+
+	return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+
 static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = filp->f_mapping->host;

Christoph Hellwig

2009-04-21 14:48 UTC #10 lore raw

fs/nfs/file.c out_swapfile: · line 606

Quoted 13 lines · Suresh Jayaraman

fs/nfs/file.c +13 -0

+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+				     struct file *filp, loff_t *ppos,
+				     size_t count, unsigned int flags)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+
+	dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+		dentry->d_parent->d_name.name, dentry->d_name.name,
+		(unsigned long) count, (unsigned long long) *ppos);
+
+	return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+

You need all calls from nfs_file_write, too:

- most importantly the nfs_revalidate_file_size for O_APPEND
- the nfs_do_fsync for sync writes
- probably the stats increment

Trond Myklebust

2009-04-21 17:35 UTC #11 lore raw

Quoted 17 lines · Christoph Hellwig

fs/nfs/file.c out_swapfile: · line 606

Quoted 13 lines · Suresh Jayaraman

fs/nfs/file.c +13 -0

+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+				     struct file *filp, loff_t *ppos,
+				     size_t count, unsigned int flags)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+
+	dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+		dentry->d_parent->d_name.name, dentry->d_name.name,
+		(unsigned long) count, (unsigned long long) *ppos);
+
+	return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+

You need all calls from nfs_file_write, too:

Quoted 1 line

- most importantly the nfs_revalidate_file_size for O_APPEND

Isn't O_APPEND illegal for splice_write()? It looks like it is from a quick perusal of do_splice_from().

Quoted 1 line

- the nfs_do_fsync for sync writes

generic_file_splice_write() calls generic_osync_inode(), which should ensure sync writes even with NFS.
The one thing it won't do is propagate NFS write errors back to the caller. If we do care about this, then we should certainly test for nfs_need_sync_write() and then call nfs_do_fsync() (see nfs_file_write()).

Quoted 1 line

- probably the stats increment

We should talk to Chuck about this.

Cheers
Trond

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Chuck Lever

2009-04-21 18:48 UTC #12 lore raw

Quoted 32 lines · Trond Myklebust

Quoted 17 lines · Christoph Hellwig
fs/nfs/file.c out_swapfile: · line 606
Quoted 13 lines · Suresh Jayaraman
fs/nfs/file.c +13 -0
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+				     struct file *filp, loff_t *ppos,
+				     size_t count, unsigned int flags)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+
+	dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+		dentry->d_parent->d_name.name, dentry->d_name.name,
+		(unsigned long) count, (unsigned long long) *ppos);
+
+	return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+
You need all calls from nfs_file_write, too:
Quoted 1 line
- most importantly the nfs_revalidate_file_size for O_APPEND
Isn't O_APPEND illegal for splice_write()? It looks like it is from a quick perusal of do_splice_from().
Quoted 1 line
- the nfs_do_fsync for sync writes
generic_file_splice_write() calls generic_osync_inode(), which should ensure sync writes even with NFS.
The one thing it won't do is propagate NFS write errors back to the caller. If we do care about this, then we should certainly test for nfs_need_sync_write() and then call nfs_do_fsync() (see nfs_file_write()).

Quoted 3 lines

Quoted 1 line
- probably the stats increment
We should talk to Chuck about this.

I don't know much about the characteristics of splice writes, so you can do one of two things:

1. Create a separate BYTESWRITTEN counter for these
2. Lump them into NORMALBYTESWRITTEN if these are effectively cached writes.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Suresh Jayaraman

author

2009-04-22 08:33 UTC #13 lore raw

In reply to Trond Myklebust

Quoted 17 lines · Trond Myklebust

Quoted 16 lines · Christoph Hellwig

Review

Quoted 13 lines · Suresh Jayaraman

(quoted diff) +13 -0

+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+				     struct file *filp, loff_t *ppos,
+				     size_t count, unsigned int flags)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+
+	dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+		dentry->d_parent->d_name.name, dentry->d_name.name,
+		(unsigned long) count, (unsigned long long) *ppos);
+
+	return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+

You need all calls from nfs_file_write, too:

Quoted 4 lines

Quoted 1 line
- most importantly the nfs_revalidate_file_size for O_APPEND
Isn't O_APPEND illegal for splice_write()? It looks like it is from a quick perusal of do_splice_from().

Yes, I would also think so and also from upstream commit efc968d450e013049a662d22727cf132618dcb2f.
I've added a little comment to make his clear.

Quoted 8 lines

Quoted 1 line
- the nfs_do_fsync for sync writes
generic_file_splice_write() calls generic_osync_inode(), which should ensure sync writes even with NFS.
The one thing it won't do is propagate NFS write errors back to the caller. If we do care about this, then we should certainly test for nfs_need_sync_write() and then call nfs_do_fsync() (see nfs_file_write()).

I think it makes sense to propagate NFS write errors back.

Quoted 4 lines

Quoted 1 line
- probably the stats increment
We should talk to Chuck about this.

Have added it to NORMALBYTESWRITTEN based on Chuck's suggestion.

Here is the updated patch. Does this look OK?

Signed-off-by Suresh Jayaraman <[email protected]>

---
 fs/nfs/file.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

fs/nfs/file.c +32 -0

index 5a97bcf..6f2cd67 100644
@@ -48,6 +48,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
 					size_t count, unsigned int flags);
 static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+					struct file *filp, loff_t *ppos,
+					size_t count, unsigned int flags);
 static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
 static int  nfs_file_flush(struct file *, fl_owner_t id);

@@ -73,6 +76,7 @@ const struct file_operations nfs_file_operations = {
 	.lock		= nfs_lock,
 	.flock		= nfs_flock,
 	.splice_read	= nfs_file_splice_read,
+	.splice_write	= nfs_file_splice_write,
 	.check_flags	= nfs_check_flags,
 	.setlease	= nfs_setlease,
 };

@@ -587,6 +591,34 @@ out_swapfile:
 	goto out;
 }
 
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+				     struct file *filp, loff_t *ppos,
+				     size_t count, unsigned int flags)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+	ssize_t ret;
+
+	dprintk("NFS splice_write(%s/%s, %lu@%llu)\n",
+		dentry->d_parent->d_name.name, dentry->d_name.name,
+		(unsigned long) count, (unsigned long long) *ppos);
+
+	/*
+	 * We don't need to worry about O_APPEND as the combination of splice
+	 * and an O_APPEND destination is disallowed.
+	 */
+
+	nfs_add_stats(inode, NFSIOS_NORMALWRITTENBYTES, count);
+
+	ret = generic_file_splice_write(pipe, filp, ppos, count, flags);
+	if (ret >= 0 && nfs_need_sync_write(filp, inode)) {
+		int err = nfs_do_fsync(nfs_file_open_context(filp), inode);
+		if (err < 0)
+			ret = err;
+	}
+	return ret;
+}
+
 static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode *inode = filp->f_mapping->host;

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html