On Mon, Jul 30, 2018 at 2:53 PM Hugh Dickins <hughd@xxxxxxxxxx> wrote:
>
> I have no problem with reverting -rc7's vma_is_anonymous() series.
I don't think we need to revert the whole series: I think the rest are
all fairly obvious cleanups, and shouldn't really have any semantic
changes.
It's literally only that last patch in the series that then changes
that meaning of "vm_ops". And I don't really _mind_ that last step
either, but since we don't know exactly what it was that it broke, and
we're past rc7, I don't think we really have any option but the revert
it.
And if we revert it, I think we need to just remove the
VM_BUG_ON_VMA() that it was supposed to fix. Because I do think that
it is quite likely that the real bug is that overzealous BUG_ON(),
since I can't see any reason why anonymous mappings should be special
there.
But I'm certainly also ok with re-visiting that commit later. I just
think that right _now_ the above is my preferred plan.
Kirill?
> I'm all for deleting that VM_BUG_ON_VMA() in zap_pmd_range(), it was
> just a compromise with those who wanted to keep something there;
> I don't think we even need a WARN_ON_ONCE() now.
So to me it looks like a historical check that simply doesn't
"normally" trigger, but there's no reason I can see why we should care
about the case it tests against.
> (It remains quite interesting how exit_mmap() does not come that way,
> and most syscalls split the vma beforehand in vma_adjust(): it's mostly
> about madvise(,,MADV_DONTNEED), perhaps others now, which zap ptes
> without prior splitting.)
Well, in this case it's the ftruncate() path, which fundamentally
doesn't split the vma at all (prior *or* later). But yes, madvise() is
in the same boat - it doesn't change the vma at all, it just changes
the contents of the vma.
And exit_mmap() is special because it just tears down everything.
So we do have three very distinct cases:
(a) changing and thus splitting the vma itself (mprotect, munmap/mmap, mlock),
(b) not changing the vma, but changing the underlying mapping
(truncate and madvise(MADV_DONTNEED)
(c) tearing down everything, and no locking needed because it's the
last user (exit_mmap).
that are different for what I think are good reasons.
Linus
