atimes not updated over NFS

atimes not updated over NFS

[Andre Majorel]

After upgrading an NFS server from kernel 2.6.12 to 2.6.23, the
atime of files is not updated any more when clients read them :

  client$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
  -rw-r--r-- 1 xxx xxx 0 2007-12-21_14:58:11 /nfsmnt/zz
  client$ md5sum /nfsmnt/zz
  d41d8cd98f00b204e9800998ecf8427e  /nfsmnt/zz
  client$ date +%Y-%m-%d_%H:%M:%S
  2007-12-21_15:00:28
  client$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
  -rw-r--r-- 1 xxx xxx 0 2007-12-21_14:58:11 /nfsmnt/zz

Reading the file locally, i.e. on the NFS server, does update the
atime :

  server$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
  -rw-r--r-- 1 xxx xxx 0 2007-12-21_14:58:11 /nfsmnt/zz
  serveur$ md5sum /nfsmnt/zz
  d41d8cd98f00b204e9800998ecf8427e  /nfsmnt/zz
  serveur$ date +%Y-%m-%d_%H:%M:%S
  2007-12-21_15:04:00
  serveur$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
  -rw-r--r-- 1 xxx xxx 0 2007-12-21_15:03:57 /nfsmnt/zz

/nfsmnt is mounted with options :
- server : rw,noexec,nosuid,nodev,nodiratime
- client : rw,user=3Droot,nosuid,nodev,nodiratime,intr,rsize=3D8192,wsi=
ze=3D8192

Any ideas ?

--=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.

Re: atimes not updated over NFS

[Trond Myklebust]

On Wed, 2008-01-09 at 11:13 +0100, Andre Majorel wrote:
> After upgrading an NFS server from kernel 2.6.12 to 2.6.23, the
> atime of files is not updated any more when clients read them :
> 
>   client$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
>   -rw-r--r-- 1 xxx xxx 0 2007-12-21_14:58:11 /nfsmnt/zz
>   client$ md5sum /nfsmnt/zz
>   d41d8cd98f00b204e9800998ecf8427e  /nfsmnt/zz
>   client$ date +%Y-%m-%d_%H:%M:%S
>   2007-12-21_15:00:28
>   client$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
>   -rw-r--r-- 1 xxx xxx 0 2007-12-21_14:58:11 /nfsmnt/zz
> 
> Reading the file locally, i.e. on the NFS server, does update the
> atime :
> 
>   server$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
>   -rw-r--r-- 1 xxx xxx 0 2007-12-21_14:58:11 /nfsmnt/zz
>   serveur$ md5sum /nfsmnt/zz
>   d41d8cd98f00b204e9800998ecf8427e  /nfsmnt/zz
>   serveur$ date +%Y-%m-%d_%H:%M:%S
>   2007-12-21_15:04:00
>   serveur$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
>   -rw-r--r-- 1 xxx xxx 0 2007-12-21_15:03:57 /nfsmnt/zz
> 
> /nfsmnt is mounted with options :
> - server : rw,noexec,nosuid,nodev,nodiratime
> - client : rw,user=root,nosuid,nodev,nodiratime,intr,rsize=8192,wsize=8192
> 
> Any ideas ?

The file is probably entirely in cache on the client. When this happens,
no READ rpc calls need to be made to the server and so the atime will
not change either.

This has always been the case, BTW...

Trond

Re: atimes not updated over NFS

[Andre Majorel]

On 2008-01-09 10:27 -0500, Trond Myklebust wrote:
> On Wed, 2008-01-09 at 11:13 +0100, Andre Majorel wrote:
>
> > After upgrading an NFS server from kernel 2.6.12 to 2.6.23, the
> > atime of files is not updated any more when clients read them :
> >=20
> >   client$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
> >   -rw-r--r-- 1 xxx xxx 0 2007-12-21_14:58:11 /nfsmnt/zz
> >   client$ md5sum /nfsmnt/zz
> >   d41d8cd98f00b204e9800998ecf8427e  /nfsmnt/zz
> >   client$ date +%Y-%m-%d_%H:%M:%S
> >   2007-12-21_15:00:28
> >   client$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
> >   -rw-r--r-- 1 xxx xxx 0 2007-12-21_14:58:11 /nfsmnt/zz
> >=20
> > Reading the file locally, i.e. on the NFS server, does update the
> > atime :
> >=20
> >   server$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
> >   -rw-r--r-- 1 xxx xxx 0 2007-12-21_14:58:11 /nfsmnt/zz
> >   serveur$ md5sum /nfsmnt/zz
> >   d41d8cd98f00b204e9800998ecf8427e  /nfsmnt/zz
> >   serveur$ date +%Y-%m-%d_%H:%M:%S
> >   2007-12-21_15:04:00
> >   serveur$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
> >   -rw-r--r-- 1 xxx xxx 0 2007-12-21_15:03:57 /nfsmnt/zz
> >=20
> > /nfsmnt is mounted with options :
> > - server : rw,noexec,nosuid,nodev,nodiratime
> > - client : rw,user=3Droot,nosuid,nodev,nodiratime,intr,rsize=3D8192=
,wsize=3D8192
> >=20
> > Any ideas ?
>=20
> The file is probably entirely in cache on the client. When this happe=
ns,
> no READ rpc calls need to be made to the server and so the atime will
> not change either.

The client has 512 MB of RAM. A 513-MB file cannot possibly be
cached so let's try with that.

  atc5:/aym/etc$ time dd if=3D/dev/zero bs=3D1M count=3D513 >/nfsmnt/zz
  513+0 records in
  513+0 records out

  real    0m50.412s
  user    0m0.004s
  sys     0m8.905s
  atc5:/aym/etc$ ls -l --time-style +%Y-%m-%d_%H:%M:%S -u /nfsmnt/zz
  -rw-r--r-- 1 aym aym 537919488 2007-12-21_13:46:54 /nfsmnt/zz
  atc5:/aym/etc$ time wc /nfsmnt/zz
          0         0 537919488 /nfsmnt/zz

  real    1m30.743s
  user    0m47.091s
  sys     0m22.369s
  atc5:/aym/etc$ date +%Y-%m-%d_%H:%M:%S
  2008-01-09_23:21:06
  atc5:/aym/etc$ ls -l --time-style +%Y-%m-%d_%H:%M:%S -u /nfsmnt/zz
  -rw-r--r-- 1 aym aym 537919488 2007-12-21_13:46:54 /nfsmnt/zz

As the times show, the file is not in cache. And yet the atime is
still not updated.

> This has always been the case, BTW...

I'll take your word for it. However, let me assure you that atimes
somehow used to work over NFS, even for files small enough to fit
in the cache. Had they been broken, I would have known. I use Mutt.

Thanks.

--=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.

Re: atimes not updated over NFS

[Trond Myklebust]

On Wed, 2008-01-09 at 23:37 +0100, Andre Majorel wrote:
> I'll take your word for it. However, let me assure you that atimes
> somehow used to work over NFS, even for files small enough to fit
> in the cache. Had they been broken, I would have known. I use Mutt.

How would the server know to update the atime? If the file is in cache
we don't READ.

Trond

Re: atimes not updated over NFS

[Trond Myklebust]

On Wed, 2008-01-09 at 18:23 -0500, Trond Myklebust wrote:
> On Wed, 2008-01-09 at 23:37 +0100, Andre Majorel wrote:
> > I'll take your word for it. However, let me assure you that atimes
> > somehow used to work over NFS, even for files small enough to fit
> > in the cache. Had they been broken, I would have known. I use Mutt.
> 
> How would the server know to update the atime? If the file is in cache
> we don't READ.

Put differently. Have you checked on the _server_ that the atime is
actually changing when you do these tests on the client?

Trond

Re: atimes not updated over NFS

[Andre Majorel]

On 2008-01-09 18:23 -0500, Trond Myklebust wrote:
> On Wed, 2008-01-09 at 23:37 +0100, Andre Majorel wrote:
>
> > I'll take your word for it. However, let me assure you that atimes
> > somehow used to work over NFS, even for files small enough to fit
> > in the cache. Had they been broken, I would have known. I use Mutt.
>=20
> How would the server know to update the atime? If the file is in cach=
e
> we don't READ.

You tell me. All I can tell you is that Mutt used to work so
atimes were updated one way or another.

As of 2.6.23, access times are never updated, even for files that
are too big to fit in the cache. I'm willing to run tests if that
helps.

--=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.

Re: atimes not updated over NFS

[Frank van Maarseveen]

On Thu, Jan 10, 2008 at 12:51:53AM +0100, Andre Majorel wrote:
> On 2008-01-09 18:23 -0500, Trond Myklebust wrote:
> > On Wed, 2008-01-09 at 23:37 +0100, Andre Majorel wrote:
> >
> > > I'll take your word for it. However, let me assure you that atimes
> > > somehow used to work over NFS, even for files small enough to fit
> > > in the cache. Had they been broken, I would have known. I use Mutt.
> > 
> > How would the server know to update the atime? If the file is in cache
> > we don't READ.
> 
> You tell me. All I can tell you is that Mutt used to work so
> atimes were updated one way or another.

I think 2.6.22 introduced this regression. On 2.6.22.10, reading a file
on NFS does not affect atime according to stat(), not on the client and
not on the server. This breaks several programs.

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)


-- 
Frank

Re: atimes not updated over NFS

[Trond Myklebust]

On Mon, 2008-01-14 at 09:34 +0100, Frank van Maarseveen wrote:
> On Thu, Jan 10, 2008 at 12:51:53AM +0100, Andre Majorel wrote:
> > On 2008-01-09 18:23 -0500, Trond Myklebust wrote:
> > > On Wed, 2008-01-09 at 23:37 +0100, Andre Majorel wrote:
> > >
> > > > I'll take your word for it. However, let me assure you that atimes
> > > > somehow used to work over NFS, even for files small enough to fit
> > > > in the cache. Had they been broken, I would have known. I use Mutt.
> > > 
> > > How would the server know to update the atime? If the file is in cache
> > > we don't READ.
> > 
> > You tell me. All I can tell you is that Mutt used to work so
> > atimes were updated one way or another.
> 
> I think 2.6.22 introduced this regression. On 2.6.22.10, reading a file
> on NFS does not affect atime according to stat(), not on the client and
> not on the server. This breaks several programs.
> 
> 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.

Cheers
  Trond

Re: atimes not updated over NFS

[Frank van Maarseveen]

On Mon, Jan 14, 2008 at 10:43:12AM -0500, Trond Myklebust wrote:
> 
> On Mon, 2008-01-14 at 09:34 +0100, Frank van Maarseveen wrote:
> > On Thu, Jan 10, 2008 at 12:51:53AM +0100, Andre Majorel wrote:
> > > On 2008-01-09 18:23 -0500, Trond Myklebust wrote:
> > > > On Wed, 2008-01-09 at 23:37 +0100, Andre Majorel wrote:
> > > >
> > > > > I'll take your word for it. However, let me assure you that atimes
> > > > > somehow used to work over NFS, even for files small enough to fit
> > > > > in the cache. Had they been broken, I would have known. I use Mutt.
> > > > 
> > > > How would the server know to update the atime? If the file is in cache
> > > > we don't READ.
> > > 
> > > You tell me. All I can tell you is that Mutt used to work so
> > > atimes were updated one way or another.
> > 
> > I think 2.6.22 introduced this regression. On 2.6.22.10, reading a file
> > on NFS does not affect atime according to stat(), not on the client and
> > not on the server. This breaks several programs.
> > 
> > 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)

-- 
Frank

Re: atimes not updated over NFS

[J. Bruce Fields]

On Mon, Jan 14, 2008 at 06:51:23PM +0100, Frank van Maarseveen wrote:
> On Mon, Jan 14, 2008 at 10:43:12AM -0500, Trond Myklebust wrote:
> > 
> > On Mon, 2008-01-14 at 09:34 +0100, Frank van Maarseveen wrote:
> > > On Thu, Jan 10, 2008 at 12:51:53AM +0100, Andre Majorel wrote:
> > > > On 2008-01-09 18:23 -0500, Trond Myklebust wrote:
> > > > > On Wed, 2008-01-09 at 23:37 +0100, Andre Majorel wrote:
> > > > >
> > > > > > I'll take your word for it. However, let me assure you that atimes
> > > > > > somehow used to work over NFS, even for files small enough to fit
> > > > > > in the cache. Had they been broken, I would have known. I use Mutt.
> > > > > 
> > > > > How would the server know to update the atime? If the file is in cache
> > > > > we don't READ.
> > > > 
> > > > You tell me. All I can tell you is that Mutt used to work so
> > > > atimes were updated one way or another.
> > > 
> > > I think 2.6.22 introduced this regression. On 2.6.22.10, reading a file
> > > on NFS does not affect atime according to stat(), not on the client and
> > > not on the server. This breaks several programs.
> > > 
> > > 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....

--b.

Re: atimes not updated over NFS

[Trond Myklebust]

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

                ssh server 'dd </dev/zero >testfile count=1; ls -l --time=atime --full-time testfile'
                
                cat testfile >/dev/null
                
                ssh server 'ls -l --time=atime --full-time testfile'
                ls -l --time=atime --full-time testfile

should normally force the client to read the file, and should tell if
the atime is being updates correctly on both server and client.

Cheers
  Trond

Re: atimes not updated over NFS

[Frank van Maarseveen]

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

Re: atimes not updated over NFS

[J. Bruce Fields]

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

Re: atimes not updated over NFS

[J. Bruce Fields]

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.

Re: atimes not updated over NFS

[J. Bruce Fields]

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 <jens.axboe@oracle.com>
Date:   Fri Jun 1 11:51:43 2007 +0200

    sendfile: convert nfs to using splice_read()
    
    Acked-by: Trond Myklebust <trond.myklebust@fys.uio.no>
    Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

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;
 }
 

Re: atimes not updated over NFS

[J. Bruce Fields]

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 <jens.axboe@oracle.com>
Date:   Tue Jun 12 21:22:14 2007 +0200

    sendfile: convert nfsd to splice_direct_to_actor()
    
    Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

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);

Re: atimes not updated over NFS

[Jens Axboe]

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

Re: atimes not updated over NFS

[J. Bruce Fields]

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
> 

Re: atimes not updated over NFS

[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

Re: atimes not updated over NFS

[J. Bruce Fields]

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;
 

Re: atimes not updated over NFS

[Jens Axboe]

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

Re: atimes not updated over NFS

[J. Bruce Fields]

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
> 

Re: atimes not updated over NFS

[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

Re: atimes not updated over NFS

[Andre Majorel]

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.

Re: atimes not updated over NFS

[Jens Axboe]

On Tue, Jan 29 2008, Andre Majorel wrote:
> 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;
> > +	}
> >  
> >  	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.

Good point, I'll commit a fixup for that patch.

-- 
Jens Axboe

Re: atimes not updated over NFS

[Andre Majorel]

On 2008-01-30 12:23 +0100, Jens Axboe wrote:
> On Tue, Jan 29 2008, Andre Majorel wrote:
> > On 2008-01-29 19:30 +0100, Jens Axboe wrote:
> >=20
> > > diff --git a/fs/splice.c b/fs/splice.c
> > > index 0a0b79b..504a096 100644
> > > --- a/fs/splice.c
> >=20
> > 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
> Good point, I'll commit a fixup for that patch.

Excellent. Thanks to everyone involved.

--=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.

atimes not updated over NFS

[Andre Majorel]

After upgrading an NFS server from kernel 2.6.12 to 2.6.23, the
atime of files is not updated any more when clients read them :

  client$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
  -rw-r--r-- 1 xxx xxx 0 2007-12-21_14:58:11 /nfsmnt/zz
  client$ md5sum /nfsmnt/zz
  d41d8cd98f00b204e9800998ecf8427e  /nfsmnt/zz
  client$ date +%Y-%m-%d_%H:%M:%S
  2007-12-21_15:00:28
  client$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
  -rw-r--r-- 1 xxx xxx 0 2007-12-21_14:58:11 /nfsmnt/zz

Reading the file locally, i.e. on the NFS server, does update the
atime :

  server$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
  -rw-r--r-- 1 xxx xxx 0 2007-12-21_14:58:11 /nfsmnt/zz
  serveur$ md5sum /nfsmnt/zz
  d41d8cd98f00b204e9800998ecf8427e  /nfsmnt/zz
  serveur$ date +%Y-%m-%d_%H:%M:%S
  2007-12-21_15:04:00
  serveur$ ll -u --time-style +%Y-%m-%d_%H:%M:%S /nfsmnt/zz
  -rw-r--r-- 1 xxx xxx 0 2007-12-21_15:03:57 /nfsmnt/zz

/nfsmnt is mounted with options :
- server : rw,noexec,nosuid,nodev,nodiratime
- client : rw,user=root,nosuid,nodev,nodiratime,intr,rsize=8192,wsize=8192

Any ideas ?

-- 
André Majorel <URL:http://www.teaser.fr/~amajorel/>
Do not use this account for regular correspondence.
See the URL above for contact information.