Re: [PATCH] 9p/virtio: restrict page pinning to user_backed_iter() iovec

[Christian Schoenebeck <linux_oss@crudebyte.com>]

On Monday, 15 December 2025 08:34:12 CET Dominique Martinet wrote:
> Thanks for having a look
> 
> Christoph Hellwig wrote on Sun, Dec 14, 2025 at 09:55:12PM -0800:
> > > Ok, I don't understand why the current code locks everything down and
> > > wants to use a single scatterlist shared for the whole channel (and
> > > capped to 128 pages?), it should only need to lock around the
> > > virtqueue_add_sg() call, I'll need to play with that some more.
> > 
> > What do you mean with "lock down"?
> 
> Just the odd (to me) use of the chan->lock around basically all of
> p9_virtio_request() and most of p9_virtio_zc_request() -- I'm not pretty
> sure this was just the author trying to avoid an allocation by recycling
> the chan->sg array around though, so ignore this.

The lock protects the channel wide, shared scatterlist while the scatterlist 
is filled from the linear buffers by pack_sg_list(). Then virtqueue_add_sgs() 
pushes scatterlist's segments as virtio descriptors into the virtio FIFOs. 
From this point it safe to unlock as the scatterlist is no longer needed.

And yes, the assumption probably was that handling the scatterlist as a 
temporary was more expensive due to allocation.

> > > Looking at other virtio drivers I could probably use a sg_table and
> > > have extract_iter_to_sg() do all the work for us...
> > 
> > Looking at the code I'm actually really confused.  Both because I
> > actually though we were talking about the 9fs direct I/O code, but
> > that has actually been removed / converted to netfs a long time ago.
> > 
> > But even more so what the net/9p code is actually doing..  How do
> > we even end up with user addresses here at all?
> 
> FWIW I tried logging and saw ITER_BVEC, ITER_KVEC and ITER_FOLIOQ --
> O_DIRECT writes are seen as BVEC so I guess it's not as direct as I
> expected them to be -- that code could very well be leftovers from
> the switch to iov_iter back in 2015...
> 
> (I'm actually not sure why Christian suggested checking for is_iovec()
> in https://lkml.kernel.org/r/2245723.irdbgypaU6@weasel -- then I
> generalized it to user_backed_iter() and it just worked because checking
> for that moved out bvec and folioq from iov_iter_get_pages_alloc2()
> to... something that obviously should not work in my opinion but
> apparently was enough to not trigger this particular BUG.)

Sorry, I should have explained why I suggested that change: My understanding 
of the p9_get_mapped_pages() code was that the original author intended to go 
down the iov_iter_get_pages_alloc2() path only for user space memory 
exclusively and that he assumed that his preceding !iov_iter_is_kvec(data) 
check would guard this appropriately. But apparently there are several other 
iterator types that are kernel memory as well. So I thought switching the 
check to is_iovec() would be better as these are user space memory, however I 
missed that there is also ITER_UBUF with user space memory, which you realized 
fortunately by suggesting user_backed_iter() check instead.

/Christian