Re: async O_DIRECT vs. buffered synchronization (or lack thereof)

[Dave Chinner <david@fromorbit.com>]

On Thu, Oct 22, 2015 at 03:42:45PM -0800, Kent Overstreet wrote:
> While off reading code, I noticed something that didn't look quite right...
> 
> Look at generic_file_direct_write(), in mm/filemap.c. What the code there is
> doing is:
> 
>  - dropping the range we're writing to from the page cache (writing it first if
>    necessary), then
>  - doing the write, then
>  - invalidating that range in the pagecache again.
....

> Yet _another_ fun fact: I mentioned that for the filemap_write_and_wait_range();
> invalidate_inode_pages2() sequence to work we have to be preventing pages from
> being redirtied. Well, i_mutex does the job for buffered writes, but not
> page faults - AFAICT page_mkwrite() would have to take i_mutex for this code to
> not race with page faults, and the default page_mkwrite implementation
> (filemap_page_mkwrite()) definitely does not.
> 
> It does _lock_ the page though, so if we had something that combined
> filemap_write_and_wait_range() with invalidating pages, making sure to have the
> page still locked when removing it from the page cache - that ought to work.
> 
> XFS does seem to attempt to get this right - its .page_mkwrite takes the inode
> XFS_MMAPLOCK_SHARED lock, and the xfs truncate and fallocate code both take
> XFS_MMAPLOCK_EXCL (truncate and (in particular) fcollapse also need to drop
> ranges from the page cache, fcollapse is where I first noticed this particular
> issue).
> 
> But AFAICT xfs's dio path does _not_ take the correct lock for this to work -
> although if you look at xfs_file_dio_aio_write() they were clearly thinking
> about page cache synchronization, so perhaps I'm missing something about how
> xfs's locking works.

We can't take it across direct IO submission/completion because that
creates a mmap_sem/XFS_MMAPLOCK inversion due to the direct IO code
calling get_user_pages().

We can't put locks in the page fault path to solve this problem - I
created the XFS_MMAPLOCK locking to solve the "page faults race with
extent manipulation operations" knowing that it couldn't be used to
solve the DIO vs mmap race conditions.

There's a reason why XFS developers still say "if you mix
buffered/mmap IO on the same file as direct IO, you get to keep all
the corrupted bits" and then point users at the open(2) man page:

	Applications should avoid mixing O_DIRECT and normal I/O to
	the same file, and especially to overlapping byte regions in
	the  same  file.   Even  when  the filesystem  correctly
	handles the coherency issues in this situation, overall I/O
	throughput is likely to be slower than using either mode
	alone.  Likewise, applications should avoid mixing mmap(2)
	of files with direct I/O to the same files.

We attempt best effort at maintaining coherency and preventing data
corruption, but we cannot guarantee coherency...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com