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
