Re: [patch] fs: improved handling of page and buffer IO errors

[Nick Piggin <npiggin@suse.de>]

On Tue, Oct 21, 2008 at 02:52:45PM +0200, Miklos Szeredi wrote:
> On Tue, 21 Oct 2008, Nick Piggin wrote:
> > IO error handling in the core mm/fs still doesn't seem perfect, but with
> > the recent round of patches and this one, it should be getting on the
> > right track.
> > 
> > I kind of get the feeling some people would rather forget about all this
> > and brush it under the carpet. Hopefully I'm mistaken, but if anybody
> > disagrees with my assertion that error handling, and data integrity
> > semantics are first-class correctness issues, and therefore are more
> > important than all other non-correctness problems... speak now and let's
> > discuss that, please.
> 
> I agree that error handling is important.  But careful: some
> filesystems (NFS I know) don't set PG_error on async read errors, and
> changing the semantics of ->readpage() from returning EIO to retrying
> will potentially cause infinite loops.  And no casual testing will

OK, they'll just need to be fixed.


> reveal those because peristent read errors are extremely rare.

Same as other read errors I guess. We need to be doing more testing
of error cases. I've been doing it a little bit recently for a couple
of block based filesystems... but the hardest code I think is actually
ensuring each filesystem actually does sane things.
  

> So I think a better aproach would be to do
> 
> 			error = lock_page_killable(page);
> 			if (unlikely(error))
> 				goto readpage_error;
> 			if (PageError(page) || !PageUptodate(page)) {
> 				unlock_page(page);
> 				shrink_readahead_size_eio(filp, ra);
> 				error = -EIO;
> 				goto readpage_error;
> 			}
> 			if (!page->mapping) {
> 				unlock_page(page);
> 				page_cache_release(page);
> 				goto find_page;
> 			}
> 
> etc...
> 
> Is there a case where retrying in case of !PageUptodate() makes any
> sense?

Invalidate I guess is covered now (I don't exactly like the solution,
but it's what we have for now). Truncate hmm, I thought that still
clears PageUptodate, but it doesn't seem to either?

Maybe we can use !PageUptodate, with care, for read errors. It might 
actually be a bit preferable in the sense that PageError can just be
used for write errors only.