Re: [Lsf-pc] [LSF/MM TOPIC] improving writeback error handling

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2018-04-20 at 09:47 +1000, Dave Chinner wrote:
> On Thu, Apr 19, 2018 at 01:14:04PM -0400, Jeff Layton wrote:
> > On Thu, 2018-04-19 at 10:44 +1000, Dave Chinner wrote:
> > > On Wed, Apr 18, 2018 at 12:00:10PM -0400, Jeff Layton wrote:
> > > > > > 4) syncfs doesn't currently report an error when a single inode fails
> > > > > > writeback, only when syncing out the block device. Should it report
> > > > > > errors in that case as well?
> > > > > 
> > > > > Yes.
> > > > 
> > > > I have a small patch that implements this that I posted a few days ago.
> > > > I meant to mark it as an RFC, but didn't, fwiw. I'm not convinced that
> > > > it's the right approach.
> > > > 
> > > > Instead of growing struct file to accommodate a second errseq cursor,
> > > > this only implements that behavior when the file is opened with O_PATH
> > > > (as we know that that will have the fsync op set to NULL). Maybe we can
> > > > do this better somehow though.
> > > 
> > > No idea whether this is possible, or even a good idea, but could we
> > > just have syncfs() create a temporary struct file for the duration
> > > of the syncfs call, so it works on any fd passed in? (sorta like an
> > > internal dupfd() call?)
> > > 
> > 
> > No, we need something that will persist the errseq_t cursor between
> > syncfs calls.
> > 
> > If we did what you're suggesting, once your temporary file goes away,
> > you'd lose your place in the error stream and you'd end up reporting
> > the same errors more than once on subsequent calls to syncfs.
> 
> Which, IMO, is correct behaviour. If there is a persistent data
> writeback errors, then syncfs() should report it *every time it is
> called* because that syncfs() call did not result in the filesystem
> being fully committed to stable storage.
>
> I don't care whether the error has been reported before - the
> context of syncfs() is "commit the entire filesystem to stable
> storage". If any IO failed then we have not acheived what the
> application asked us to do and so we should be returning an error on
> every call that sees a data writeback error.
> 

The problem is that that you would get back errors even if the problem
went away:

Suppose you write some data, and that fails writeback. You call syncfs
and that returns an error. So, you write the data again, and this time
it succeeds, and you call syncfs again on the same fd that you
originally called it. It will still return error because we have no way
to persist the fact that you already saw the original error on the first
syncfs call. That's wrong behavior, IMO.

The errseq_t mechanism requires that you keep a record of the last error
that you saw, so that we know whether to report it again or not.

-- 
Jeff Layton <jlayton@kernel.org>