Messages in this thread | | | Date | Wed, 20 Jul 2022 21:55:35 +0200 | From | David Hildenbrand <> | Subject | Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect |
| |
On 20.07.22 21:48, Peter Xu wrote: > On Wed, Jul 20, 2022 at 09:33:35PM +0200, David Hildenbrand wrote: >> On 20.07.22 21:15, Peter Xu wrote: >>> On Wed, Jul 20, 2022 at 05:10:37PM +0200, David Hildenbrand wrote: >>>> For pagecache pages it may as well be *plain wrong* to bypass the write >>>> fault handler and simply mark pages dirty+map them writable. >>> >>> Could you elaborate? >> >> Write-fault handling for some filesystems (that even require this >> "slow path") is a bit special. >> >> For example, do_shared_fault() might have to call page_mkwrite(). >> >> AFAIK file systems use that for lazy allocation of disk blocks. >> If you simply go ahead and map a !dirty pagecache page writable >> and mark it dirty, it will not trigger page_mkwrite() and you might >> end up corrupting data. >> >> That's why we the old change_pte_range() code never touched >> anything if the pte wasn't already dirty. > > I don't think that pte_dirty() check was for the pagecache code. For any fs > that has page_mkwrite() defined, it'll already have vma_wants_writenotify() > return 1, so we'll never try to add write bit, hence we'll never even try > to check pte_dirty(). >
I might be too tired, but the whole reason we had this magic before my commit in place was only for the pagecache.
With vma_wants_writenotify()=0 you can directly map the pages writable and don't have to do these advanced checks here. In a writable MAP_SHARED VMA you'll already have pte_write().
We only get !pte_write() in case we have vma_wants_writenotify()=1 ...
try_change_writable = vma_wants_writenotify(vma, vma->vm_page_prot);
and that's the code that checked the dirty bit after all to decide -- amongst other things -- if we can simply map it writable without going via the write fault handler and triggering do_shared_fault() .
See crazy/ugly FOLL_FORCE code in GUP that similarly checks the dirty bit.
But yeah, it's all confusing so I might just be wrong regarding pagecache pages.
-- Thanks,
David / dhildenb
| |