Re: [PATCH V3 00/16] io_uring/ublk: add IORING_OP_FUSED_CMD

[Ming Lei <ming.lei@redhat.com>]

On Thu, Mar 16, 2023 at 11:13:39AM +0800, Xiaoguang Wang wrote:
> hi,
> 
> > Hello,
> >
> > Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to
> > be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd
> > 64byte SQE(slave) is another normal 64byte OP. For any OP which needs
> > to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1,
> > and its ->issue() can retrieve/import buffer from master request's
> > fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of
> > this idea is from Xiaoguang's ublk ebpf patchset, but this patchset
> > submits slave OP just like normal OP issued from userspace, that said,
> > SQE order is kept, and batching handling is done too.
> Thanks for this great work, seems that we're now in the right direction
> to support ublk zero copy, I believe this feature will improve io throughput
> greatly and reduce ublk's cpu resource usage.
> 
> I have gone through your 2th patch, and have some little concerns here:
> Say we have one ublk loop target device, but it has 4 backend files,
> every file will carry 25% of device capacity and it's implemented in stripped
> way, then for every io request, current implementation will need issed 4
> fused_cmd, right? 4 slave sqes are necessary, but it would be better to
> have just one master sqe, so I wonder whether we can have another

Yeah, the current approach needs 4 fused command with 4 slave request,
but from user viewpoint it is just 4 128byte SQEs.

It is pretty lightweight to handle master command, just calling io_fused_cmd_provide_kbuf()
for providing the buffer, so IMO it is fine to submit 4 fused command to handle
single stripped IO.

> method. The key point is to let io_uring support register various kernel
> memory objects, which come from kernel, such as ITER_BVEC or
> ITER_KVEC. so how about below actions:
> 1. add a new infrastructure in io_uring, which will support to register
> various kernel memory objects in it, this new infrastructure could be
> maintained in a xarray structure, every memory objects in it will have
> a unique id. This registration could be done in a ublk uring cmd, io_uring
> offers registration interface.
> 2. then any sqe can use these memory objects freely, so long as it
> passes above unique id in sqe properly.
> Above are just rough ideas, just for your reference.

I'd rather not add more complexity from the beginning, and IMO probably it
could be the most simple & generic way to handle it by single fused command,
at least the buffer lifetime/ownership won't cross multiple OPs.

Registering per-io buffer isn't free, Pavel actually mentioned the
idea, basically:

1) one OP is for registering buffer
2) another OP is for un-registering buffer

Then we still need 3+ OPs(SQEs) for handling single IO, not mention the buffer
has to be stored in global(per-ctx) data structure, and you have to pay
cost to read/write global data structure in IO fast path. In the case of
4 stripped underlying device, you still need 6 64byte SQEs for handling single io.

But in future if we don't have other better candidates and fused command can't
scale well, we can extend it or add new OPs for improving the multiple underlying
devices, but so far, not see the problem.

> 
> And current zero-copy method only supports raw data redirection, if

Yeah.

> ublk targets need to crc, compress, encrypt raw io requests' pages,
> then we'll still need to copy block layer's io data to userspace daemon.

Yes, zero copy can't cover all cases, that is why I add read/write
interface to support other cases, see patch 14, then userspace can
do whatwever they like.

Actually once zero copy is accepted, I'd suggest to mark the non-zc code path
as legacy, since the copy can be done explicitly in userspace by the added
read()/write(). And ublk driver can get simplified & cleaned, same with
userspace implementation.

> In that way, ebpf may give a help :) we directly operate block layer's
> io data in ebpf prog, doing crc or compress, encrypt, still does not need
> to copy to userspace daemon. But as you said before, ebpf may not
> support complicated user io logic, a much long way to go...

Of course, there can be lots of work for future improvement, and ebpf is
really one great weapon, but let's start effectively with something
reliable & simple.


thanks,
Ming