Re: (subset) [PATCH V10 0/12] io_uring: support group buffer & ublk zc

[Jens Axboe <axboe@kernel.dk>]

On 11/13/24 6:43 AM, Pavel Begunkov wrote:
> On 11/12/24 00:53, Ming Lei wrote:
>> On Thu, Nov 07, 2024 at 03:25:59PM -0700, Jens Axboe wrote:
>>> On 11/7/24 3:25 PM, Jens Axboe wrote:
> ...
>> Hi Jens,
>>
>> Any comment on the rest of the series?
> 
> Ming, it's dragging on because it's over complicated. I very much want
> it to get to some conclusion, get it merged and move on, and I strongly
> believe Jens shares the sentiment on getting the thing done.
> 
> Please, take the patches attached, adjust them to your needs and put
> ublk on top. Or tell if there is a strong reason why it doesn't work.
> The implementation is very simple and doesn't need almost anything
> from io_uring, it's low risk and we can merge in no time.

Indeed, nobody would love to get this moving forward more than me!
Pavel, can you setup a branch with the required patches? Should be your
two and then the buf update and mapping bits I did earlier. I can do it
too. Ming, would you mind if we try and setup a base that we can do this
on more trivially? I had merged the sqe grouping, but the more I think
about it, the less I like the added complexity, and the limitations we
had to put in there because relationships weren't fully understandable.

With the #1 goal of getting leased/borrowed buffers working asap, here's
what I suggest:

1) Pavel/I provide a base for doing the bare minimum, which is having an
   ephemeral buffer that zc can use.
2) You do the ublk bits on top

Yes this won't have grouping, so buf update will have to be done
separately. The downside here is that it'll add a (tiny) bit of overhead
as there's an extra sqe involved, but I don't really think that's an
issue, not even a minor one. The main objective here is NOT copying the
data, which will dwarf any other tiny extra overhead added.

This avoids introducing sqe grouping as a concept as a requirement for
zero copy ublk, which I did mention earlier is part of the complication
here. I'm a BIG fan of keeping things simple initially, particularly
when it adds major dependency complexity to the core code.

The goal here is doing the simple buffer leasing in such a way that it's
trivially understandable, and doesn't depend on grouping. With that, we
can easily get this done for the 6.14 kernel and finally ship it. I wish
6.13 was a week more away because then I think we could get it in for
6.13, but we only really have a few days at this point, so it's a bit
late. Unfortunately!

Ming, what do you think? Let's get this sorted so we can move on to
actually being able to use zc ublk, which is the goal we all share here.

> If you can't cache the allocation in ublk, io_uring can add a cache.
> If ublk needs more space and cannot embed the structure, we can add
> a "private" pointer into io_mapped_ubuf. If it needs to check the IO
> direction, we can add that as well (though I have doubts you really need
> it, read-only might makes sense, write-only not so much). We'll also
> merge Jens' patch allowing to remove a buffer with a request.

Right, we can always improve the little things as we go forward and make
it more efficient. I do think it's diminishing returns at that point,
but that doesn't mean we should not do it and make it better. But first,
let's get the concept working.

That's just violent agreement btw, I think we all share that objective
:-)

-- 
Jens Axboe