Re: [PATCH v2 08/17] fs: retrofit old error reporting API onto new infrastructure

[Jeff Layton]

On Tue, 2017-04-18 at 08:56 +1000, NeilBrown wrote:
> On Wed, Apr 12 2017, Jeff Layton wrote:
> 
> > On Thu, 2017-04-13 at 08:14 +1000, NeilBrown wrote:
> > > 
> > > I suspect that the filemap_check_wb_error() will need to be moved
> > > into some parent of the current call site, which is essentially what you
> > > suggest below.  It would be nice if we could do that first, rather than
> > > having the current rather odd code.  But maybe this way is an easier
> > > transition.  It isn't obviously wrong, it just isn't obviously right
> > > either.
> > > 
> > 
> > Yeah. It's just such a daunting task to have to change so much of the
> > existing code. I'm looking for ways to make this simpler.
> > 
> > I think it probably is reasonable for filemap_write_and_wait* to just
> > sample it as early as possible in those functions. filemap_fdatawait is
> > the real questionable one, as you may have already had some writebacks
> > complete with errors.
> > 
> > In any case, my thinking was that the old code is not obviously correct
> > either, so while this shortens the "error capture window" on these
> > calls, it seems like a reasonable place to start improving things.
> 
> I agree.  It wouldn't hurt to add a note to this effect in the patch
> comment so that people understand that the code isn't seen to be
> "correct" but only "no worse" with clear direction on what sort of
> improvement might be appropriate.
> 

I've got a cleaned-up set that is getting close to ready for
reposting.ÂBefore I do though, I think there is another option here
that's worth discussing.

We could store a second wb_err_t (aka errseq_t in the new set) in the
mapping that would would basically act as a "cursor" for these cases.
filemap_check_errors would need to do something like 
filemap_report_wb_error, but it would swap the value into the mapping's
cursor instead of dealing with the one in struct file.

I don't really like adding yet another field here, but the struct
address_space definition has this:

    __attribute__((aligned(sizeof(long))));

Adding the wb_err field means that we end up growing the struct by 8
bytes on x86_64 anyway. Adding another 4 bytes would just consume the
pad, so it wouldn't cost anything there. YMMV on other arches of
course.

That's also not perfectly like what we have with AS_EIO/AS_ENOSPC
flags, but is probably close enough not to matter.

So...this would let us limp along for even longer with the model of
reporting since last check. I'm not sure that's a good thing though. A
long term goal here is to have kernel code that's dealing with
writeback be more deliberate about the point from which it's checking
errors, and this doesn't help promote that.
-- 
Jeff Layton <jlayton@xxxxxxxxxx>