Re: [PATCH v2 1/5] mm/shmem: Unconditionally set pte dirty in mfill_atomic_install_pte

David Hildenbrand
2021-09-03T07:42:40Z

On 02.09.21 22:17, Peter Xu wrote:
> 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/
>
> 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?

>
> 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 ...

--
Thanks,

David / dhildenb