Re: RFC: reviving mlock isolation dead code

[Hugh Dickins <hughd@google.com>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4671 bytes --]

On Mon, 15 Nov 2010, Michel Lespinasse wrote:
> On Mon, Nov 15, 2010 at 5:44 PM, Hugh Dickins <hughd@google.com> wrote:
> > On Sun, 14 Nov 2010, KOSAKI Motohiro wrote:
> >> Michel Lespinasse <walken@google.com> wrote:
> >> > ...
> >> > The other mlock related issue I have is that it marks pages as dirty
> >> > (if they are in a writable VMA), and causes writeback to work on them,
> >> > even though the pages have not actually been modified. This looks like
> >> > it would be solvable with a new get_user_pages flag for mlock use
> >> > (breaking cow etc, but not writing to the pages just yet).
> >>
> >> To be honest, I haven't understand why current code does so. I dislike it too. but
> >> I'm not sure such change is safe or not. I hope another developer comment you ;-)
> >
> > It's been that way for years, and the primary purpose is to do the COWs
> > in advance, so we won't need to allocate new pages later to the locked
> > area: the pages that may be needed are already locked down.
> 
> Thanks Hugh for posting your comments. I was aware of Suleiman's
> proposal to always do a READ mode get_user_pages years ago, and I
> could see that we'd need a new flag instead so we can break COW
> without dirtying pages, but I hadn't thought about other issues.
> 
> > That justifies it for the private mapping case, but what of shared maps?
> > There the justification is that the underlying file might be sparse, and
> > we want to allocate blocks upfront for the locked area.
> >
> > Do we?  I dislike it also, as you both do.  It seems crazy to mark a
> > vast number of pages as dirty when they're not.
> >
> > It makes sense to mark pte_dirty when we have a real write fault to a
> > page, to save the mmu from making that pagetable transaction immediately
> > after; but it does not make sense when the write (if any) may come
> > minutes later - we'll just do a pointless write and clear dirty meanwhile.
> 
> If we just mlocked the page but did not made it writable (or mark it
> dirty) yet, would we be allowed to skip the page_mkwrite method call ?

Yes, indeed you should skip it in that case.

> 
> I believe this would be legal:

Yes, I agree that it would be legal.

> 
> - If/when an actual write comes later on, we'll run through
> do_wp_page() again, and reuse the old page, making it writable and
> dirty from then on. Since this is a shared mapping, we won't have to
> allocate a new page at a that time, so this preserves the mlock
> semantic of having all necessary pages preallocated.
> 
> - If we skip page_mkwrite(), we can't guarantee that the filesystem
> will have a free block to allocate, but is this actually part of the
> mlock() semantics ? I think not, given that only a few filesystems
> implement page_mkwrite() in the first place. ext4 does, but ext2/3
> does not, for example. So while skipping page_mkwrite() would prevent
> data blocks from being pre-allocated, I don't really see it as
> breaking mlock() ?

Yes, allocating the blocks is not actually part of mlock() semantics.

And a few years ago, there was no ->page_mkwrite(), and the ->nopage()
interface didn't tell the filesystem whether it was read or write fault
(and mlocking a writable vma certainly didn't do synchronous writes back
to disk before the mlock returned success or failure).

It's all a matter of QoS: is it acceptable to make the change, that
a write fault to an mlocked area of a sparse file might now generate
SIGBUS, on a few filesystems which have recently been guaranteeing not?

Personally, I believe that's more acceptable than doing a huge rush of
(almost always) pointless writes at the time of mlock().  But I can
see that others may disagree.

> 
> > If it does work out, I think you'd need to be passing the flag down to
> > follow_page too: I have a patch or patches to merge the FOLL_flags with
> > the FAULT_FLAGs - Linus wanted that a year ago, and I recently met a
> > need for it with shmem - I'd better accelerate sending those in.
> 
> The follow_page change is simpler, it might even be sufficient to not
> pass in the FOLL_TOUCH flag I think.

Yes, in fact, is anything required beyond Peter's original simple patch?

There are some tweaks that could be added.  A FAULT_FLAG to let filesystem
know that we're mlocking a writable area, so it could be careful about it?
only useful if some filesystem uses it!  A check on vma_wants_writenotify()
or something like it, so mlock does set pte_write if it's okay e.g. tmpfs?
Second order things, probably don't matter.

Added Ccs of those most likely to agree or disagree with us.

Hugh