On Mon, Jan 21, 2008 at 02:31:05PM -0500, Trond Myklebust wrote:
>
> On Mon, 2008-01-21 at 14:06 -0500, J. Bruce Fields wrote:
> > > > > Commands to reproduce this problem on the client:
> > > > >
> > > > > dd </dev/zero >testfile count=1
> > > > > ls -l --time=atime --full-time testfile
> > > > > sleep 2
> > > > > wc testfile
> > > > > ls -l --time=atime --full-time testfile
> > > > > (same atime, not good)
> > > >
> > > > Can you confirm that it does not change on the server? If so, then we
> > > > need to look at the server for a fix. The client should only be
> > > > mirroring the server's idea of the correct atime.
> > >
> > > It doesn't change on the server (2.6.23.12)
> >
> > That still leaves open the question as to whether this is due to changes
> > in the client that are causing it not to issue a read to the server when
> > it would have before, or whether the server is just refusing to update
> > the atime on read for some reason....
>
> A script of the form
"rm -f testfile" and at least one "touch ." in advance to get deterministic behavior.
>
> ssh server 'dd </dev/zero >testfile count=1; ls -l --time=atime --full-time testfile'
put a sleep 2 here.
>
> cat testfile >/dev/null
put a sleep 2 here just in case we would race with something in flight
>
> ssh server 'ls -l --time=atime --full-time testfile'
> ls -l --time=atime --full-time testfile
>
I tried some kernels (same on client and server). There are three atimes
after each experiment.
2.6.21.7:
t
t + 2
t + 2
This seems to work but it doesn't: only the first "cat testfile" will
update the atime. Subsequent cat commands will not update atime unless
"cat" is executed on the server.
2.6.22.10:
t
t + 2
t + 2
(same behavior)
2.6.23.12:
t
t
t
definately not good. "cat" on the server updates atime again.
Trying a different combination of kernels:
server 2.6.23.12, client 2.6.22.10:
t
t
t
server 2.6.22.10, client 2.6.23.12:
t
t + 2
t + 2
so, there are 2 phenomena and one has been introduced with 2.6.23 on
the server side. Filesystem on the server is ext3.
--
Frank
On Mon, Jan 28, 2008 at 09:59:40PM -0500, bfields wrote:
> On Tue, Jan 22, 2008 at 12:17:17PM -0500, bfields wrote:
> > On Mon, Jan 21, 2008 at 04:09:25PM -0500, bfields wrote:
> > > On Mon, Jan 21, 2008 at 10:02:30PM +0100, Frank van Maarseveen wrote:
> > > > 2.6.22.10:
> > > > t
> > > > t + 2
> > > > t + 2
> > > >
> > > > (same behavior)
> > > >
> > > > 2.6.23.12:
> > > > t
> > > > t
> > > > t
> > > >
> > > > definately not good. "cat" on the server updates atime again.
> > >
> > > Yes, that looks like a server bug, and this:...
> > >
> > > >
> > > >
> > > > Trying a different combination of kernels:
> > > >
> > > > server 2.6.23.12, client 2.6.22.10:
> > > > t
> > > > t
> > > > t
> > > >
> > > > server 2.6.22.10, client 2.6.23.12:
> > > > t
> > > > t + 2
> > > > t + 2
> > >
> > > ...confirms that since the results appear to depend only on the server
> > > version, not on the client version.
> >
> > And I can confirm this here on 2.6.24-rc8 (+ a few patches).
> > Unfortunately, I don't have any suggestion better right now than
> > bisecting....
>
> It looks like this happened in the switch from sendfile to sparse.
^^^^^^
err, splice
> Jens, any advice? What happened was nfsd reads stopped updating the
> atime after the following commit.
Erm, sorry, wrong commit--it's the following one that touches nfsd,
below.
--b.
commit cf8208d0eabd1d5d2625ec02a175a294c3f30d36
Author: Jens Axboe <[email protected]>
Date: Tue Jun 12 21:22:14 2007 +0200
sendfile: convert nfsd to splice_direct_to_actor()
Signed-off-by: Jens Axboe <[email protected]>
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 7e6aa24..15471a9 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -23,7 +23,7 @@
#include <linux/file.h>
#include <linux/mount.h>
#include <linux/major.h>
-#include <linux/ext2_fs.h>
+#include <linux/pipe_fs_i.h>
#include <linux/proc_fs.h>
#include <linux/stat.h>
#include <linux/fcntl.h>
@@ -801,26 +801,32 @@ found:
}
/*
- * Grab and keep cached pages assosiated with a file in the svc_rqst
- * so that they can be passed to the netowork sendmsg/sendpage routines
- * directrly. They will be released after the sending has completed.
+ * Grab and keep cached pages associated with a file in the svc_rqst
+ * so that they can be passed to the network sendmsg/sendpage routines
+ * directly. They will be released after the sending has completed.
*/
static int
-nfsd_read_actor(read_descriptor_t *desc, struct page *page, unsigned long offset , unsigned long size)
+nfsd_splice_actor(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
+ struct splice_desc *sd)
{
- unsigned long count = desc->count;
- struct svc_rqst *rqstp = desc->arg.data;
+ struct svc_rqst *rqstp = sd->u.data;
struct page **pp = rqstp->rq_respages + rqstp->rq_resused;
+ struct page *page = buf->page;
+ size_t size;
+ int ret;
+
+ ret = buf->ops->pin(pipe, buf);
+ if (unlikely(ret))
+ return ret;
- if (size > count)
- size = count;
+ size = sd->len;
if (rqstp->rq_res.page_len == 0) {
get_page(page);
put_page(*pp);
*pp = page;
rqstp->rq_resused++;
- rqstp->rq_res.page_base = offset;
+ rqstp->rq_res.page_base = buf->offset;
rqstp->rq_res.page_len = size;
} else if (page != pp[-1]) {
get_page(page);
@@ -832,11 +838,15 @@ nfsd_read_actor(read_descriptor_t *desc, struct page *page, unsigned long offset
} else
rqstp->rq_res.page_len += size;
- desc->count = count - size;
- desc->written += size;
return size;
}
+static int nfsd_direct_splice_actor(struct pipe_inode_info *pipe,
+ struct splice_desc *sd)
+{
+ return __splice_from_pipe(pipe, sd, nfsd_splice_actor);
+}
+
static __be32
nfsd_vfs_read(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
loff_t offset, struct kvec *vec, int vlen, unsigned long *count)
@@ -861,10 +871,15 @@ nfsd_vfs_read(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
if (ra && ra->p_set)
file->f_ra = ra->p_ra;
- if (file->f_op->sendfile && rqstp->rq_sendfile_ok) {
- rqstp->rq_resused = 1;
- host_err = file->f_op->sendfile(file, &offset, *count,
- nfsd_read_actor, rqstp);
+ if (file->f_op->splice_read && rqstp->rq_splice_ok) {
+ struct splice_desc sd = {
+ .len = 0,
+ .total_len = *count,
+ .pos = offset,
+ .u.data = rqstp,
+ };
+
+ host_err = splice_direct_to_actor(file, &sd, nfsd_direct_splice_actor);
} else {
oldfs = get_fs();
set_fs(KERNEL_DS);
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 4a7ae8a..129d50f 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -253,7 +253,7 @@ struct svc_rqst {
* determine what device number
* to report (real or virtual)
*/
- int rq_sendfile_ok; /* turned off in gss privacy
+ int rq_splice_ok; /* turned off in gss privacy
* to prevent encrypting page
* cache pages */
wait_queue_head_t rq_wait; /* synchronization */
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 099a983..c094583 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -853,7 +853,7 @@ unwrap_priv_data(struct svc_rqst *rqstp, struct xdr_buf *buf, u32 seq, struct gs
u32 priv_len, maj_stat;
int pad, saved_len, remaining_len, offset;
- rqstp->rq_sendfile_ok = 0;
+ rqstp->rq_splice_ok = 0;
priv_len = svc_getnl(&buf->head[0]);
if (rqstp->rq_deferred) {
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index e673ef9..55ea6df 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -814,7 +814,7 @@ svc_process(struct svc_rqst *rqstp)
rqstp->rq_res.tail[0].iov_base = NULL;
rqstp->rq_res.tail[0].iov_len = 0;
/* Will be turned off only in gss privacy case: */
- rqstp->rq_sendfile_ok = 1;
+ rqstp->rq_splice_ok = 1;
/* tcp needs a space for the record length... */
if (rqstp->rq_prot == IPPROTO_TCP)
svc_putnl(resv, 0);
On Mon, Jan 28 2008, J. Bruce Fields wrote:
> On Mon, Jan 28, 2008 at 09:59:40PM -0500, bfields wrote:
> > On Tue, Jan 22, 2008 at 12:17:17PM -0500, bfields wrote:
> > > On Mon, Jan 21, 2008 at 04:09:25PM -0500, bfields wrote:
> > > > On Mon, Jan 21, 2008 at 10:02:30PM +0100, Frank van Maarseveen wrote:
> > > > > 2.6.22.10:
> > > > > t
> > > > > t + 2
> > > > > t + 2
> > > > >
> > > > > (same behavior)
> > > > >
> > > > > 2.6.23.12:
> > > > > t
> > > > > t
> > > > > t
> > > > >
> > > > > definately not good. "cat" on the server updates atime again.
> > > >
> > > > Yes, that looks like a server bug, and this:...
> > > >
> > > > >
> > > > >
> > > > > Trying a different combination of kernels:
> > > > >
> > > > > server 2.6.23.12, client 2.6.22.10:
> > > > > t
> > > > > t
> > > > > t
> > > > >
> > > > > server 2.6.22.10, client 2.6.23.12:
> > > > > t
> > > > > t + 2
> > > > > t + 2
> > > >
> > > > ...confirms that since the results appear to depend only on the server
> > > > version, not on the client version.
> > >
> > > And I can confirm this here on 2.6.24-rc8 (+ a few patches).
> > > Unfortunately, I don't have any suggestion better right now than
> > > bisecting....
> >
> > It looks like this happened in the switch from sendfile to sparse.
> ^^^^^^
> err, splice
>
> > Jens, any advice? What happened was nfsd reads stopped updating the
> > atime after the following commit.
>
> Erm, sorry, wrong commit--it's the following one that touches nfsd,
> below.
Probably because do_generic_mapping_read() does a file_accessed() on the
input file. Does this fixup current -git?
diff --git a/fs/splice.c b/fs/splice.c
index 56b802b..c212fde 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1090,8 +1090,10 @@ long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
long ret;
ret = splice_direct_to_actor(in, &sd, direct_splice_actor);
- if (ret > 0)
+ if (ret > 0) {
*ppos += ret;
+ file_accessed(in);
+ }
return ret;
}
--
Jens Axboe
On Tue, Jan 29, 2008 at 09:34:02AM +0100, Jens Axboe wrote:
> On Mon, Jan 28 2008, J. Bruce Fields wrote:
> > On Mon, Jan 28, 2008 at 09:59:40PM -0500, bfields wrote:
> > > On Tue, Jan 22, 2008 at 12:17:17PM -0500, bfields wrote:
> > > > On Mon, Jan 21, 2008 at 04:09:25PM -0500, bfields wrote:
> > > > > On Mon, Jan 21, 2008 at 10:02:30PM +0100, Frank van Maarseveen wrote:
> > > > > > 2.6.22.10:
> > > > > > t
> > > > > > t + 2
> > > > > > t + 2
> > > > > >
> > > > > > (same behavior)
> > > > > >
> > > > > > 2.6.23.12:
> > > > > > t
> > > > > > t
> > > > > > t
> > > > > >
> > > > > > definately not good. "cat" on the server updates atime again.
> > > > >
> > > > > Yes, that looks like a server bug, and this:...
> > > > >
> > > > > >
> > > > > >
> > > > > > Trying a different combination of kernels:
> > > > > >
> > > > > > server 2.6.23.12, client 2.6.22.10:
> > > > > > t
> > > > > > t
> > > > > > t
> > > > > >
> > > > > > server 2.6.22.10, client 2.6.23.12:
> > > > > > t
> > > > > > t + 2
> > > > > > t + 2
> > > > >
> > > > > ...confirms that since the results appear to depend only on the server
> > > > > version, not on the client version.
> > > >
> > > > And I can confirm this here on 2.6.24-rc8 (+ a few patches).
> > > > Unfortunately, I don't have any suggestion better right now than
> > > > bisecting....
> > >
> > > It looks like this happened in the switch from sendfile to sparse.
> > ^^^^^^
> > err, splice
> >
> > > Jens, any advice? What happened was nfsd reads stopped updating the
> > > atime after the following commit.
> >
> > Erm, sorry, wrong commit--it's the following one that touches nfsd,
> > below.
>
> Probably because do_generic_mapping_read() does a file_accessed() on the
> input file. Does this fixup current -git?
No, nfsd is calling splice_direct_to_actor(), not do_splice_direct().
--b.
>
> diff --git a/fs/splice.c b/fs/splice.c
> index 56b802b..c212fde 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1090,8 +1090,10 @@ long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
> long ret;
>
> ret = splice_direct_to_actor(in, &sd, direct_splice_actor);
> - if (ret > 0)
> + if (ret > 0) {
> *ppos += ret;
> + file_accessed(in);
> + }
>
> return ret;
> }
>
> --
> Jens Axboe
>
On Tue, Jan 29 2008, J. Bruce Fields wrote:
> On Tue, Jan 29, 2008 at 09:34:02AM +0100, Jens Axboe wrote:
> > On Mon, Jan 28 2008, J. Bruce Fields wrote:
> > > On Mon, Jan 28, 2008 at 09:59:40PM -0500, bfields wrote:
> > > > On Tue, Jan 22, 2008 at 12:17:17PM -0500, bfields wrote:
> > > > > On Mon, Jan 21, 2008 at 04:09:25PM -0500, bfields wrote:
> > > > > > On Mon, Jan 21, 2008 at 10:02:30PM +0100, Frank van Maarseveen wrote:
> > > > > > > 2.6.22.10:
> > > > > > > t
> > > > > > > t + 2
> > > > > > > t + 2
> > > > > > >
> > > > > > > (same behavior)
> > > > > > >
> > > > > > > 2.6.23.12:
> > > > > > > t
> > > > > > > t
> > > > > > > t
> > > > > > >
> > > > > > > definately not good. "cat" on the server updates atime again.
> > > > > >
> > > > > > Yes, that looks like a server bug, and this:...
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Trying a different combination of kernels:
> > > > > > >
> > > > > > > server 2.6.23.12, client 2.6.22.10:
> > > > > > > t
> > > > > > > t
> > > > > > > t
> > > > > > >
> > > > > > > server 2.6.22.10, client 2.6.23.12:
> > > > > > > t
> > > > > > > t + 2
> > > > > > > t + 2
> > > > > >
> > > > > > ...confirms that since the results appear to depend only on the server
> > > > > > version, not on the client version.
> > > > >
> > > > > And I can confirm this here on 2.6.24-rc8 (+ a few patches).
> > > > > Unfortunately, I don't have any suggestion better right now than
> > > > > bisecting....
> > > >
> > > > It looks like this happened in the switch from sendfile to sparse.
> > > ^^^^^^
> > > err, splice
> > >
> > > > Jens, any advice? What happened was nfsd reads stopped updating the
> > > > atime after the following commit.
> > >
> > > Erm, sorry, wrong commit--it's the following one that touches nfsd,
> > > below.
> >
> > Probably because do_generic_mapping_read() does a file_accessed() on the
> > input file. Does this fixup current -git?
>
> No, nfsd is calling splice_direct_to_actor(), not do_splice_direct().
Ah doh, this instead then. Should work for both types.
diff --git a/fs/splice.c b/fs/splice.c
index 0a0b79b..504a096 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1052,8 +1052,10 @@ out_release:
/*
* If we transferred some data, return the number of bytes:
*/
- if (bytes > 0)
+ if (bytes > 0) {
+ file_accessed(in);
return bytes;
+ }
return ret;
--
Jens Axboe
On Tue, Jan 29, 2008 at 07:30:42PM +0100, Jens Axboe wrote:
> On Tue, Jan 29 2008, J. Bruce Fields wrote:
> > On Tue, Jan 29, 2008 at 09:34:02AM +0100, Jens Axboe wrote:
> > > On Mon, Jan 28 2008, J. Bruce Fields wrote:
> > > > On Mon, Jan 28, 2008 at 09:59:40PM -0500, bfields wrote:
> > > > > On Tue, Jan 22, 2008 at 12:17:17PM -0500, bfields wrote:
> > > > > > On Mon, Jan 21, 2008 at 04:09:25PM -0500, bfields wrote:
> > > > > > > On Mon, Jan 21, 2008 at 10:02:30PM +0100, Frank van Maarseveen wrote:
> > > > > > > > 2.6.22.10:
> > > > > > > > t
> > > > > > > > t + 2
> > > > > > > > t + 2
> > > > > > > >
> > > > > > > > (same behavior)
> > > > > > > >
> > > > > > > > 2.6.23.12:
> > > > > > > > t
> > > > > > > > t
> > > > > > > > t
> > > > > > > >
> > > > > > > > definately not good. "cat" on the server updates atime again.
> > > > > > >
> > > > > > > Yes, that looks like a server bug, and this:...
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Trying a different combination of kernels:
> > > > > > > >
> > > > > > > > server 2.6.23.12, client 2.6.22.10:
> > > > > > > > t
> > > > > > > > t
> > > > > > > > t
> > > > > > > >
> > > > > > > > server 2.6.22.10, client 2.6.23.12:
> > > > > > > > t
> > > > > > > > t + 2
> > > > > > > > t + 2
> > > > > > >
> > > > > > > ...confirms that since the results appear to depend only on the server
> > > > > > > version, not on the client version.
> > > > > >
> > > > > > And I can confirm this here on 2.6.24-rc8 (+ a few patches).
> > > > > > Unfortunately, I don't have any suggestion better right now than
> > > > > > bisecting....
> > > > >
> > > > > It looks like this happened in the switch from sendfile to sparse.
> > > > ^^^^^^
> > > > err, splice
> > > >
> > > > > Jens, any advice? What happened was nfsd reads stopped updating the
> > > > > atime after the following commit.
> > > >
> > > > Erm, sorry, wrong commit--it's the following one that touches nfsd,
> > > > below.
> > >
> > > Probably because do_generic_mapping_read() does a file_accessed() on the
> > > input file. Does this fixup current -git?
> >
> > No, nfsd is calling splice_direct_to_actor(), not do_splice_direct().
>
> Ah doh, this instead then. Should work for both types.
>
> diff --git a/fs/splice.c b/fs/splice.c
> index 0a0b79b..504a096 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1052,8 +1052,10 @@ out_release:
> /*
> * If we transferred some data, return the number of bytes:
> */
> - if (bytes > 0)
> + if (bytes > 0) {
> + file_accessed(in);
> return bytes;
> + }
>
> return ret;
Hm. It's still missing a case. I've confirmed that the following fixes
the problem. (Or maybe it would be better to have them "goto" a common
out with the file_accessed() check?)
--b.
diff --git a/fs/splice.c b/fs/splice.c
index 6bdcb61..8a832d6 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1033,6 +1033,8 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
goto out_release;
}
+ if (bytes)
+ file_accessed(in);
pipe->nrbufs = pipe->curbuf = 0;
return bytes;
@@ -1054,8 +1056,10 @@ out_release:
/*
* If we transferred some data, return the number of bytes:
*/
- if (bytes > 0)
+ if (bytes > 0) {
+ file_accessed(in);
return bytes;
+ }
return ret;
On Tue, Jan 29 2008, J. Bruce Fields wrote:
> On Tue, Jan 29, 2008 at 07:30:42PM +0100, Jens Axboe wrote:
> > On Tue, Jan 29 2008, J. Bruce Fields wrote:
> > > On Tue, Jan 29, 2008 at 09:34:02AM +0100, Jens Axboe wrote:
> > > > On Mon, Jan 28 2008, J. Bruce Fields wrote:
> > > > > On Mon, Jan 28, 2008 at 09:59:40PM -0500, bfields wrote:
> > > > > > On Tue, Jan 22, 2008 at 12:17:17PM -0500, bfields wrote:
> > > > > > > On Mon, Jan 21, 2008 at 04:09:25PM -0500, bfields wrote:
> > > > > > > > On Mon, Jan 21, 2008 at 10:02:30PM +0100, Frank van Maarseveen wrote:
> > > > > > > > > 2.6.22.10:
> > > > > > > > > t
> > > > > > > > > t + 2
> > > > > > > > > t + 2
> > > > > > > > >
> > > > > > > > > (same behavior)
> > > > > > > > >
> > > > > > > > > 2.6.23.12:
> > > > > > > > > t
> > > > > > > > > t
> > > > > > > > > t
> > > > > > > > >
> > > > > > > > > definately not good. "cat" on the server updates atime again.
> > > > > > > >
> > > > > > > > Yes, that looks like a server bug, and this:...
> > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Trying a different combination of kernels:
> > > > > > > > >
> > > > > > > > > server 2.6.23.12, client 2.6.22.10:
> > > > > > > > > t
> > > > > > > > > t
> > > > > > > > > t
> > > > > > > > >
> > > > > > > > > server 2.6.22.10, client 2.6.23.12:
> > > > > > > > > t
> > > > > > > > > t + 2
> > > > > > > > > t + 2
> > > > > > > >
> > > > > > > > ...confirms that since the results appear to depend only on the server
> > > > > > > > version, not on the client version.
> > > > > > >
> > > > > > > And I can confirm this here on 2.6.24-rc8 (+ a few patches).
> > > > > > > Unfortunately, I don't have any suggestion better right now than
> > > > > > > bisecting....
> > > > > >
> > > > > > It looks like this happened in the switch from sendfile to sparse.
> > > > > ^^^^^^
> > > > > err, splice
> > > > >
> > > > > > Jens, any advice? What happened was nfsd reads stopped updating the
> > > > > > atime after the following commit.
> > > > >
> > > > > Erm, sorry, wrong commit--it's the following one that touches nfsd,
> > > > > below.
> > > >
> > > > Probably because do_generic_mapping_read() does a file_accessed() on the
> > > > input file. Does this fixup current -git?
> > >
> > > No, nfsd is calling splice_direct_to_actor(), not do_splice_direct().
> >
> > Ah doh, this instead then. Should work for both types.
> >
> > diff --git a/fs/splice.c b/fs/splice.c
> > index 0a0b79b..504a096 100644
> > --- a/fs/splice.c
> > +++ b/fs/splice.c
> > @@ -1052,8 +1052,10 @@ out_release:
> > /*
> > * If we transferred some data, return the number of bytes:
> > */
> > - if (bytes > 0)
> > + if (bytes > 0) {
> > + file_accessed(in);
> > return bytes;
> > + }
> >
> > return ret;
>
> Hm. It's still missing a case. I've confirmed that the following fixes
> the problem. (Or maybe it would be better to have them "goto" a common
> out with the file_accessed() check?)
Indeed it is, insert standard disclaimer here on the evil of multiple
returns. So can we agree that this then fixes both cases?
diff --git a/fs/splice.c b/fs/splice.c
index 0a0b79b..1577a73 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1031,7 +1031,11 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
goto out_release;
}
+done:
pipe->nrbufs = pipe->curbuf = 0;
+ if (bytes > 0)
+ file_accessed(in);
+
return bytes;
out_release:
@@ -1047,16 +1051,11 @@ out_release:
buf->ops = NULL;
}
}
- pipe->nrbufs = pipe->curbuf = 0;
-
- /*
- * If we transferred some data, return the number of bytes:
- */
- if (bytes > 0)
- return bytes;
- return ret;
+ if (!bytes)
+ bytes = ret;
+ goto done;
}
EXPORT_SYMBOL(splice_direct_to_actor);
--
Jens Axboe
On Tue, Jan 29, 2008 at 08:51:55PM +0100, Jens Axboe wrote:
> On Tue, Jan 29 2008, J. Bruce Fields wrote:
> > Hm. It's still missing a case. I've confirmed that the following fixes
> > the problem. (Or maybe it would be better to have them "goto" a common
> > out with the file_accessed() check?)
>
> Indeed it is, insert standard disclaimer here on the evil of multiple
> returns. So can we agree that this then fixes both cases?
Yep, thanks! And I ran one more test just to make sure. Looks fine.
--b.
>
> diff --git a/fs/splice.c b/fs/splice.c
> index 0a0b79b..1577a73 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1031,7 +1031,11 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
> goto out_release;
> }
>
> +done:
> pipe->nrbufs = pipe->curbuf = 0;
> + if (bytes > 0)
> + file_accessed(in);
> +
> return bytes;
>
> out_release:
> @@ -1047,16 +1051,11 @@ out_release:
> buf->ops = NULL;
> }
> }
> - pipe->nrbufs = pipe->curbuf = 0;
> -
> - /*
> - * If we transferred some data, return the number of bytes:
> - */
> - if (bytes > 0)
> - return bytes;
>
> - return ret;
> + if (!bytes)
> + bytes = ret;
>
> + goto done;
> }
> EXPORT_SYMBOL(splice_direct_to_actor);
>
>
> --
> Jens Axboe
>
On Tue, Jan 29 2008, J. Bruce Fields wrote:
> On Tue, Jan 29, 2008 at 08:51:55PM +0100, Jens Axboe wrote:
> > On Tue, Jan 29 2008, J. Bruce Fields wrote:
> > > Hm. It's still missing a case. I've confirmed that the following fixes
> > > the problem. (Or maybe it would be better to have them "goto" a common
> > > out with the file_accessed() check?)
> >
> > Indeed it is, insert standard disclaimer here on the evil of multiple
> > returns. So can we agree that this then fixes both cases?
>
> Yep, thanks! And I ran one more test just to make sure. Looks fine.
Super, thanks a lot for testing and confirming! Patch is committed.
--
Jens Axboe
On 2008-01-29 19:30 +0100, Jens Axboe wrote:
> diff --git a/fs/splice.c b/fs/splice.c
> index 0a0b79b..504a096 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1052,8 +1052,10 @@ out_release:
> /*
> * If we transferred some data, return the number of bytes:
> */
> - if (bytes > 0)
> + if (bytes > 0) {
> + file_accessed(in);
> return bytes;
> + }
> =20
> return ret;
Does the (bytes > 0) test mean that read(2) on an empty file will
not update the atime ? Not a big deal for me personally, but that
would be inconsistent with local file system behaviour.
--=20
Andr=E9 Majorel <URL:http://www.teaser.fr/~amajorel/>
Do not use this account for regular correspondence.
See the URL above for contact information.
On Tue, Jan 22, 2008 at 12:17:17PM -0500, bfields wrote:
> On Mon, Jan 21, 2008 at 04:09:25PM -0500, bfields wrote:
> > On Mon, Jan 21, 2008 at 10:02:30PM +0100, Frank van Maarseveen wrote:
> > > 2.6.22.10:
> > > t
> > > t + 2
> > > t + 2
> > >
> > > (same behavior)
> > >
> > > 2.6.23.12:
> > > t
> > > t
> > > t
> > >
> > > definately not good. "cat" on the server updates atime again.
> >
> > Yes, that looks like a server bug, and this:...
> >
> > >
> > >
> > > Trying a different combination of kernels:
> > >
> > > server 2.6.23.12, client 2.6.22.10:
> > > t
> > > t
> > > t
> > >
> > > server 2.6.22.10, client 2.6.23.12:
> > > t
> > > t + 2
> > > t + 2
> >
> > ...confirms that since the results appear to depend only on the server
> > version, not on the client version.
>
> And I can confirm this here on 2.6.24-rc8 (+ a few patches).
> Unfortunately, I don't have any suggestion better right now than
> bisecting....
It looks like this happened in the switch from sendfile to sparse.
Jens, any advice? What happened was nfsd reads stopped updating the
atime after the following commit.
--b.
commit f0930fffa99e7fe0a0c4b6c7d9a244dc88288c27
Author: Jens Axboe <[email protected]>
Date: Fri Jun 1 11:51:43 2007 +0200
sendfile: convert nfs to using splice_read()
Acked-by: Trond Myklebust <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 9eb8eb4..8689b73 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -41,7 +41,9 @@ static int nfs_file_open(struct inode *, struct file *);
static int nfs_file_release(struct inode *, struct file *);
static loff_t nfs_file_llseek(struct file *file, loff_t offset, int origin);
static int nfs_file_mmap(struct file *, struct vm_area_struct *);
-static ssize_t nfs_file_sendfile(struct file *, loff_t *, size_t, read_actor_t, void *);
+static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
+ struct pipe_inode_info *pipe,
+ 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_write(struct kiocb *, const struct iovec *iov,
@@ -65,7 +67,7 @@ const struct file_operations nfs_file_operations = {
.fsync = nfs_fsync,
.lock = nfs_lock,
.flock = nfs_flock,
- .sendfile = nfs_file_sendfile,
+ .splice_read = nfs_file_splice_read,
.check_flags = nfs_check_flags,
};
@@ -224,20 +226,21 @@ nfs_file_read(struct kiocb *iocb, const struct iovec *iov,
}
static ssize_t
-nfs_file_sendfile(struct file *filp, loff_t *ppos, size_t count,
- read_actor_t actor, void *target)
+nfs_file_splice_read(struct file *filp, loff_t *ppos,
+ struct pipe_inode_info *pipe, size_t count,
+ unsigned int flags)
{
struct dentry *dentry = filp->f_path.dentry;
struct inode *inode = dentry->d_inode;
ssize_t res;
- dfprintk(VFS, "nfs: sendfile(%s/%s, %lu@%Lu)\n",
+ dfprintk(VFS, "nfs: splice_read(%s/%s, %lu@%Lu)\n",
dentry->d_parent->d_name.name, dentry->d_name.name,
(unsigned long) count, (unsigned long long) *ppos);
res = nfs_revalidate_mapping(inode, filp->f_mapping);
if (!res)
- res = generic_file_sendfile(filp, ppos, count, actor, target);
+ res = generic_file_splice_read(filp, ppos, pipe, count, flags);
return res;
}
On Mon, Jan 21, 2008 at 10:02:30PM +0100, Frank van Maarseveen wrote:
> On Mon, Jan 21, 2008 at 02:31:05PM -0500, Trond Myklebust wrote:
> >
> > On Mon, 2008-01-21 at 14:06 -0500, J. Bruce Fields wrote:
> > > > > > Commands to reproduce this problem on the client:
> > > > > >
> > > > > > dd </dev/zero >testfile count=1
> > > > > > ls -l --time=atime --full-time testfile
> > > > > > sleep 2
> > > > > > wc testfile
> > > > > > ls -l --time=atime --full-time testfile
> > > > > > (same atime, not good)
> > > > >
> > > > > Can you confirm that it does not change on the server? If so, then we
> > > > > need to look at the server for a fix. The client should only be
> > > > > mirroring the server's idea of the correct atime.
> > > >
> > > > It doesn't change on the server (2.6.23.12)
> > >
> > > That still leaves open the question as to whether this is due to changes
> > > in the client that are causing it not to issue a read to the server when
> > > it would have before, or whether the server is just refusing to update
> > > the atime on read for some reason....
> >
> > A script of the form
>
> "rm -f testfile" and at least one "touch ." in advance to get deterministic behavior.
>
> >
> > ssh server 'dd </dev/zero >testfile count=1; ls -l --time=atime --full-time testfile'
>
> put a sleep 2 here.
>
> >
> > cat testfile >/dev/null
>
> put a sleep 2 here just in case we would race with something in flight
>
> >
> > ssh server 'ls -l --time=atime --full-time testfile'
> > ls -l --time=atime --full-time testfile
> >
>
> I tried some kernels (same on client and server). There are three atimes
> after each experiment.
>
> 2.6.21.7:
> t
> t + 2
> t + 2
>
> This seems to work but it doesn't: only the first "cat testfile" will
> update the atime. Subsequent cat commands will not update atime unless
> "cat" is executed on the server.
I think that's just the expected result of client caching, so all you
can do in that case is add "set check_mbox_size=yes" to your .muttrc or
something.....
>
> 2.6.22.10:
> t
> t + 2
> t + 2
>
> (same behavior)
>
> 2.6.23.12:
> t
> t
> t
>
> definately not good. "cat" on the server updates atime again.
Yes, that looks like a server bug, and this:...
>
>
> Trying a different combination of kernels:
>
> server 2.6.23.12, client 2.6.22.10:
> t
> t
> t
>
> server 2.6.22.10, client 2.6.23.12:
> t
> t + 2
> t + 2
...confirms that since the results appear to depend only on the server
version, not on the client version.
--b.
>
> so, there are 2 phenomena and one has been introduced with 2.6.23 on
> the server side. Filesystem on the server is ext3.
>
> --
> Frank
On Mon, Jan 21, 2008 at 04:09:25PM -0500, bfields wrote:
> On Mon, Jan 21, 2008 at 10:02:30PM +0100, Frank van Maarseveen wrote:
> > "rm -f testfile" and at least one "touch ." in advance to get deterministic behavior.
> >
> > >
> > > ssh server 'dd </dev/zero >testfile count=1; ls -l --time=atime --full-time testfile'
> >
> > put a sleep 2 here.
> >
> > >
> > > cat testfile >/dev/null
> >
> > put a sleep 2 here just in case we would race with something in flight
> >
> > >
> > > ssh server 'ls -l --time=atime --full-time testfile'
> > > ls -l --time=atime --full-time testfile
> > >
> >
> > I tried some kernels (same on client and server). There are three atimes
> > after each experiment.
> >
> > 2.6.21.7:
> > t
> > t + 2
> > t + 2
> >
> > This seems to work but it doesn't: only the first "cat testfile" will
> > update the atime. Subsequent cat commands will not update atime unless
> > "cat" is executed on the server.
>
> I think that's just the expected result of client caching, so all you
> can do in that case is add "set check_mbox_size=yes" to your .muttrc or
> something.....
>
> >
> > 2.6.22.10:
> > t
> > t + 2
> > t + 2
> >
> > (same behavior)
> >
> > 2.6.23.12:
> > t
> > t
> > t
> >
> > definately not good. "cat" on the server updates atime again.
>
> Yes, that looks like a server bug, and this:...
>
> >
> >
> > Trying a different combination of kernels:
> >
> > server 2.6.23.12, client 2.6.22.10:
> > t
> > t
> > t
> >
> > server 2.6.22.10, client 2.6.23.12:
> > t
> > t + 2
> > t + 2
>
> ...confirms that since the results appear to depend only on the server
> version, not on the client version.
And I can confirm this here on 2.6.24-rc8 (+ a few patches).
Unfortunately, I don't have any suggestion better right now than
bisecting....
--b.