On Fri, Oct 21, 2022 at 04:45:27PM +0200, David Hildenbrand wrote: > On 21.10.22 16:28, Peter Xu wrote: > > On Fri, Oct 21, 2022 at 04:10:41PM +0200, David Hildenbrand wrote: > > > On 21.10.22 16:01, Peter Xu wrote: > > > > On Fri, Oct 21, 2022 at 09:23:00AM +0200, David Hildenbrand wrote: > > > > > On 20.10.22 23:10, Peter Xu wrote: > > > > > > On Thu, Oct 20, 2022 at 09:14:09PM +0100, Matthew Wilcox wrote: > > > > > > > In yesterday's call, David brought up the case where we fallocate a file > > > > > > > in shmem, call mmap(MAP_PRIVATE) and then store to a page which is over > > > > > > > a hole. That currently causes shmem to allocate a page, zero-fill it, > > > > > > > then COW it, resulting in two pages being allocated when only the > > > > > > > COW page really needs to be allocated. > > > > > > > > > > > > > > The path we currently take through the MM when we take the page fault > > > > > > > looks like this (correct me if I'm wrong ...): > > > > > > > > > > > > > > handle_mm_fault() > > > > > > > __handle_mm_fault() > > > > > > > handle_pte_fault() > > > > > > > do_fault() > > > > > > > do_cow_fault() > > > > > > > __do_fault() > > > > > > > vm_ops->fault() > > > > > > > > > > > > > > ... which is where we come into shmem_fault(). Apart from the > > > > > > > horrendous hole-punch handling case, shmem_fault() is quite simple: > > > > > > > > > > > > > > err = shmem_get_folio_gfp(inode, vmf->pgoff, &folio, SGP_CACHE, > > > > > > > gfp, vma, vmf, &ret); > > > > > > > if (err) > > > > > > > return vmf_error(err); > > > > > > > vmf->page = folio_file_page(folio, vmf->pgoff); > > > > > > > return ret; > > > > > > > > > > > > > > What we could do here is detect this case. Something like: > > > > > > > > > > > > > > enum sgp_type sgp = SGP_CACHE; > > > > > > > > > > > > > > if ((vmf->flags & FAULT_FLAG_WRITE) && !(vma->vm_flags & VM_SHARED)) > > > > > > > sgp = SGP_READ; > > > > > > > > > > > > Yes this will start to save the space, but just to mention this may start > > > > > > to break anything that will still depend on the pagecache to work. E.g., > > > > > > it'll change behavior if the vma is registered with uffd missing mode; > > > > > > we'll start to lose MISSING events for these private mappings. Not sure > > > > > > whether there're other side effects. > > > > > > > > > > I don't follow, can you elaborate? > > > > > > > > > > hugetlb doesn't perform this kind of unnecessary allocation and should be fine in regards to uffd. Why should it matter here and how exactly would a problematic sequence look like? > > > > > > > > Hugetlb is special because hugetlb detects pte first and relies on pte at > > > > least for uffd. shmem is not. > > > > > > > > Feel free to also reference the recent fix which relies on the stable > > > > hugetlb pte with commit 2ea7ff1e39cbe375. > > > > > > Sorry to be dense here, but I don't follow how that relates. > > > > > > Assume we have a MAP_PRIVATE shmem mapping and someone registers uffd > > > missing events on that mapping. > > > > > > Assume we get a page fault on a hole. We detect no page is mapped and check > > > if the page cache has a page mapped -- which is also not the case, because > > > there is a hole. > > > > > > So we notify uffd. > > > > > > Uffd will place a page. It should *not* touch the page cache and only insert > > > that page into the page table -- otherwise we'd be violating MAP_PRIVATE > > > semantics. > > > > That's actually exactly what we do right now... we insert into page cache > > for the shmem. See shmem_mfill_atomic_pte(). > > > > Why it violates MAP_PRIVATE? Private pages only guarantee the exclusive > > ownership of pages, I don't see why it should restrict uffd behavior. Uffd > > missing mode (afaiu) is defined to resolve page cache missings in this > > case. Hugetlb is special but not shmem IMO comparing to most of the rest > > of the file systems. > > If a write (or uffd placement) via a MAP_PRIVATE mapping results in other > shared/private mappings from observing these modifications, you have a clear > violation of MAP_PRIVATE semantics. I think I understand what you meant, but just to mention again that I think it's a matter of how we defined the uffd missing semantics when shmem missing mode was introduced years ago. It does not need to be the same semantic as writting directly to a private mapping. -- Peter Xu
