From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:40468 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750818AbdHHIlh (ORCPT ); Tue, 8 Aug 2017 04:41:37 -0400 Date: Tue, 8 Aug 2017 10:41:32 +0200 From: Lukas Czerner To: Jeff Moyer Cc: Dave Chinner , viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org, jack@suse.cz, hch@infradead.org Subject: Re: [PATCH v5] fs: Fix page cache inconsistency when mixing buffered and AIO DIO Message-ID: <20170808084132.hlvh7zn6s6ggiunr@rh_laptop> References: <1500380368-31661-1-git-send-email-lczerner@redhat.com> <1500463692-4982-1-git-send-email-lczerner@redhat.com> <20170804100909.GD21024@dastard> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Aug 07, 2017 at 11:52:45AM -0400, Jeff Moyer wrote: > Dave Chinner 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