On Wed, Apr 19, 2023 at 07:35:33PM +0100, Matthew Wilcox wrote: > On Wed, Apr 19, 2023 at 03:24:55PM -0300, Jason Gunthorpe wrote: > > On Wed, Apr 19, 2023 at 07:23:00PM +0100, Matthew Wilcox wrote: > > > On Wed, Apr 19, 2023 at 07:18:26PM +0100, Lorenzo Stoakes wrote: > > > > So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I > > > > would still need to come along and delete a bunch of your code > > > > afterwards. And unfortunately Pavel's recent change which insists on not > > > > having different vm_file's across VMAs for the buffer would have to be > > > > reverted so I expect it might not be entirely without discussion. > > > > > > I don't even understand why Pavel wanted to make this change. The > > > commit log really doesn't say. > > > > > > commit edd478269640 > > > Author: Pavel Begunkov <asml.silence@xxxxxxxxx> > > > Date: Wed Feb 22 14:36:48 2023 +0000 > > > > > > io_uring/rsrc: disallow multi-source reg buffers > > > > > > If two or more mappings go back to back to each other they can be passed > > > into io_uring to be registered as a single registered buffer. That would > > > even work if mappings came from different sources, e.g. it's possible to > > > mix in this way anon pages and pages from shmem or hugetlb. That is not > > > a problem but it'd rather be less prone if we forbid such mixing. > > > > > > Cc: <stable@xxxxxxxxxxxxxxx> > > > Signed-off-by: Pavel Begunkov <asml.silence@xxxxxxxxx> > > > Signed-off-by: Jens Axboe <axboe@xxxxxxxxx> > > > > > > It even says "That is not a problem"! So why was this patch merged > > > if it's not fixing a problem? > > > > > > It's now standing in the way of an actual cleanup. So why don't we > > > revert it? There must be more to it than this ... > > > > https://lore.kernel.org/all/61ded378-51a8-1dcb-b631-fda1903248a9@xxxxxxxxx/ > > So um, it's disallowed because Pavel couldn't understand why it > should be allowed? This gets less and less convincing. > > FWIW, what I was suggesting was that we should have a FOLL_SINGLE_VMA > flag, which would use our shiny new VMA lock infrastructure to look > up and lock _one_ VMA instead of having the caller take the mmap_lock. > Passing that flag would be a tighter restriction that Pavel implemented, > but would certainly relieve some of his mental load. > > By the way, even if all pages are from the same VMA, they may still be a > mixture of anon and file pages; think a MAP_PRIVATE of a file when > only some pages have been written to. Or an anon MAP_SHARED which is > accessible by a child process. Indeed, my motive for the series came out of a conversation with you about vmas being odd (thanks! :), however I did end up feeling FOLL_SINGLE_VMA would be too restricted and would break the uAPI. For example, imagine if a user (yes it'd be weird) mlock'd some pages in a buffer and not others, then we'd break their use case. Also (perhaps?) more feasibly, a user might mix hugetlb and anon pages. So I think that'd be too restrictive here. However the idea of just essentially taking what Jens has had to do open-coded and putting it into GUP as a whole really feels like the right thing to do. I do like the idea of a FOLL_SINGLE_VMA for other use cases though, the majority of which want one and one page only. Perhaps worth taking the helper added in this series (get_user_page_vma_remote() from [1]) and replacing it with an a full GUP function which has an interface explicitly for this common single page/vma case. [1]:https://lore.kernel.org/linux-mm/7c6f1ae88320bf11d2f583178a3d9e653e06ac63.1681831798.git.lstoakes@xxxxxxxxx/
