Re: NFS regression - EIO is returned instead of ENOSPC

[NeilBrown <neilb@suse.de>]

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

On Tue, 11 Dec 2012 23:20:38 +0000 "Myklebust, Trond"
<Trond.Myklebust@netapp.com> wrote:

> On Tue, 2012-12-11 at 18:16 -0500, Trond Myklebust wrote:
> > Hmm... I can see 2 places where we're setting the PageError flag.
> > 
> >      1. nfs_updatepage(): in this case, the error occurred when we tried
> >         to change the page contents. Since we're holding the page lock,
> >         and so rather than mark the page as bad, we could probably just
> >         write back existing dirty areas (using nfs_wb_page()) and then
> >         remove it from the mapping.
> >      2.  nfs_write_completion(): here the writeback error applies to the
> >         entire dirty area on the page, and there is no point in try to
> >         write back again. Better just evict the page from the page cache
> >         (which is what nfs_zap_mapping() is supposed to do). While
> >         setting the PageError flag does cause some of the writeback
> >         functions to return EIO, that's not really what we're after; we
> >         already report errors more completely via the open context.
> > 
> > So for now, can't we just change nfs_set_pageerror() to not bother
> > setting the PG_error flag? Then in the future we might want to make
> > nfs_updatepage a bit more sophisticated in how it deals with those
> > errors...
> 
> Ultimately, what I'm saying is that PageError is a hack for passing
> errors around. Since we have our own hack for doing the same, then why
> use PageError at all?
> 

Sounds like a real possibility.

I just tested with

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 9347ab7..e5da5e8 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -202,7 +202,6 @@ out:
 /* A writeback failed: mark the page as bad, and invalidate the page cache */
 static void nfs_set_pageerror(struct page *page)
 {
-	SetPageError(page);
 	nfs_zap_mapping(page_file_mapping(page)->host, page_file_mapping(page));
 }
 

and I get the correct "ENOSPC" error, so that's a good sign.

Tested-by: NeilBrown <neilb@suse.de>
if you like.

I've haven't tried to examine all the consequences of the change to ensure
there are no unintended side effect, but you know the code better than me so
if you think it is safe - I suspect it is.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]