On Jul 20, 2022, at 12:55 PM, David Hildenbrand <david@xxxxxxxxxx> wrote: > 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. I thought you want to get rid of it at least for anonymous pages. No? > > But yeah, it's all confusing so I might just be wrong regarding > pagecache pages. Just to note: I am not very courageous and I did not intend to change condition for when non-anonymous pages are set as writable. That’s the reason I did not change the dirty for non-writable non-anonymous entries (as Peter said). And that’s the reason that setting the dirty bit (at least as I should have done it) is only performed after we made the decision on the write-bit. IOW, after you made your decision about the write-bit, then and only then you may be able to set the dirty bit for writable entries. Since the entry is already writeable (i.e., can be written without a fault later directly from userspace), there should be no concern of correctness when you set it.
