Re: [PATCH V6 4/8] io_uring: support SQE group

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

On Wed, Oct 09, 2024 at 12:53:34PM +0100, Pavel Begunkov wrote:
> On 10/6/24 04:54, Ming Lei wrote:
> > On Fri, Oct 04, 2024 at 02:12:28PM +0100, Pavel Begunkov wrote:
> > > On 9/12/24 11:49, Ming Lei wrote:
> > > ...
> > > > --- a/io_uring/io_uring.c
> > > > +++ b/io_uring/io_uring.c
> > > > @@ -111,13 +111,15 @@
> > > ...
> > > > +static void io_complete_group_member(struct io_kiocb *req)
> > > > +{
> > > > +	struct io_kiocb *lead = get_group_leader(req);
> > > > +
> > > > +	if (WARN_ON_ONCE(!(req->flags & REQ_F_SQE_GROUP) ||
> > > > +			 lead->grp_refs <= 0))
> > > > +		return;
> > > > +
> > > > +	/* member CQE needs to be posted first */
> > > > +	if (!(req->flags & REQ_F_CQE_SKIP))
> > > > +		io_req_commit_cqe(req->ctx, req);
> > > > +
> > > > +	req->flags &= ~REQ_F_SQE_GROUP;
> > > 
> > > I can't say I like this implicit state machine too much,
> > > but let's add a comment why we need to clear it. i.e.
> > > it seems it wouldn't be needed if not for the
> > > mark_last_group_member() below that puts it back to tunnel
> > > the leader to io_free_batch_list().
> > 
> > Yeah, the main purpose is for reusing the flag for marking last
> > member, will add comment for this usage.
> > 
> > > 
> > > > +
> > > > +	/* Set leader as failed in case of any member failed */
> > > > +	if (unlikely((req->flags & REQ_F_FAIL)))
> > > > +		req_set_fail(lead);
> > > > +
> > > > +	if (!--lead->grp_refs) {
> > > > +		mark_last_group_member(req);
> > > > +		if (!(lead->flags & REQ_F_CQE_SKIP))
> > > > +			io_req_commit_cqe(lead->ctx, lead);
> > > > +	} else if (lead->grp_refs == 1 && (lead->flags & REQ_F_SQE_GROUP)) {
> > > > +		/*
> > > > +		 * The single uncompleted leader will degenerate to plain
> > > > +		 * request, so group leader can be always freed via the
> > > > +		 * last completed member.
> > > > +		 */
> > > > +		lead->flags &= ~REQ_F_SQE_GROUP_LEADER;
> > > 
> > > What does this try to handle? A group with a leader but no
> > > members? If that's the case, io_group_sqe() and io_submit_state_end()
> > > just need to fail such groups (and clear REQ_F_SQE_GROUP before
> > > that).
> > 
> > The code block allows to issue leader and members concurrently, but
> > we have changed to always issue members after leader is completed, so
> > the above code can be removed now.
> 
> One case to check, what if the user submits just a single request marked
> as a group? The concern is that we create a group with a leader but
> without members otherwise, and when the leader goes through
> io_submit_flush_completions for the first time it drops it refs and
> starts waiting for members that don't exist to "wake" it. I mentioned
> above we should probably just fail it, but would be nice to have a
> test for it if not already.

The corner case isn't handled yet, and we can fail it by calling
req_fail_link_node(head, -EINVAL) in io_submit_state_end().


thanks,
Ming