Re: Hole punching and mmap races

[Dave Chinner <david@fromorbit.com>]

On Wed, May 16, 2012 at 03:04:45PM +0200, Jan Kara wrote:
> On Wed 16-05-12 12:14:23, Dave Chinner wrote:
> > On Wed, May 16, 2012 at 12:48:05AM +0200, Jan Kara wrote:
> > > It's not easy to protect against these races. For truncate, i_size protects
> > > us against similar races but for hole punching we don't have any such
> > > mechanism. One way to avoid the race would be to hold mmap_sem while we are
> > > invalidating the page cache and punching hole but that sounds a bit ugly.
> > > Alternatively we could just have some special lock (rwsem?) held during
> > > page_mkwrite() (for reading) and during whole hole punching (for writing)
> > > to serialize these two operations.
> > 
> > What really needs to happen is that .page_mkwrite() can be made to
> > fail with -EAGAIN and retry the entire page fault from the start an
> > arbitrary number of times instead of just once as the current code
> > does with VM_FAULT_RETRY. That would allow us to try to take the
> > filesystem lock that provides IO exclusion for all other types of IO
> > and fail with EAGAIN if we can't get it without blocking. For XFS,
> > that is the i_iolock rwsem, for others it is the i_mutex, and some
> > other filesystems might take other locks.
>   Actually, I've been playing with VM_FAULT_RETRY recently (for freezing
> patches) and it's completely unhandled for .page_mkwrite() callbacks.

Yeah, it's a mess.

> Also
> only x86 really tries to handle it at all. Other architectures just don't
> allow it at all. Also there's a ton of callers of things like
> get_user_pages() which would need to handle VM_FAULT_RETRY and for some of
> them it would be actually non-trivial.

Seems kind of silly to me to have a generic retry capability in the
page fault handler and then not implement it in a useful manner for
*anyone*.

> But in this particular case, I don't think VM_FAULT_RETRY is strictly
> necessary. We can have a lock, which ranks below mmap_sem (and thus
> i_mutex / i_iolock) and above i_mmap_mutex (thus page lock), transaction
> start, etc. Such lock could be taken in page_mkwrite() before taking page
> lock, in truncate() and punch_hold() just after i_mutex, and direct IO
> paths could be tweaked to use it as well I think.

Which means we'd be adding another layer of mostly redundant locking
just to avoid i_mutex/mmap_sem inversion. But I don't see how it
solves the direct IO problem because we still need to grab the
mmap_sem inside the IO during get_user_pages_fast() while holding
i_mutex/i_iolock....

> > FWIW, I've been running at "use the IO lock in page_mkwrite" patch
> > for XFS for several months now, but I haven't posted it because
> > without the VM side being able to handle such locking failures
> > gracefully there's not much point in making the change. I did this
> > patch to reduce the incidence of mmap vs direct IO races that are
> > essentially identical in nature to rule them out of the cause of
> > stray delalloc blocks in files that fsstress has been producing on
> > XFS. FYI, this race condition hasn't been responsible for any of the
> > problems I've found recently....
>   Yeah, I've been trying to hit the race window for a while and I failed as
> well...

IIRC, it's a rare case (that I consider insane, BTW):  read from a
file with into a buffer that is a mmap()d region of the same file
that has not been faulted in yet.....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com