Re: [PATCH v3 4/7] mm/gup: introduce the FOLL_SAME_FILE GUP flag

From: Lorenzo Stoakes <lstoakes@gmail.com>
To: David Hildenbrand <david@redhat.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Matthew Wilcox <willy@infradead.org>,
	Jens Axboe <axboe@kernel.dk>
Subject: Re: [PATCH v3 4/7] mm/gup: introduce the FOLL_SAME_FILE GUP flag
Date: Mon, 17 Apr 2023 12:27:22 +0100	[thread overview]
Message-ID: <e614a3c1-d933-40aa-9cee-a4175b5d0cec@lucifer.local> (raw)
In-Reply-To: <e4087551-c5d3-e82d-3c4e-dcfa476a971e@redhat.com>

On Mon, Apr 17, 2023 at 01:13:58PM +0200, David Hildenbrand wrote:
> On 15.04.23 14:09, Lorenzo Stoakes wrote:
> > This flag causes GUP to assert that all VMAs within the input range possess
> > the same vma->vm_file. If not, the operation fails.
> >
> > This is part of a patch series which eliminates the vmas parameter from the
> > GUP API, implementing the one remaining assertion within the entire kernel
> > that requires access to the VMAs associated with a GUP range.
> >
> > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
>
> [...]
>
> > ---
> >   						&start, &nr_pages, i,
> > @@ -1595,7 +1603,7 @@ long faultin_vma_page_range(struct vm_area_struct *vma, unsigned long start,
> >   	 * We want to report -EINVAL instead of -EFAULT for any permission
> >   	 * problems or incompatible mappings.
> >   	 */
> > -	if (check_vma_flags(vma, gup_flags))
> > +	if (check_vma_flags(vma, vma->vm_file, gup_flags))
> >   		return -EINVAL;
>
> FOLL_SAME_FILE is never set here, just pass NULL instead of vma->vm_file.
>
>
> As we're not allowing to drop the mmap lock, why can't io_uring simply go
> over all VMAs once, after pinning succeeded, and make sure that the files
> match (or even before pinning)?
>
> In most cases, we're dealing with a single VMA only, it's not like the
> common case is that io_uring pins accross 100s of VMAs.
>
> So I really wonder if the GUP complexity is justified by something.

This is one that needs some input from Jens I think (added to cc, for some
reason I didn't include him on this one but I did on the one updating uring
to use it).

I agree it'd be better not to introduce this flag if we can avoid it,
intent was to avoid having to add complexity to the uring code when we're
already iterating through VMAs in the GUP code, but as you say it's highly
likely most cases will consist of a single VMA anyway.

If Jens is OK with us iterating here I'm more than happy to respin without
adding the flag!

> (removing the VMAs is certainly a welcome surprise -- as it doesn't make any
> sense when used with FOLL_UNLOCKABLE).

This is a product of reading the GUP code when writing the GUP bit for the
book and wishing it were more sane :) I suspect I'll have some more patches
in this area aimed at marrying the disparate APIs where sensible to do so.

>
> --
> Thanks,
>
> David / dhildenb
>
>