[RFC] 2.4.20-rc3 change broke NFS

[RFC] 2.4.20-rc3 change broke NFS

[Joe Korty]

Hi Trond, Everyone,
 A change made to nfs_readpage_result() back in 2.4.20-rc3 broke NFS
for me.  The problem shows up as EIO errors under heavy read/write
load (kernel builds) when Linux is the client and PowerMAX OS
(Concurrent Computer Corp) is the server.  Both sides are running
NFSv3.

As the problem does not show up when Linux is also the server, it is
possible that the bug is in the PowerMAX OS side.  However, the patch
made to Linux at that time simply looks *wrong* .. why should errors
be returned by a NFS read routine just because EOF is not set?
Perhaps we are hitting a race on close that Linux, when it is the
server, doesn't see because it responds faster?

Regards,
Joe

Patch is against 2.4.21-pre5+bk and removes the offending code snippet.

--- fs/nfs/read.c.orig	2003-03-11 05:02:10.000000000 -0500
+++ fs/nfs/read.c	2003-03-11 14:18:00.000000000 -0500
@@ -424,14 +424,9 @@
 				memset(p + count, 0, PAGE_CACHE_SIZE - count);
 				kunmap(page);
 				count = 0;
-				if (data->res.eof)
-					SetPageUptodate(page);
-				else
-					SetPageError(page);
-			} else {
+			} else
 				count -= PAGE_CACHE_SIZE;
-				SetPageUptodate(page);
-			}
+			SetPageUptodate(page);
 		} else
 			SetPageError(page);
 		flush_dcache_page(page);

[RFC] 2.4.20-rc3 change broke NFS

[Trond Myklebust]

>>>>> " " == Joe Korty <jak@rudolph.ccur.com> writes:

     > As the problem does not show up when Linux is also the server,
     > it is possible that the bug is in the PowerMAX OS side.
     > However, the patch made to Linux at that time simply looks
     > *wrong* .. why should errors be returned by a NFS read routine
     > just because EOF is not set?  Perhaps we are hitting a race on
     > close that Linux, when it is the server, doesn't see because it
     > responds faster?

You are actually probably hitting a hole in the file. If the client
has written beyond the eof, but not yet transmitted the data to the
server, then you would indeed probably see an EIO.

Your fix is probably correct, but could you just cross-check by seeing
if the appended patch also fixes the problem?

Cheers,
  Trond

--- linux-2.4.21-up/fs/nfs/read.c.orig	2002-12-12 02:23:09.000000000 -0800
+++ linux-2.4.21-up/fs/nfs/read.c	2003-03-11 13:52:26.000000000 -0800
@@ -424,7 +424,7 @@
 				memset(p + count, 0, PAGE_CACHE_SIZE - count);
 				kunmap(page);
 				count = 0;
-				if (data->res.eof)
+				if (data->res.eof || page_index(page) <= inode->i_size >> PAGE_CACHE_SHIFT)
 					SetPageUptodate(page);
 				else
 					SetPageError(page);

Re: [RFC] 2.4.20-rc3 change broke NFS

[Joe Korty]

> You are actually probably hitting a hole in the file. If the client
> has written beyond the eof, but not yet transmitted the data to the
> server, then you would indeed probably see an EIO.
> 
> Your fix is probably correct, but could you just cross-check by seeing
> if the appended patch also fixes the problem?



Hi Trond,
Your fix to nfs_readpage_result worked.  There may be a small hole
remaining, which the following closes.  This also worked.

- if (data->res.eof || page_index(page) <= inode->i_size >> PAGE_CACHE_SHIFT)
+ if (data->res.eof || page_index(page) < PAGE_CACHE_ALIGN(inode->i_size) >> PAGE_CACHE_SHIFT)

Thanks for your help,
(I'll leave it to you to submit the patch you think best to Marcello and/or Linus).       
Joe

Re: [RFC] 2.4.20-rc3 change broke NFS

[Trond Myklebust]

>>>>> " " == Joe Korty <jak@rudolph.ccur.com> writes:

    >> You are actually probably hitting a hole in the file. If the
    >> client has written beyond the eof, but not yet transmitted the
    >> data to the server, then you would indeed probably see an EIO.
    >>
    >> Your fix is probably correct, but could you just cross-check by
    >> seeing if the appended patch also fixes the problem?



     > Hi Trond, Your fix to nfs_readpage_result worked.  There may be
     > a small hole remaining, which the following closes.  This also
     > worked.

     > - if (data->res.eof || page_index(page) <= inode->i_size >>
     >   PAGE_CACHE_SHIFT)
     > + if (data->res.eof || page_index(page) <
     >   PAGE_CACHE_ALIGN(inode->i_size) >> PAGE_CACHE_SHIFT)

Thanks.

Cheers,
  Trond