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

[Jan Kara <jack@suse.cz>]

On Fri 20-04-18 09:47:45, 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.

I believe syncfs() behavior should be consistent with an fsync() behavior.
Otherwise it would be rather confusing for userspace. So what you suggest
would mean that fsync() would have to keep reporting error once it reported
an error since we've dropped the dirty bits from pages and thus cannot ever
make user data durable anymore.

Or we could do what Jeff proposes - i.e., make syncfs() behave like fsync()
currently by reporting each error for each fd only once.

And I find both behaviors justifiable. 

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR