Re: [PATCH] iomap: Handle I/O errors gracefully in page_mkwrite

[Matthew Wilcox]

On Sat, Jun 06, 2020 at 07:48:41AM +1000, Dave Chinner wrote:
> On Fri, Jun 05, 2020 at 05:48:26AM -0700, Matthew Wilcox wrote:
> > ... I don't think that's the interesting path.  I mean, that's
> > the submission path, and usually we discover errors in the completion
> > path, not the submission path.
> 
> Where in the iomap write IO completion path do we call
> ClearPageUptodate()?

Oh, I misread.  You're right, I was looking at the read completion path.

So, this is also inconsistent.  We clear PageUptodate on errors we
discover during submission, but not for errors we discover during
completion.  That doesn't make sense.

> This comes back to my original, underlying worry about the fragility
> of the page fault path: the page fault path is not even checking for
> PageError during faults, and I'm betting that almost no
> ->page_mkwrite implementation is checking it, either....

I think it's a reasonable assumption that user page tables should never
contain a PTE for a page which is !Uptodate.  Otherwise the user can
read stale data.

> > I don't see why it can't be done from the submission path.
> > unmap_mapping_range() calls i_mmap_lock_write(), which is
> > down_write(i_mmap_rwsem) in drag.  There might be a lock ordering
> > issue there, although lockdep should find it pretty quickly.
> > 
> > The bigger problem is the completion path.  We're in softirq context,
> > so that will have to punt to a thread that can take mutexes.
> 
> Punt to workqueue if we aren't already in a workqueue context -
> for a lot of writes on XFS we already will be running completion in
> a workqueue context....

Yep.