Messages in this thread | | | Subject | Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address | From | Jason Wang <> | Date | Fri, 8 Mar 2019 17:13:26 +0800 |
| |
On 2019/3/8 上午5:27, Andrea Arcangeli wrote: > Hello Jerome, > > On Thu, Mar 07, 2019 at 03:17:22PM -0500, Jerome Glisse wrote: >> So for the above the easiest thing is to call set_page_dirty() from >> the mmu notifier callback. It is always safe to use the non locking >> variant from such callback. Well it is safe only if the page was >> map with write permission prior to the callback so here i assume >> nothing stupid is going on and that you only vmap page with write >> if they have a CPU pte with write and if not then you force a write >> page fault. > So if the GUP doesn't set FOLL_WRITE, set_page_dirty simply shouldn't > be called in such case. It only ever makes sense if the pte is > writable. > > On a side note, the reason the write bit on the pte enabled avoids the > need of the _lock suffix is because of the stable page writeback > guarantees? > >> Basicly from mmu notifier callback you have the same right as zap >> pte has. > Good point. > > Related to this I already was wondering why the set_page_dirty is not > done in the invalidate. Reading the patch it looks like the dirty is > marked dirty when the ring wraps around, not in the invalidate, Jeson > can tell if I misread something there.
Actually not wrapping around, the pages for used ring was marked as dirty after a round of virtqueue processing when we're sure vhost wrote something there.
Thanks
> > For transient data passing through the ring, nobody should care if > it's lost. It's not user-journaled anyway so it could hit the disk in > any order. The only reason to flush it to do disk is if there's memory > pressure (to pageout like a swapout) and in such case it's enough to > mark it dirty only in the mmu notifier invalidate like you pointed out > (and only if GUP was called with FOLL_WRITE). > >> O_DIRECT can suffer from the same issue but the race window for that >> is small enough that it is unlikely it ever happened. But for device > Ok that clarifies things. > >> driver that GUP page for hours/days/weeks/months ... obviously the >> race window is big enough here. It affects many fs (ext4, xfs, ...) >> in different ways. I think ext4 is the most obvious because of the >> kernel log trace it leaves behind. >> >> Bottom line is for set_page_dirty to be safe you need the following: >> lock_page() >> page_mkwrite() >> set_pte_with_write() >> unlock_page() > I also wondered why ext4 writepage doesn't recreate the bh if they got > dropped by the VM and page->private is 0. I mean, page->index and > page->mapping are still there, that's enough info for writepage itself > to take a slow path and calls page_mkwrite to find where to write the > page on disk. > >> Now when loosing the write permission on the pte you will first get >> a mmu notifier callback so anyone that abide by mmu notifier is fine >> as long as they only write to the page if they found a pte with >> write as it means the above sequence did happen and page is write- >> able until the mmu notifier callback happens. >> >> When you lookup a page into the page cache you still need to call >> page_mkwrite() before installing a write-able pte. >> >> Here for this vmap thing all you need is that the original user >> pte had the write flag. If you only allow write in the vmap when >> the original pte had write and you abide by mmu notifier then it >> is ok to call set_page_dirty from the mmu notifier (but not after). >> >> Hence why my suggestion is a special vunmap that call set_page_dirty >> on the page from the mmu notifier. > Agreed, that will solve all issues in vhost context with regard to > set_page_dirty, including the case the memory is backed by VM_SHARED ext4. > > Thanks! > Andrea
| |