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

[Pavel Begunkov <asml.silence@gmail.com>]

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.

Forgot to mention, with the mentioned changes I believe the patch
should be good enough.

-- 
Pavel Begunkov