On Wed, Aug 18, 2021 at 11:49 PM Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > [ Sorry for the delay, I was on the road and this fell through the cracks ] No harm done, I was busy enough implementing your previous suggestions. > On Mon, Aug 16, 2021 at 12:14 PM Andreas Gruenbacher > <agruenba@xxxxxxxxxx> wrote: > > > > On Tue, Aug 3, 2021 at 9:45 PM Linus Torvalds > > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > Hmm. Have you tried to figure out why that "still returns 0" happens? > > > > The call stack is: > > > > gup_pte_range > > gup_pmd_range > > gup_pud_range > > gup_p4d_range > > gup_pgd_range > > lockless_pages_from_mm > > internal_get_user_pages_fast > > get_user_pages_fast > > iov_iter_get_pages > > __bio_iov_iter_get_pages > > bio_iov_iter_get_pages > > iomap_dio_bio_actor > > iomap_dio_actor > > iomap_apply > > iomap_dio_rw > > gfs2_file_direct_write > > > > In gup_pte_range, pte_special(pte) is true and so we return 0. > > Ok, so that is indeed something that the fast-case can't handle, > because some of the special code wants to have the mm_lock so that it > can look at the vma flags (eg "vm_normal_page()" and friends. > > That said, some of these cases even the full GUP won't ever handle, > simply because a mapping doesn't necessarily even _have_ a 'struct > page' associated with it if it's a VM_IO mapping. > > So it turns out that you can't just always do > fault_in_iov_iter_readable() and then assume that you can do > iov_iter_get_pages() and repeat until successful. > > We could certainly make get_user_pages_fast() handle a few more cases, > but I get the feeling that we need to have separate error cases for > EFAULT - no page exists - and the "page exists, but cannot be mapped > as a 'struct page'" case. Hmm, what if GUP is made to skip VM_IO vmas without adding anything to the pages array? That would match fault_in_iov_iter_writeable, which is modeled after __mm_populate and which skips VM_IO and VM_PFNMAP vmas. The other strategy I've added is to scale back the page fault windows to a single page if faulting in multiple pages didn't help, and to give up if the I/O operation still fails after that. So pathological cases won't loop indefinitely anymore at least. > I also do still think that even regardless of that, we want to just > add a FOLL_NOFAULT flag that just disables calling handle_mm_fault(), > and then you can use the regular get_user_pages(). > > That at least gives us the full _normal_ page handling stuff. And it does fix the generic/208 failure. Thanks, Andreas
