Re: [PATCH V2 00/17] io_uring/ublk: add IORING_OP_FUSED_CMD

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

On Wed, Mar 08, 2023 at 04:22:15PM +0000, Pavel Begunkov wrote:
> On 3/8/23 01:08, Ming Lei wrote:
> > On Tue, Mar 07, 2023 at 03:37:21PM +0000, Pavel Begunkov wrote:
> > > On 3/7/23 14:15, Ming Lei wrote:
> > > > 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.
> > > 
> > >  From a quick look through patches it all looks a bit complicated
> > > and intrusive, all over generic hot paths. I think instead we
> > 
> > Really? The main change to generic hot paths are adding one 'true/false'
> > parameter to io_init_req(). For others, the change is just check on
> > req->flags or issue_flags, which is basically zero cost.
> 
> Extra flag in io_init_req() but also exporting it, which is an
> internal function, to non-core code. Additionally it un-inlines it

We can make it inline for core code only.

> and even looks recurse calls it (max depth 2). From a quick look,

The reurse call is only done for fused command, and won't be one
issue for normal OPs.

> there is some hand coded ->cached_refs manipulations, it takes extra
> space in generic sections of io_kiocb.

Yeah, but it is still done on fused command only. I think people
is happy to pay the cost for the benefit, and we do not cause trouble
for others.

> It makes all cmd users to
> check for IO_URING_F_FUSED. There is also a two-way dependency b/w

The check is zero cost, and just for avoiding to add ->fused_cmd() callback,
otherwise the check can be killed.

> requests, which never plays out well, e.g. I still hate how linked
> timeouts stick out in generic paths.

I appreciate you may explain it in details.

Yeah, part of fused command's job is to submit one new io and wait its completion.
slave request is actually invisible in the linked list, and only fused
command can be in the linked list.

> 
> Depending on SQE128 also doesn't seem right, though it can be dealt
> with, e.g. sth like how it's done with links requests.

I thought about handling it by linked request, but we need fused command to be
completed after the slave request is done, and that becomes one deadlock if
the two are linked together.

SQE128 is per-context feature, when we need to submit uring SQE128 command, the
same ring is required to handle IO, then IMO it is perfect for this
case, at least for ublk.

> 
> > > should be able to use registered buffer table as intermediary and
> > > reuse splicing. Let me try it out
> > 
> > I will take a look at you patch, but last time, Linus has pointed out that
> > splice isn't one good way, in which buffer ownership transferring is one big
> > issue for writing data to page retrieved from pipe.
> 
> There are no real pipes, better to say io_uring replaces a pipe,
> and splice bits are used to get pages from a file. Though, there
> will be some common problems. Thanks for the link, I'll need to
> get through it first, thanks for the link

Yeah, here the only value of pipe is to reuse ->splice_read() interface,
that is why I figure out fused command for this job. I am open for
other approaches, if the problem can be solved(reliably and efficiently).

Thanks, 
Ming