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
