async O_DIRECT vs. buffered synchronization (or lack thereof)

[Kent Overstreet <kent.overstreet@gmail.com>]

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.

The second invalidate_inode_pages2() call is because we aren't holding any locks
that prevent _reads_ from pulling data into the pagecache, and we need to make
sure the pagecache is consistent with what's on disk - hence, we need to
invalidate the range we wrote _after_ our write completes, when new reads will
see the correct data.

But - if it's as async write, the write hasn't actually completed and the second
invalidate_inode_pages2() call is pointless (and it's not happening anywhere
else, either).

So - perhaps you're going to say "Why not just add a call to
invalidate_inode_pages2() in the correct spot, in the async completion path?"

Well, invalidate_inode_pages2() drops dirty pages on the floor - you need to to
call filemap_write_and_wait_range() first, and be holding something that
prevents pages from being redirtied - i_mutex. But we only hold i_mutex for IO
submission, it isn't held for the entire duration of the IO (that would suck).

This is particularly bad since O_DIRECT granularity is the fs block size - i.e.
potentially smaller than PAGE_SIZE. That buffered write that happens while the
AIO write is in flight could be going to the same page but different blocks -
but one of them is going to end up dropped/overwritten.

And another scary thing is the fact that O_DIRECT writes fall back to buffered
at the drop of a hat - writes to holes are the main case, but it can be anything
the filesystem didn't feel like implementing in the O_DIRECT case (I vaguelly
recall there being some weird btrfs case involving some weird extent situation
where it would fall back). So a user doesn't have to be intentionally doing
O_DIRECT and buffered IO at the same time.

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.

BTW, I'd love if someone could just show me that I was mistaken about some of
this and it does in fact work, a lot of this locking is damned subtle and it's
entirely possible I'm missing something crucial...