On Fri, Sep 03, 2021 at 09:42:34AM +0200, David Hildenbrand wrote:

On 02.09.21 22:17, Peter Xu wrote:

quoted

It was conditionally done previously, as there's one shmem special case that we
use SetPageDirty() instead.  However that's not necessary and it should be
easier and cleaner to do it unconditionally in mfill_atomic_install_pte().

The most recent discussion about this is here, where Hugh explained the history
of SetPageDirty() and why it's possible that it's not required at all:

https://lore.kernel.org/lkml/alpine.LSU.2.11.2104121657050.1097@eggly.anvils/ (local)

Currently mfill_atomic_install_pte() has three callers:

         1. shmem_mfill_atomic_pte
         2. mcopy_atomic_pte
         3. mcontinue_atomic_pte

After the change: case (1) should have its SetPageDirty replaced by the dirty
bit on pte (so we unify them together, finally), case (2) should have no
functional change at all as it has page_in_cache==false, case (3) may add a
dirty bit to the pte.  However since case (3) is UFFDIO_CONTINUE for shmem,
it's merely 100% sure the page is dirty after all, so should not make a real
difference either.

Would it be worth adding VM_BUG_ON() to make sure that "100%" is really the
case?

I won't be able to make it 100% sure (and that's where I put it "merely").  The
example discussed between Axel and me in the other thread could be an outlier
(when two processes, uffd target, and uffd minor resolver, map the region as
RO), it's just that neither do I think that's a great matter, nor do I think it
would be worth a BUG_ON(), not to mention we use BUG_ON so carefully.

quoted

This should make it much easier to follow on which case will set dirty for
uffd, as we'll simply set it all now for all uffd related ioctls.  Meanwhile,
no special handling of SetPageDirty() if there's no need.

To me this all sounds sane, but I'm certainly not an expert on that code, so
...

No problem.  I hope this patch didn't bring much headache to a lot of people.
It's just that I do think this is the right thing to do so I will insist until
someone says no to me.  Already appreciate a lot for all the comments and r-bs!

-- 
Peter Xu