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

From: Jan Kara <jack@suse.cz>
To: Lukas Czerner <lczerner@redhat.com>
Cc: Jeff Moyer <jmoyer@redhat.com>, Jan Kara <jack@suse.cz>,
	linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
	david@fromorbit.com
Subject: Re: [PATCH v6] fs: Fix page cache inconsistency when mixing buffered and AIO DIO
Date: Mon, 14 Aug 2017 11:43:31 +0200	[thread overview]
Message-ID: <20170814094331.GD16353@quack2.suse.cz> (raw)
In-Reply-To: <20170811090301.kr5f4tgbaw2jaj7j@rh_laptop>

Hello,

On Fri 11-08-17 11:03:01, Lukas Czerner wrote:
> On Thu, Aug 10, 2017 at 10:22:47AM -0400, Jeff Moyer wrote:
> > Jan Kara <jack@suse.cz> writes:
> > 
> > > On Thu 10-08-17 14:59:57, Lukas Czerner wrote:
> > >> Currently when mixing buffered reads and asynchronous direct writes it
> > >> is possible to end up with the situation where we have stale data in the
> > >> page cache while the new data is already written to disk. This is
> > >> permanent until the affected pages are flushed away. Despite the fact
> > >> that mixing buffered and direct IO is ill-advised it does pose a thread
> > >> for a data integrity, is unexpected and should be fixed.
> > >> 
> > >> Fix this by deferring completion of asynchronous direct writes to a
> > >> process context in the case that there are mapped pages to be found in
> > >> the inode. Later before the completion in dio_complete() invalidate
> > >> the pages in question. This ensures that after the completion the pages
> > >> in the written area are either unmapped, or populated with up-to-date
> > >> data. Also do the same for the iomap case which uses
> > >> iomap_dio_complete() instead.
> > >> 
> > >> This has a side effect of deferring the completion to a process context
> > >> for every AIO DIO that happens on inode that has pages mapped. However
> > >> since the consensus is that this is ill-advised practice the performance
> > >> implication should not be a problem.
> > >> 
> > >> This was based on proposal from Jeff Moyer, thanks!
> > >
> > > It seems the invalidation can be also removed from
> > > generic_file_direct_write(), can't it? It is duplicit there the same way as
> > > it was in the iomap code...
> > 
> > Yep, sure looks that way.
> 
> Hrm, ok. Technically speaking generic_file_direct_write does not have to
> eventually end up with dio_complete() being called. This will change the
> behaviour for those that implement dio differently. Looking at the users
> now, vast majority will end up with complete_dio() so maybe this is not
> a problem.

OK, so this seems to be the problem with 9p, fuse, nfs, lustre.

> This is in contrast with iomap_dio_rw() which will end up calling
> iomap_dio_complete() so the situation is different there.
> 
> Maybe adding mapping->nrpages check would be better than outright
> removing it ?

OK, I agree we cannot just remove the invalidation. But shouldn't we rather
fix the above mentioned filesystems? Otherwise they will keep having issues
you are trying to fix? But for now I could live with keeping the
invalidation behind nrpages check and adding a comment why we kept it
there...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR