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

[Jeff Moyer <jmoyer@redhat.com>]

Dave Chinner <david@fromorbit.com> writes:

> 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()...

Yep, good catch.  Lukas, care to respin?

-Jeff