Re: atimes not updated over NFS

[Jens Axboe <jens.axboe@oracle.com>]

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