Re: [PATCH v5] fs: Fix page cache inconsistency when mixing buffered and AIO DIO

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 03, 2017 at 02:10:47PM -0400, Jeff Moyer wrote:
> Al, would you mind taking this in through your tree?  It's been reviewed
> by myself and Jan in this mail thread.

Still needs more fixing, I think?

Sorry, this is the first time I've seen this patch....

> > diff --git a/fs/iomap.c b/fs/iomap.c
> > index 1732228..144512e 100644
> > --- a/fs/iomap.c
> > +++ b/fs/iomap.c
> > @@ -713,8 +713,15 @@ struct iomap_dio {
> >  static ssize_t iomap_dio_complete(struct iomap_dio *dio)
> >  {
> >  	struct kiocb *iocb = dio->iocb;
> > +	struct inode *inode = file_inode(iocb->ki_filp);
> >  	ssize_t ret;
> >  
> > +	if (!dio->error &&
> > +	    (dio->flags & IOMAP_DIO_WRITE) && inode->i_mapping->nrpages)
> > +		invalidate_inode_pages2_range(inode->i_mapping,
> > +				iocb->ki_pos >> PAGE_SHIFT,
> > +				(iocb->ki_pos + dio->size - 1) >> PAGE_SHIFT);
> > +
> >  	if (dio->end_io) {
> >  		ret = dio->end_io(iocb,
> >  				dio->error ? dio->error : dio->size,
> 

This invalidation is already run in iomap_dio_rw() for the sync IO
case directly after the call to iomap_dio_complete().  It also has a
comment to explain exactly why the the invalidation is needed, and
it issues a warning to dmesg if the invalidation fails to indicate
the reason why the user is reporting data corruption to us. i.e.:

.....
        ret = iomap_dio_complete(dio);

        /*
         * Try again to invalidate clean pages which might have been cached by
         * non-direct readahead, or faulted in by get_user_pages() if the source
         * of the write was an mmap'ed region of the file we're writing.  Either
         * one is a pretty crazy thing to do, so we don't support it 100%.  If
         * this invalidation fails, tough, the write still worked...
         */
        if (iov_iter_rw(iter) == WRITE) {
                int err = invalidate_inode_pages2_range(mapping,
                                start >> PAGE_SHIFT, end >> PAGE_SHIFT);
                WARN_ON_ONCE(err);
        }

        return ret;

If we're going to replace this with an invalidation in
iomap_dio_complete() so it also handles the AIO path, then the
comment and warning on invalidation failure also need to be moved to
iomap_dio_complete() and the duplicate code removed from
iomap_dio_rw()...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com