On Mon, Aug 07, 2017 at 11:52:45AM -0400, Jeff Moyer wrote:
> Dave Chinner <david@xxxxxxxxxxxxx> 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?
Of course, I'll respin.
-Lukas
>
> -Jeff
