2.6.28-rc3 truncates nfsd results

2.6.28-rc3 truncates nfsd results

[Doug Nazar]

Commit 8d7c4203 "nfsd: fix failure to set eof in readdir in some situations"
breaks the nfsd server. Bisected it back to this commit and reverting it
fixes the problem.

However, it only happens on certain machines even with the same kernel &
filesystem (ext3). I've two groups of similar computers, each group running
identical kernels. The ones listing only ~250 files are of course in error.
Eldritch is running 2.6.28-rc3 with that commit reverted. With 2.8.28-rc3 it
showed the incorrect number.


[root@dryad - Tue Nov 04 13:15:41]
[/usr/src/linux] for f in golem eldritch slyph kobold basilisk faun  dryad
nymph troll ogre banshee cockatrice; do echo $f $(ls -1 /net/$f/usr/bin/ |
wc -l); done

golem 1082
eldritch 1644
slyph 1718
kobold 1081
basilisk 267
faun 265

dryad 1595
nymph 245
troll 243
ogre 243
banshee 926
cockatrice 237

Re: 2.6.28-rc3 truncates nfsd results

[Jeff Garzik]

Doug Nazar wrote:
> Commit 8d7c4203 "nfsd: fix failure to set eof in readdir in some situations"
> breaks the nfsd server. Bisected it back to this commit and reverting it
> fixes the problem.
> 
> However, it only happens on certain machines even with the same kernel &
> filesystem (ext3). I've two groups of similar computers, each group running
> identical kernels. The ones listing only ~250 files are of course in error.
> Eldritch is running 2.6.28-rc3 with that commit reverted. With 2.8.28-rc3 it
> showed the incorrect number.

I'm seeing "weirdness" on my NFS4-mounted home directory (client and 
server are both x86-64 2.6.28-rc) -- gconfd locking up, and other 
desktop gadgets breaking.  2.6.27 works great.

I'll try reverting this and see if the behavior improves, thanks for the 
pointer...

	Jeff

Re: 2.6.28-rc3 truncates nfsd results

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

On Tue, 2008-11-04 at 13:43 -0500, Jeff Garzik wrote:
> I'll try reverting this and see if the behavior improves, thanks for the 
> pointer...

See attached...

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: Attached message - [PATCH] nfsd: fix failure to set eof in readdir in some situations --]
[-- Type: message/rfc822, Size: 3187 bytes --]

From: "J. Bruce Fields" <bfields@fieldses.org>
To: linux-nfs@vger.kernel.org
Cc: David Woodhouse <David.Woodhouse@intel.com>, Al Viro <viro@zeniv.linux.org.uk>
Subject: [PATCH] nfsd: fix failure to set eof in readdir in some situations
Date: Thu, 30 Oct 2008 17:31:25 -0400
Message-ID: <20081030213125.GC24156@fieldses.org>

From: J. Bruce Fields <bfields@citi.umich.edu>

Before 14f7dd632011bb89c035722edd6ea0d90ca6b078 "[PATCH] Copy XFS
readdir hack into nfsd code", readdir_cd->err was reset to eof before
each call to vfs_readdir; afterwards, it is set only once.  Similarly,
c002a6c7977320f95b5edede5ce4e0eeecf291ff "[PATCH] Optimise NFS readdir
hack slightly", can cause us to exit without nfserr_eof set.  Fix this.

This ensures the "eof" bit is set when needed in readdir replies.  (The
particular case I saw was an nfsv4 readdir of an empty directory, which
returned with no entries (the protocol requires "." and ".." to be
filtered out), but with eof unset.)

Cc: David Woodhouse <David.Woodhouse@intel.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/vfs.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

I'm queuing this up for 2.6.28.--b.

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 0bc56f6..848a03e 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1912,6 +1912,7 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
 			de = (struct buffered_dirent *)((char *)de + reclen);
 		}
 		offset = vfs_llseek(file, 0, SEEK_CUR);
+		cdp->err = nfserr_eof;
 		if (!buf.full)
 			break;
 	}
-- 
1.5.5.rc1

RE: 2.6.28-rc3 truncates nfsd results

[Doug Nazar]

That's the commit I'm reverting <g>

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of David Woodhouse
> Sent: November 4, 2008 14:04
> To: Jeff Garzik
> Cc: Doug Nazar; 'J. Bruce Fields'; 'Al Viro'; linux-
> kernel@vger.kernel.org
> Subject: Re: 2.6.28-rc3 truncates nfsd results
> 
> On Tue, 2008-11-04 at 13:43 -0500, Jeff Garzik wrote:
> > I'll try reverting this and see if the behavior improves, thanks for
> the
> > pointer...
> 
> See attached...
> 
> --
> David Woodhouse                            Open Source Technology
> Centre
> David.Woodhouse@intel.com                              Intel
> Corporation

RE: 2.6.28-rc3 truncates nfsd results

[David Woodhouse]

On Tue, 2008-11-04 at 14:32 -0500, Doug Nazar wrote:
> That's the commit I'm reverting <g>

Oh, sorry; not paying sufficient attention :)

-- 
dwmw2

Re: 2.6.28-rc3 truncates nfsd results

[J. Bruce Fields]

On Tue, Nov 04, 2008 at 01:43:38PM -0500, Jeff Garzik wrote:
> Doug Nazar wrote:
>> Commit 8d7c4203 "nfsd: fix failure to set eof in readdir in some situations"
>> breaks the nfsd server. Bisected it back to this commit and reverting it
>> fixes the problem.
>>
>> However, it only happens on certain machines even with the same kernel &
>> filesystem (ext3). I've two groups of similar computers, each group running
>> identical kernels. The ones listing only ~250 files are of course in error.
>> Eldritch is running 2.6.28-rc3 with that commit reverted. With 2.8.28-rc3 it
>> showed the incorrect number.
>
> I'm seeing "weirdness" on my NFS4-mounted home directory (client and  
> server are both x86-64 2.6.28-rc) -- gconfd locking up, and other  
> desktop gadgets breaking.  2.6.27 works great.
>
> I'll try reverting this and see if the behavior improves, thanks for the  
> pointer...

Any more details on that "weirdness"?

--b.

Re: 2.6.28-rc3 truncates nfsd results

[J. Bruce Fields]

On Tue, Nov 04, 2008 at 01:27:23PM -0500, Doug Nazar wrote:
> Commit 8d7c4203 "nfsd: fix failure to set eof in readdir in some situations"
> breaks the nfsd server. Bisected it back to this commit and reverting it
> fixes the problem.

Ugh, thanks.  So the test case is just listing a big directory?  I must
have broken the case where we have to fill up multiple pages....

--b.

> However, it only happens on certain machines even with the same kernel &
> filesystem (ext3). I've two groups of similar computers, each group running
> identical kernels. The ones listing only ~250 files are of course in error.
> Eldritch is running 2.6.28-rc3 with that commit reverted. With 2.8.28-rc3 it
> showed the incorrect number.
> 
> 
> [root@dryad - Tue Nov 04 13:15:41]
> [/usr/src/linux] for f in golem eldritch slyph kobold basilisk faun  dryad
> nymph troll ogre banshee cockatrice; do echo $f $(ls -1 /net/$f/usr/bin/ |
> wc -l); done
> 
> golem 1082
> eldritch 1644
> slyph 1718
> kobold 1081
> basilisk 267
> faun 265
> 
> dryad 1595
> nymph 245
> troll 243
> ogre 243
> banshee 926
> cockatrice 237
> 
> 

Re: 2.6.28-rc3 truncates nfsd results

[J. Bruce Fields]

On Tue, Nov 04, 2008 at 01:27:23PM -0500, Doug Nazar wrote:
> Commit 8d7c4203 "nfsd: fix failure to set eof in readdir in some situations"
> breaks the nfsd server. Bisected it back to this commit and reverting it
> fixes the problem.
> 
> However, it only happens on certain machines even with the same kernel &
> filesystem (ext3). I've two groups of similar computers, each group running
> identical kernels. The ones listing only ~250 files are of course in error.
> Eldritch is running 2.6.28-rc3 with that commit reverted. With 2.8.28-rc3 it
> showed the incorrect number.

Well, that's strange; it must be staring me in the face, but I don't see
the problem (and can't reproduce it).  Can you watch for the readdir
with wireshark and see if it's returning an error on the readdir?  Or is
it  just returning succesfully with eof set after the first ~250
entries?

(If you don't want to figure that out yourself you can collect a packet
trace with
	tcpdump -s0 -wtmp.pcap
and send me tmp.pcap.)

--b.

> 
> 
> [root@dryad - Tue Nov 04 13:15:41]
> [/usr/src/linux] for f in golem eldritch slyph kobold basilisk faun  dryad
> nymph troll ogre banshee cockatrice; do echo $f $(ls -1 /net/$f/usr/bin/ |
> wc -l); done
> 
> golem 1082
> eldritch 1644
> slyph 1718
> kobold 1081
> basilisk 267
> faun 265
> 
> dryad 1595
> nymph 245
> troll 243
> ogre 243
> banshee 926
> cockatrice 237
> 
> 

RE: 2.6.28-rc3 truncates nfsd results

[Doug Nazar]

> From: J. Bruce Fields [mailto:bfields@fieldses.org]
> On Tue, Nov 04, 2008 at 01:27:23PM -0500, Doug Nazar wrote:
> > Commit 8d7c4203 "nfsd: fix failure to set eof in readdir in some situations"
> > breaks the nfsd server. Bisected it back to this commit and reverting it
> > fixes the problem.
> >
> > However, it only happens on certain machines even with the same kernel &
> > filesystem (ext3). I've two groups of similar computers, each group running
> > identical kernels. The ones listing only ~250 files are of course in error.
> > Eldritch is running 2.6.28-rc3 with that commit reverted. With 2.8.28-rc3 it
> > showed the incorrect number.
> 
> Well, that's strange; it must be staring me in the face, but I don't see
> the problem (and can't reproduce it).  Can you watch for the readdir
> with wireshark and see if it's returning an error on the readdir?  Or is
> it  just returning succesfully with eof set after the first ~250
> entries?

Ok, think I've figured it out. 

The computers showing the issue are not using dir_index. This causes ext3 to read a block at a time, which then means we can end up
with buf.full==0 but not finished reading the directory. Before 8d7c4203, we'd always get called again because we never set
nfserr_eof which papered over it.

I think the correct solution is to move nfserr_eof into the loop and remove the buf.full check so that we loop until buf.used==0.
The following seems to do the right thing and reduces the network traffic since we now ensure each buffer is full.

Tested on an empty directory & large directory, eof is properly sent and no short buffers.

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 848a03e..4433c8f 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1875,11 +1875,11 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
 		return -ENOMEM;
 
 	offset = *offsetp;
-	cdp->err = nfserr_eof; /* will be cleared on successful read */
 
 	while (1) {
 		unsigned int reclen;
 
+		cdp->err = nfserr_eof; /* will be cleared on successful read */
 		buf.used = 0;
 		buf.full = 0;
 
@@ -1912,9 +1912,6 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
 			de = (struct buffered_dirent *)((char *)de + reclen);
 		}
 		offset = vfs_llseek(file, 0, SEEK_CUR);
-		cdp->err = nfserr_eof;
-		if (!buf.full)
-			break;
 	}
 
  done:

Re: 2.6.28-rc3 truncates nfsd results

[J. Bruce Fields]

On Wed, Nov 05, 2008 at 06:16:28AM -0500, Doug Nazar wrote:
> 
> 
> > From: J. Bruce Fields [mailto:bfields@fieldses.org]
> > On Tue, Nov 04, 2008 at 01:27:23PM -0500, Doug Nazar wrote:
> > > Commit 8d7c4203 "nfsd: fix failure to set eof in readdir in some situations"
> > > breaks the nfsd server. Bisected it back to this commit and reverting it
> > > fixes the problem.
> > >
> > > However, it only happens on certain machines even with the same kernel &
> > > filesystem (ext3). I've two groups of similar computers, each group running
> > > identical kernels. The ones listing only ~250 files are of course in error.
> > > Eldritch is running 2.6.28-rc3 with that commit reverted. With 2.8.28-rc3 it
> > > showed the incorrect number.
> > 
> > Well, that's strange; it must be staring me in the face, but I don't see
> > the problem (and can't reproduce it).  Can you watch for the readdir
> > with wireshark and see if it's returning an error on the readdir?  Or is
> > it  just returning succesfully with eof set after the first ~250
> > entries?
> 
> Ok, think I've figured it out. 

Awesome, good job working this out.

Anyone know where to find the best documentation of the vfs_readdir
semantics?  I obviously didn't understand it as well as I should.

> The computers showing the issue are not using dir_index. This causes
> ext3 to read a block at a time, which then means we can end up with
> buf.full==0 but not finished reading the directory. Before 8d7c4203,
> we'd always get called again because we never set nfserr_eof which
> papered over it.

OK, so if I'm understanding you correctly: there was also (as of
c002a6c797 "Optimise NFS readdir hack slightly"?) a performance
regression which could force the client to do more round trips to the
server to read the whole directory?

> I think the correct solution is to move nfserr_eof into the loop and
> remove the buf.full check so that we loop until buf.used==0.  The
> following seems to do the right thing and reduces the network traffic
> since we now ensure each buffer is full.
> 
> Tested on an empty directory & large directory, eof is properly sent
> and no short buffers.

Thanks, looking....

--b.

> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 848a03e..4433c8f 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1875,11 +1875,11 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
>  		return -ENOMEM;
>  
>  	offset = *offsetp;
> -	cdp->err = nfserr_eof; /* will be cleared on successful read */
>  
>  	while (1) {
>  		unsigned int reclen;
>  
> +		cdp->err = nfserr_eof; /* will be cleared on successful read */
>  		buf.used = 0;
>  		buf.full = 0;
>  
> @@ -1912,9 +1912,6 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
>  			de = (struct buffered_dirent *)((char *)de + reclen);
>  		}
>  		offset = vfs_llseek(file, 0, SEEK_CUR);
> -		cdp->err = nfserr_eof;
> -		if (!buf.full)
> -			break;
>  	}
>  
>   done:
> 
> 
> 

Re: 2.6.28-rc3 truncates nfsd results

[J. Bruce Fields]

Seems good to me; I cleaned up commit message, as below, and I'll submit
in a few days.  If you didn't mind me adding a signed-off-by for you,
that'd also be good.  I hope we've got it right this time!

--b.

commit 19e1e96e0b51f9ad4cb0329e6b32a742cf9db713
Author: Doug Nazar <nazard@dragoninc.ca>
Date:   Wed Nov 5 06:16:28 2008 -0500

    Fix nfsd truncation of readdir results
    
    Commit 8d7c4203 "nfsd: fix failure to set eof in readdir in some
    situations" introduced a bug: on a directory in an exported ext3
    filesystem with dir_index unset, a READDIR will only return about 250
    entries, even if the directory was larger.
    
    Bisected it back to this commit; reverting it fixes the problem.
    
    It turns out that in this case ext3 reads a block at a time, then
    returns from readdir, which means we can end up with buf.full==0 but
    with more entries in the directory still to be read.  Before 8d7c4203
    (but after c002a6c797 "Optimise NFS readdir hack slightly"), this would
    cause us to return the READDIR result immediately, but with the eof bit
    unset.  That could cause a performance regression (because the client
    would need more roundtrips to the server to read the whole directory),
    but no loss in correctness, since the cleared eof bit caused the client
    to send another readdir.  After 8d7c4203, the setting of the eof bit
    made this a correctness problem.
    
    So, move nfserr_eof into the loop and remove the buf.full check so that
    we loop until buf.used==0.  The following seems to do the right thing
    and reduces the network traffic since we don't return a READDIR result
    until the buffer is full.
    
    Tested on an empty directory & large directory; eof is properly sent and
    there are no more short buffers.
    
    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 848a03e..4433c8f 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1875,11 +1875,11 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
 		return -ENOMEM;
 
 	offset = *offsetp;
-	cdp->err = nfserr_eof; /* will be cleared on successful read */
 
 	while (1) {
 		unsigned int reclen;
 
+		cdp->err = nfserr_eof; /* will be cleared on successful read */
 		buf.used = 0;
 		buf.full = 0;
 
@@ -1912,9 +1912,6 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
 			de = (struct buffered_dirent *)((char *)de + reclen);
 		}
 		offset = vfs_llseek(file, 0, SEEK_CUR);
-		cdp->err = nfserr_eof;
-		if (!buf.full)
-			break;
 	}
 
  done:

RE: 2.6.28-rc3 truncates nfsd results

[Doug Nazar]

> From: J. Bruce Fields [mailto:bfields@fieldses.org]
> Anyone know where to find the best documentation of the vfs_readdir
> semantics?  I obviously didn't understand it as well as I should.

Hmm.... I seem to remember a conversation about readdir from Al awhile back. Looking....

Here it is:
http://kerneltrap.org/mailarchive/linux-fsdevel/2008/8/12/2903744

Maybe that will help. 

> > The computers showing the issue are not using dir_index. This causes
> > ext3 to read a block at a time, which then means we can end up with
> > buf.full==0 but not finished reading the directory. Before 8d7c4203,
> > we'd always get called again because we never set nfserr_eof which
> > papered over it.
> 
> OK, so if I'm understanding you correctly: there was also (as of
> c002a6c797 "Optimise NFS readdir hack slightly"?) a performance
> regression which could force the client to do more round trips to the
> server to read the whole directory?

>From what I could see yes. On an empty directory there would actually be two readdir 
requests both with eof not set. On a full directory when the sending buffer was full,
func() would return NFS4ERR_TOOSMALL and nfsd_buffered_read would return success. After 
c002a6c797 "Optimise NFS readdir hack slightly", I ended up with buf.used=912, 
buf.full=0 & offset=4096 so we would finish the while loop with a small output buffer, 
eof not set. The client then makes another request and continues so long as forward 
progress was made. After 8d7c4203c6 "nfsd: fix failure to set eof in readdir in some situations"
it would force eof for any small buffer causing the client to terminate the request.

Commit look fine and sure add my signed-off-by. Not sure if we got *all* the bugs but the way 
I see it, every bug is a chance to learn <g>