On Jul 20, 2022, at 1:38 PM, David Hildenbrand <david@xxxxxxxxxx> wrote: > On 20.07.22 22:22, Nadav Amit wrote: >> 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? > > Yes. Especially for any MAP_PRIVATE mappings. > > If you want to write to something that's not mapped writable in a > MAP_PRIVATE mapping it > a) Has to be an exclusive anonymous page > b) The pte has to be dirty Do you need both conditions to be true? I thought (a) is sufficient (if the soft-dirty and related checks succeed). > > In any other case, you clearly missed to COW or the modifications might > get lost if the PTE is not dirty. > > MAP_SHARED is a bit more involved. But whether the pte is dirty might be > good enough ... but this needs a lot more care. > >>> 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. > > Good. As long as we stick to anonymous pages I roughly know what we we > can and cannot do at this point :) > > > The problem I see is that detection whether we can write requires the > dirty bit ... and whether to set the dirty bit requires the information > whether we can write. > > Again, for anonymous pages we should be able to relax the "dirty" > requirement when detecting whether we can write. That’s all I wanted to do there. > >> 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. > > That makes sense to me. What keeps confusing me are architectures with > and without a hw-managed dirty bit ... :) I don’t know which arch you have in your mind. But the moment a PTE is writable, then marking it logically/architecturally as dirty should be fine. But… if the Exclusive check is not good enough for private+anon without the “logical” dirty bit, then there would be a problem.
