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?
-Jeff
