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(). > Because as long as it's not writable, > the FS might have to be informed about the write-unprotect. > > And we end up in the case here for VM_SHARED with vma_wants_writenotify(). > Where we, for example, check > > /* The backer wishes to know when pages are first written to? * > if (vm_ops && (vm_ops->page_mkwrite || vm_ops->pfn_mkwrite))$ > return 1; > > > Long story short, we should be really careful with write-fault handler bypasses, > especially when deciding to set dirty bits. For pagecache pages, we have to be > especially careful. Since you mentioned page_mkwrite, IMHO it's really the write bit not dirty bit that matters here (and IMHO that's why it's called page_mkwrite() not page_mkdirty()). Here Nadav's patch added pte_mkdirty() only if pte_mkwrite() happens. So I'm a bit confused on what's your worry, and what you're against doing. Say, even if with my original proposal to set dirty unconditionally, it'll be still be after the pte_mkwrite(). I never see how that could affect page_mkwrite not to mention it'll not even reach there. > > For exclusive anon pages it's mostly ok, because wp_page_reuse() > doesn't really contain that much magic. The only thing to consider for ordinary > mprotect() is that there is -- IMHO -- absolutely no guarantee that someone will > write to a specific write-unprotected page soon. For uffd-wp-unprotect it might be > easier to guess, especially, if we un-protect only a single page. Yeh, as mentioned I think that's a valid point - looks good to me to attach the dirty bit only when with a hint. Thanks, -- Peter Xu
