| Message ID | 20240808162503.345913-5-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | io_uring: support sqe group and provide group kbuf | expand |
On 8/8/24 17:24, Ming Lei wrote: > SQE group is defined as one chain of SQEs starting with the first SQE that > has IOSQE_SQE_GROUP set, and ending with the first subsequent SQE that > doesn't have it set, and it is similar with chain of linked SQEs. > > Not like linked SQEs, each sqe is issued after the previous one is > completed. All SQEs in one group can be submitted in parallel. To simplify > the implementation from beginning, all members are queued after the leader > is completed, however, this way may be changed and leader and members may > be issued concurrently in future. > > The 1st SQE is group leader, and the other SQEs are group member. The whole > group share single IOSQE_IO_LINK and IOSQE_IO_DRAIN from group leader, and > the two flags can't be set for group members. > > When the group is in one link chain, this group isn't submitted until the > previous SQE or group is completed. And the following SQE or group can't > be started if this group isn't completed. Failure from any group member will > fail the group leader, then the link chain can be terminated. > > When IOSQE_IO_DRAIN is set for group leader, all requests in this group and > previous requests submitted are drained. Given IOSQE_IO_DRAIN can be set for > group leader only, we respect IO_DRAIN by always completing group leader as > the last one in the group. Meantime it is natural to post leader's CQE > as the last one from application viewpoint. > > Working together with IOSQE_IO_LINK, SQE group provides flexible way to > support N:M dependency, such as: > > - group A is chained with group B together > - group A has N SQEs > - group B has M SQEs > > then M SQEs in group B depend on N SQEs in group A. > > N:M dependency can support some interesting use cases in efficient way: > > 1) read from multiple files, then write the read data into single file > > 2) read from single file, and write the read data into multiple files > > 3) write same data into multiple files, and read data from multiple files and > compare if correct data is written > > Also IOSQE_SQE_GROUP takes the last bit in sqe->flags, but we still can > extend sqe->flags with one uring context flag, such as use __pad3 for > non-uring_cmd OPs and part of uring_cmd_flags for uring_cmd OP. > > Suggested-by: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > include/linux/io_uring_types.h | 18 +++ ... > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c > index f112e9fa90d8..45a292445b18 100644 > --- a/io_uring/io_uring.c > +++ b/io_uring/io_uring.c ... > @@ -875,6 +877,116 @@ static __always_inline void io_req_commit_cqe(struct io_ring_ctx *ctx, > } > } ... > +static void io_queue_group_members(struct io_kiocb *req) > +{ > + struct io_kiocb *member = req->grp_link; > + > + if (!member) > + return; > + > + req->grp_link = NULL; > + while (member) { > + struct io_kiocb *next = member->grp_link; > + > + member->grp_leader = req; > + if (unlikely(member->flags & REQ_F_FAIL)) { > + io_req_task_queue_fail(member, member->cqe.res); > + } else if (unlikely(req->flags & REQ_F_FAIL)) { > + io_req_task_queue_fail(member, -ECANCELED); > + } else { > + io_req_task_queue(member); > + } > + member = next; > + } > +} > + > +static inline bool __io_complete_group_member(struct io_kiocb *req, > + struct io_kiocb *lead) > +{ I think it'd be better if you inline this function, it only obfuscates things. > + if (WARN_ON_ONCE(lead->grp_refs <= 0)) > + return false; > + > + req->flags &= ~REQ_F_SQE_GROUP; I'm getting completely lost when and why it clears and sets back REQ_F_SQE_GROUP and REQ_F_SQE_GROUP_LEADER. Is there any rule? > + /* > + * Set linked leader as failed if any member is failed, so > + * the remained link chain can be terminated > + */ > + if (unlikely((req->flags & REQ_F_FAIL) && > + ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link))) > + req_set_fail(lead); if (req->flags & REQ_F_FAIL) req_set_fail(lead); REQ_F_FAIL is not specific to links, if a request fails we need to mark it as such. > + return !--lead->grp_refs; > +} > + > +static inline bool leader_is_the_last(struct io_kiocb *lead) > +{ > + return lead->grp_refs == 1 && (lead->flags & REQ_F_SQE_GROUP); > +} > + > +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))) > + return; > + > + /* member CQE needs to be posted first */ > + if (!(req->flags & REQ_F_CQE_SKIP)) > + io_req_commit_cqe(req->ctx, req); > + > + if (__io_complete_group_member(req, lead)) { > + /* > + * SQE_GROUP flag is kept for the last member, so the leader > + * can be retrieved & freed from this last member > + */ > + req->flags |= REQ_F_SQE_GROUP; > + if (!(lead->flags & REQ_F_CQE_SKIP)) > + io_req_commit_cqe(lead->ctx, lead); > + } else if (leader_is_the_last(lead)) { > + /* leader will degenerate to plain req if it is the last */ > + lead->flags &= ~(REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER); What's this chunk is about? > + } > +} > + > +static void io_complete_group_leader(struct io_kiocb *req) > +{ > + WARN_ON_ONCE(req->grp_refs <= 0); > + req->flags &= ~REQ_F_SQE_GROUP; > + req->grp_refs -= 1; > + WARN_ON_ONCE(req->grp_refs == 0); Why not combine these WARN_ON_ONCE into one? > + > + /* TODO: queue members with leader in parallel */ no todos, please > + if (req->grp_link) > + io_queue_group_members(req); > +} It's spinlock'ed, we really don't want to do too much here like potentially queueing a ton of task works. io_queue_group_members() can move into io_free_batch_list(). > + > static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags) > { > struct io_ring_ctx *ctx = req->ctx; > @@ -890,7 +1002,8 @@ static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags) > * Handle special CQ sync cases via task_work. DEFER_TASKRUN requires > * the submitter task context, IOPOLL protects with uring_lock. > */ > - if (ctx->task_complete || (ctx->flags & IORING_SETUP_IOPOLL)) { > + if (ctx->task_complete || (ctx->flags & IORING_SETUP_IOPOLL) || > + req_is_group_leader(req)) { > req->io_task_work.func = io_req_task_complete; > io_req_task_work_add(req); > return; > @@ -1388,11 +1501,33 @@ static void io_free_batch_list(struct io_ring_ctx *ctx, > comp_list); > > if (unlikely(req->flags & IO_REQ_CLEAN_SLOW_FLAGS)) { > + if (req->flags & (REQ_F_SQE_GROUP | > + REQ_F_SQE_GROUP_LEADER)) { > + struct io_kiocb *leader; > + > + /* Leader is freed via the last member */ > + if (req_is_group_leader(req)) { > + node = req->comp_list.next; > + continue; > + } > + > + /* > + * Only the last member keeps GROUP flag, > + * free leader and this member together > + */ > + leader = get_group_leader(req); > + leader->flags &= ~REQ_F_SQE_GROUP_LEADER; > + req->flags &= ~REQ_F_SQE_GROUP; > + wq_stack_add_head(&leader->comp_list, > + &req->comp_list); That's quite hacky, but at least we can replace it with task work if it gets in the way later on. > + } > + > if (req->flags & REQ_F_REFCOUNT) { > node = req->comp_list.next; > if (!req_ref_put_and_test(req)) > continue; > } > + > if ((req->flags & REQ_F_POLLED) && req->apoll) { > struct async_poll *apoll = req->apoll; > > @@ -1427,8 +1562,19 @@ void __io_submit_flush_completions(struct io_ring_ctx *ctx) > struct io_kiocb *req = container_of(node, struct io_kiocb, > comp_list); > > - if (!(req->flags & REQ_F_CQE_SKIP)) > - io_req_commit_cqe(ctx, req); > + if (unlikely(req->flags & (REQ_F_CQE_SKIP | REQ_F_SQE_GROUP))) { > + if (req->flags & REQ_F_SQE_GROUP) { > + if (req_is_group_leader(req)) > + io_complete_group_leader(req); > + else > + io_complete_group_member(req); > + continue; > + } > + > + if (req->flags & REQ_F_CQE_SKIP) > + continue; > + } > + io_req_commit_cqe(ctx, req); > } > __io_cq_unlock_post(ctx); > ... > @@ -2101,6 +2251,62 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req, > return def->prep(req, sqe); > } > > +static struct io_kiocb *io_group_sqe(struct io_submit_link *group, > + struct io_kiocb *req) > +{ > + /* > + * Group chain is similar with link chain: starts with 1st sqe with > + * REQ_F_SQE_GROUP, and ends with the 1st sqe without REQ_F_SQE_GROUP > + */ > + if (group->head) { > + struct io_kiocb *lead = group->head; > + > + /* members can't be in link chain, can't be drained */ > + if (req->flags & (IO_REQ_LINK_FLAGS | REQ_F_IO_DRAIN)) > + req_fail_link_node(lead, -EINVAL); That should fail the entire link (if any) as well. I have even more doubts we even want to mix links and groups. Apart from nuances as such, which would be quite hard to track, the semantics of IOSQE_CQE_SKIP_SUCCESS is unclear. And also it doen't work with IORING_OP_LINK_TIMEOUT. > + > + lead->grp_refs += 1; > + group->last->grp_link = req; > + group->last = req; > + > + if (req->flags & REQ_F_SQE_GROUP) > + return NULL; > + > + req->grp_link = NULL; > + req->flags |= REQ_F_SQE_GROUP; > + group->head = NULL; > + if (lead->flags & REQ_F_FAIL) { > + io_queue_sqe_fallback(lead); Let's say the group was in the middle of a link, it'll complete that group and continue with assembling / executing the link when it should've failed it and honoured the request order. > + return NULL; > + } > + return lead; > + } else if (req->flags & REQ_F_SQE_GROUP) { > + group->head = req; > + group->last = req; > + req->grp_refs = 1; > + req->flags |= REQ_F_SQE_GROUP_LEADER; > + return NULL; > + } else { We shouldn't be able to get here. if (WARN_ONCE(!(req->flags & GROUP))) ... group->head = req; ... > + return req; > + } > +} > + > +static __cold struct io_kiocb *io_submit_fail_group( > + struct io_submit_link *link, struct io_kiocb *req) > +{ > + struct io_kiocb *lead = link->head; > + > + /* > + * Instead of failing eagerly, continue assembling the group link > + * if applicable and mark the leader with REQ_F_FAIL. The group > + * flushing code should find the flag and handle the rest > + */ > + if (lead && (lead->flags & IO_REQ_LINK_FLAGS) && !(lead->flags & REQ_F_FAIL)) > + req_fail_link_node(lead, -ECANCELED); Same as above, you don't need to check for IO_REQ_LINK_FLAGS. io_queue_sqe_fallback() > + > + return io_group_sqe(link, req); > +} > +
Hi Pavel, Thanks for review! On Tue, Aug 27, 2024 at 04:18:26PM +0100, Pavel Begunkov wrote: > On 8/8/24 17:24, Ming Lei wrote: > > SQE group is defined as one chain of SQEs starting with the first SQE that > > has IOSQE_SQE_GROUP set, and ending with the first subsequent SQE that > > doesn't have it set, and it is similar with chain of linked SQEs. > > > > Not like linked SQEs, each sqe is issued after the previous one is > > completed. All SQEs in one group can be submitted in parallel. To simplify > > the implementation from beginning, all members are queued after the leader > > is completed, however, this way may be changed and leader and members may > > be issued concurrently in future. > > > > The 1st SQE is group leader, and the other SQEs are group member. The whole > > group share single IOSQE_IO_LINK and IOSQE_IO_DRAIN from group leader, and > > the two flags can't be set for group members. > > > > When the group is in one link chain, this group isn't submitted until the > > previous SQE or group is completed. And the following SQE or group can't > > be started if this group isn't completed. Failure from any group member will > > fail the group leader, then the link chain can be terminated. > > > > When IOSQE_IO_DRAIN is set for group leader, all requests in this group and > > previous requests submitted are drained. Given IOSQE_IO_DRAIN can be set for > > group leader only, we respect IO_DRAIN by always completing group leader as > > the last one in the group. Meantime it is natural to post leader's CQE > > as the last one from application viewpoint. > > > > Working together with IOSQE_IO_LINK, SQE group provides flexible way to > > support N:M dependency, such as: > > > > - group A is chained with group B together > > - group A has N SQEs > > - group B has M SQEs > > > > then M SQEs in group B depend on N SQEs in group A. > > > > N:M dependency can support some interesting use cases in efficient way: > > > > 1) read from multiple files, then write the read data into single file > > > > 2) read from single file, and write the read data into multiple files > > > > 3) write same data into multiple files, and read data from multiple files and > > compare if correct data is written > > > > Also IOSQE_SQE_GROUP takes the last bit in sqe->flags, but we still can > > extend sqe->flags with one uring context flag, such as use __pad3 for > > non-uring_cmd OPs and part of uring_cmd_flags for uring_cmd OP. > > > > Suggested-by: Kevin Wolf <kwolf@redhat.com> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > include/linux/io_uring_types.h | 18 +++ > ... > > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c > > index f112e9fa90d8..45a292445b18 100644 > > --- a/io_uring/io_uring.c > > +++ b/io_uring/io_uring.c > ... > > @@ -875,6 +877,116 @@ static __always_inline void io_req_commit_cqe(struct io_ring_ctx *ctx, > > } > > } > ... > > +static void io_queue_group_members(struct io_kiocb *req) > > +{ > > + struct io_kiocb *member = req->grp_link; > > + > > + if (!member) > > + return; > > + > > + req->grp_link = NULL; > > + while (member) { > > + struct io_kiocb *next = member->grp_link; > > + > > + member->grp_leader = req; > > + if (unlikely(member->flags & REQ_F_FAIL)) { > > + io_req_task_queue_fail(member, member->cqe.res); > > + } else if (unlikely(req->flags & REQ_F_FAIL)) { > > + io_req_task_queue_fail(member, -ECANCELED); > > + } else { > > + io_req_task_queue(member); > > + } > > + member = next; > > + } > > +} > > + > > +static inline bool __io_complete_group_member(struct io_kiocb *req, > > + struct io_kiocb *lead) > > +{ > > I think it'd be better if you inline this function, it only > obfuscates things. OK. > > > + if (WARN_ON_ONCE(lead->grp_refs <= 0)) > > + return false; > > + > > + req->flags &= ~REQ_F_SQE_GROUP; > > I'm getting completely lost when and why it clears and sets > back REQ_F_SQE_GROUP and REQ_F_SQE_GROUP_LEADER. Is there any > rule? My fault, it should have been documented somewhere. REQ_F_SQE_GROUP is cleared when the request is completed, but it is reused as flag for marking the last request in this group, so we can free the group leader when observing the 'last' member request. The only other difference about the two flags is that both are cleared when the group leader becomes the last one in the group, then this leader degenerates as normal request, which way can simplify group leader freeing. > > > + /* > > + * Set linked leader as failed if any member is failed, so > > + * the remained link chain can be terminated > > + */ > > + if (unlikely((req->flags & REQ_F_FAIL) && > > + ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link))) > > + req_set_fail(lead); > > if (req->flags & REQ_F_FAIL) > req_set_fail(lead); > > REQ_F_FAIL is not specific to links, if a request fails we need > to mark it as such. It is for handling group failure. The following condition ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link)) means that this group is in one link-chain. If any member in this group is failed, we need to fail this group(lead), then the remained requests in this chain can be failed. Otherwise, it isn't necessary to fail group leader in case of any member io failure. > > > > + return !--lead->grp_refs; > > +} > > + > > +static inline bool leader_is_the_last(struct io_kiocb *lead) > > +{ > > + return lead->grp_refs == 1 && (lead->flags & REQ_F_SQE_GROUP); > > +} > > + > > +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))) > > + return; > > + > > + /* member CQE needs to be posted first */ > > + if (!(req->flags & REQ_F_CQE_SKIP)) > > + io_req_commit_cqe(req->ctx, req); > > + > > + if (__io_complete_group_member(req, lead)) { > > + /* > > + * SQE_GROUP flag is kept for the last member, so the leader > > + * can be retrieved & freed from this last member > > + */ > > + req->flags |= REQ_F_SQE_GROUP; 'req' is the last completed request, so mark it as the last one by reusing REQ_F_SQE_GROUP, so we can free group leader in io_free_batch_list() when observing the last flag. But it should have been documented. > > + if (!(lead->flags & REQ_F_CQE_SKIP)) > > + io_req_commit_cqe(lead->ctx, lead); > > + } else if (leader_is_the_last(lead)) { > > + /* leader will degenerate to plain req if it is the last */ > > + lead->flags &= ~(REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER); > > What's this chunk is about? The leader becomes the only request not completed in group, so it is degenerated as normal one by clearing the two flags. This way simplifies logic for completing group leader. > > > + } > > +} > > + > > +static void io_complete_group_leader(struct io_kiocb *req) > > +{ > > + WARN_ON_ONCE(req->grp_refs <= 0); > > + req->flags &= ~REQ_F_SQE_GROUP; > > + req->grp_refs -= 1; > > + WARN_ON_ONCE(req->grp_refs == 0); > > Why not combine these WARN_ON_ONCE into one? OK. > > > + > > + /* TODO: queue members with leader in parallel */ > > no todos, please Fine, will remove the todos. > > > + if (req->grp_link) > > + io_queue_group_members(req); > > +} > > It's spinlock'ed, we really don't want to do too much here > like potentially queueing a ton of task works. > io_queue_group_members() can move into io_free_batch_list(). Fair enough, will do it in V6. > > > + > > static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags) > > { > > struct io_ring_ctx *ctx = req->ctx; > > @@ -890,7 +1002,8 @@ static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags) > > * Handle special CQ sync cases via task_work. DEFER_TASKRUN requires > > * the submitter task context, IOPOLL protects with uring_lock. > > */ > > - if (ctx->task_complete || (ctx->flags & IORING_SETUP_IOPOLL)) { > > + if (ctx->task_complete || (ctx->flags & IORING_SETUP_IOPOLL) || > > + req_is_group_leader(req)) { > > req->io_task_work.func = io_req_task_complete; > > io_req_task_work_add(req); > > return; > > @@ -1388,11 +1501,33 @@ static void io_free_batch_list(struct io_ring_ctx *ctx, > > comp_list); > > if (unlikely(req->flags & IO_REQ_CLEAN_SLOW_FLAGS)) { > > + if (req->flags & (REQ_F_SQE_GROUP | > > + REQ_F_SQE_GROUP_LEADER)) { > > + struct io_kiocb *leader; > > + > > + /* Leader is freed via the last member */ > > + if (req_is_group_leader(req)) { > > + node = req->comp_list.next; > > + continue; > > + } > > + > > + /* > > + * Only the last member keeps GROUP flag, > > + * free leader and this member together > > + */ > > + leader = get_group_leader(req); > > + leader->flags &= ~REQ_F_SQE_GROUP_LEADER; > > + req->flags &= ~REQ_F_SQE_GROUP; > > + wq_stack_add_head(&leader->comp_list, > > + &req->comp_list); > > That's quite hacky, but at least we can replace it with > task work if it gets in the way later on. io_free_batch_list() is already called in task context, and it isn't necessary to schedule one extra tw, which hurts perf more or less. Another way is to store these leaders into one temp list, and call io_free_batch_list() for this temp list one more time. > > > + } > > + > > if (req->flags & REQ_F_REFCOUNT) { > > node = req->comp_list.next; > > if (!req_ref_put_and_test(req)) > > continue; > > } > > + > > if ((req->flags & REQ_F_POLLED) && req->apoll) { > > struct async_poll *apoll = req->apoll; > > @@ -1427,8 +1562,19 @@ void __io_submit_flush_completions(struct io_ring_ctx *ctx) > > struct io_kiocb *req = container_of(node, struct io_kiocb, > > comp_list); > > - if (!(req->flags & REQ_F_CQE_SKIP)) > > - io_req_commit_cqe(ctx, req); > > + if (unlikely(req->flags & (REQ_F_CQE_SKIP | REQ_F_SQE_GROUP))) { > > + if (req->flags & REQ_F_SQE_GROUP) { > > + if (req_is_group_leader(req)) > > + io_complete_group_leader(req); > > + else > > + io_complete_group_member(req); > > + continue; > > + } > > + > > + if (req->flags & REQ_F_CQE_SKIP) > > + continue; > > + } > > + io_req_commit_cqe(ctx, req); > > } > > __io_cq_unlock_post(ctx); > ... > > @@ -2101,6 +2251,62 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req, > > return def->prep(req, sqe); > > } > > +static struct io_kiocb *io_group_sqe(struct io_submit_link *group, > > + struct io_kiocb *req) > > +{ > > + /* > > + * Group chain is similar with link chain: starts with 1st sqe with > > + * REQ_F_SQE_GROUP, and ends with the 1st sqe without REQ_F_SQE_GROUP > > + */ > > + if (group->head) { > > + struct io_kiocb *lead = group->head; > > + > > + /* members can't be in link chain, can't be drained */ > > + if (req->flags & (IO_REQ_LINK_FLAGS | REQ_F_IO_DRAIN)) > > + req_fail_link_node(lead, -EINVAL); > > That should fail the entire link (if any) as well. Good catch, here we should fail link head by following the logic in io_submit_fail_init(). > > I have even more doubts we even want to mix links and groups. Apart Wrt. ublk, group provides zero copy, and the ublk io(group) is generic IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd, send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK for generic io_uring IO. > from nuances as such, which would be quite hard to track, the semantics > of IOSQE_CQE_SKIP_SUCCESS is unclear. IO group just follows every normal request. 1) fail in linked chain - follows IO_LINK's behavior since io_fail_links() covers io group 2) otherwise - just respect IOSQE_CQE_SKIP_SUCCESS > And also it doen't work with IORING_OP_LINK_TIMEOUT. REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I will document it in V6. > > > + > > + lead->grp_refs += 1; > > + group->last->grp_link = req; > > + group->last = req; > > + > > + if (req->flags & REQ_F_SQE_GROUP) > > + return NULL; > > + > > + req->grp_link = NULL; > > + req->flags |= REQ_F_SQE_GROUP; > > + group->head = NULL; > > + if (lead->flags & REQ_F_FAIL) { > > + io_queue_sqe_fallback(lead); > > Let's say the group was in the middle of a link, it'll > complete that group and continue with assembling / executing > the link when it should've failed it and honoured the > request order. OK, here we can simply remove the above two lines, and link submit state can handle this failure in link chain. > > > > + return NULL; > > + } > > + return lead; > > + } else if (req->flags & REQ_F_SQE_GROUP) { > > + group->head = req; > > + group->last = req; > > + req->grp_refs = 1; > > + req->flags |= REQ_F_SQE_GROUP_LEADER; > > + return NULL; > > + } else { > > We shouldn't be able to get here. > > if (WARN_ONCE(!(req->flags & GROUP))) > ... > group->head = req; > ... OK. > > > + return req; > > + } > > +} > > + > > +static __cold struct io_kiocb *io_submit_fail_group( > > + struct io_submit_link *link, struct io_kiocb *req) > > +{ > > + struct io_kiocb *lead = link->head; > > + > > + /* > > + * Instead of failing eagerly, continue assembling the group link > > + * if applicable and mark the leader with REQ_F_FAIL. The group > > + * flushing code should find the flag and handle the rest > > + */ > > + if (lead && (lead->flags & IO_REQ_LINK_FLAGS) && !(lead->flags & REQ_F_FAIL)) > > + req_fail_link_node(lead, -ECANCELED); > > Same as above, you don't need to check for IO_REQ_LINK_FLAGS. > io_queue_sqe_fallback() OK. Thanks, Ming
On 8/29/24 05:29, Ming Lei wrote: ... >>> + if (WARN_ON_ONCE(lead->grp_refs <= 0)) >>> + return false; >>> + >>> + req->flags &= ~REQ_F_SQE_GROUP; >> >> I'm getting completely lost when and why it clears and sets >> back REQ_F_SQE_GROUP and REQ_F_SQE_GROUP_LEADER. Is there any >> rule? > > My fault, it should have been documented somewhere. > > REQ_F_SQE_GROUP is cleared when the request is completed, but it is > reused as flag for marking the last request in this group, so we can > free the group leader when observing the 'last' member request. Maybe it'd be cleaner to use a second flag? > The only other difference about the two flags is that both are cleared > when the group leader becomes the last one in the group, then > this leader degenerates as normal request, which way can simplify > group leader freeing. > >> >>> + /* >>> + * Set linked leader as failed if any member is failed, so >>> + * the remained link chain can be terminated >>> + */ >>> + if (unlikely((req->flags & REQ_F_FAIL) && >>> + ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link))) >>> + req_set_fail(lead); >> >> if (req->flags & REQ_F_FAIL) >> req_set_fail(lead); >> >> REQ_F_FAIL is not specific to links, if a request fails we need >> to mark it as such. > > It is for handling group failure. > > The following condition > > ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link)) > > means that this group is in one link-chain. > > If any member in this group is failed, we need to fail this group(lead), > then the remained requests in this chain can be failed. > > Otherwise, it isn't necessary to fail group leader in case of any member > io failure. What bad would happen if you do it like this? if (req->flags & REQ_F_FAIL) req_set_fail(lead); I'm asking because if you rely on some particular combination of F_FAIL and F_LINK somewhere, it's likely wrong, but otherwise we F_FAIL a larger set of requests, which should never be an issue. >>> + return !--lead->grp_refs; >>> +} >>> + >>> +static inline bool leader_is_the_last(struct io_kiocb *lead) >>> +{ >>> + return lead->grp_refs == 1 && (lead->flags & REQ_F_SQE_GROUP); >>> +} >>> + >>> +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))) >>> + return; >>> + >>> + /* member CQE needs to be posted first */ >>> + if (!(req->flags & REQ_F_CQE_SKIP)) >>> + io_req_commit_cqe(req->ctx, req); >>> + >>> + if (__io_complete_group_member(req, lead)) { >>> + /* >>> + * SQE_GROUP flag is kept for the last member, so the leader >>> + * can be retrieved & freed from this last member >>> + */ >>> + req->flags |= REQ_F_SQE_GROUP; > > 'req' is the last completed request, so mark it as the last one > by reusing REQ_F_SQE_GROUP, so we can free group leader in > io_free_batch_list() when observing the last flag. > > But it should have been documented. > >>> + if (!(lead->flags & REQ_F_CQE_SKIP)) >>> + io_req_commit_cqe(lead->ctx, lead); >>> + } else if (leader_is_the_last(lead)) { >>> + /* leader will degenerate to plain req if it is the last */ >>> + lead->flags &= ~(REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER); >> >> What's this chunk is about? > > The leader becomes the only request not completed in group, so it is > degenerated as normal one by clearing the two flags. This way simplifies > logic for completing group leader. > ... >>> @@ -1388,11 +1501,33 @@ static void io_free_batch_list(struct io_ring_ctx *ctx, >>> comp_list); >>> if (unlikely(req->flags & IO_REQ_CLEAN_SLOW_FLAGS)) { >>> + if (req->flags & (REQ_F_SQE_GROUP | >>> + REQ_F_SQE_GROUP_LEADER)) { >>> + struct io_kiocb *leader; >>> + >>> + /* Leader is freed via the last member */ >>> + if (req_is_group_leader(req)) { >>> + node = req->comp_list.next; >>> + continue; >>> + } >>> + >>> + /* >>> + * Only the last member keeps GROUP flag, >>> + * free leader and this member together >>> + */ >>> + leader = get_group_leader(req); >>> + leader->flags &= ~REQ_F_SQE_GROUP_LEADER; >>> + req->flags &= ~REQ_F_SQE_GROUP; >>> + wq_stack_add_head(&leader->comp_list, >>> + &req->comp_list); >> >> That's quite hacky, but at least we can replace it with >> task work if it gets in the way later on. > > io_free_batch_list() is already called in task context, and it isn't > necessary to schedule one extra tw, which hurts perf more or less. > > Another way is to store these leaders into one temp list, and > call io_free_batch_list() for this temp list one more time. What I'm saying, it's fine to leave it as is for now. In the future if it becomes a problem for ome reason or another, we can do it the task_work like way. ... >>> @@ -2101,6 +2251,62 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req, >>> return def->prep(req, sqe); >>> } >>> +static struct io_kiocb *io_group_sqe(struct io_submit_link *group, >>> + struct io_kiocb *req) >>> +{ >>> + /* >>> + * Group chain is similar with link chain: starts with 1st sqe with >>> + * REQ_F_SQE_GROUP, and ends with the 1st sqe without REQ_F_SQE_GROUP >>> + */ >>> + if (group->head) { >>> + struct io_kiocb *lead = group->head; >>> + >>> + /* members can't be in link chain, can't be drained */ >>> + if (req->flags & (IO_REQ_LINK_FLAGS | REQ_F_IO_DRAIN)) >>> + req_fail_link_node(lead, -EINVAL); >> >> That should fail the entire link (if any) as well. > > Good catch, here we should fail link head by following the logic > in io_submit_fail_init(). > >> >> I have even more doubts we even want to mix links and groups. Apart > > Wrt. ublk, group provides zero copy, and the ublk io(group) is generic > IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd, > send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK > for generic io_uring IO. > >> from nuances as such, which would be quite hard to track, the semantics >> of IOSQE_CQE_SKIP_SUCCESS is unclear. > > IO group just follows every normal request. It tries to mimic but groups don't and essentially can't do it the same way, at least in some aspects. E.g. IOSQE_CQE_SKIP_SUCCESS usually means that all following will be silenced. What if a member is CQE_SKIP, should it stop the leader from posting a CQE? And whatever the answer is, it'll be different from the link's behaviour. Regardless, let's forbid IOSQE_CQE_SKIP_SUCCESS and linked timeouts for groups, that can be discussed afterwards. > 1) fail in linked chain > - follows IO_LINK's behavior since io_fail_links() covers io group > > 2) otherwise > - just respect IOSQE_CQE_SKIP_SUCCESS > >> And also it doen't work with IORING_OP_LINK_TIMEOUT. > > REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I > will document it in V6. It would still be troublesome. When a linked timeout fires it searches for the request it's attached to and cancels it, however, group leaders that queued up their members are discoverable. But let's say you can find them in some way, then the only sensbile thing to do is cancel members, which should be doable by checking req->grp_leader, but might be easier to leave it to follow up patches. >>> + >>> + lead->grp_refs += 1; >>> + group->last->grp_link = req; >>> + group->last = req; >>> + >>> + if (req->flags & REQ_F_SQE_GROUP) >>> + return NULL; >>> + >>> + req->grp_link = NULL; >>> + req->flags |= REQ_F_SQE_GROUP; >>> + group->head = NULL; >>> + if (lead->flags & REQ_F_FAIL) { >>> + io_queue_sqe_fallback(lead); >> >> Let's say the group was in the middle of a link, it'll >> complete that group and continue with assembling / executing >> the link when it should've failed it and honoured the >> request order. > > OK, here we can simply remove the above two lines, and link submit > state can handle this failure in link chain. If you just delete then nobody would check for REQ_F_FAIL and fail the request. Assuming you'd also set the fail flag to the link head when appropriate, how about deleting these two line and do like below? (can be further prettified) bool io_group_assembling() { return state->group.head || (req->flags & REQ_F_SQE_GROUP); } bool io_link_assembling() { return state->link.head || (req->flags & IO_REQ_LINK_FLAGS); } static inline int io_submit_sqe() { ... if (unlikely(io_link_assembling(state, req) || io_group_assembling(state, req) || req->flags & REQ_F_FAIL)) { if (io_group_assembling(state, req)) { req = io_group_sqe(&state->group, req); if (!req) return 0; } if (io_link_assembling(state, req)) { req = io_link_sqe(&state->link, req); if (!req) return 0; } if (req->flags & REQ_F_FAIL) { io_queue_sqe_fallback(req); return 0; } } io_queue_sqe(req); return 0; }
On Fri, Sep 06, 2024 at 06:15:32PM +0100, Pavel Begunkov wrote: > On 8/29/24 05:29, Ming Lei wrote: > ... > > > > + if (WARN_ON_ONCE(lead->grp_refs <= 0)) > > > > + return false; > > > > + > > > > + req->flags &= ~REQ_F_SQE_GROUP; > > > > > > I'm getting completely lost when and why it clears and sets > > > back REQ_F_SQE_GROUP and REQ_F_SQE_GROUP_LEADER. Is there any > > > rule? > > > > My fault, it should have been documented somewhere. > > > > REQ_F_SQE_GROUP is cleared when the request is completed, but it is > > reused as flag for marking the last request in this group, so we can > > free the group leader when observing the 'last' member request. > > Maybe it'd be cleaner to use a second flag? I will add one new flag with same value, since the two's lifetime is non-overlapping. > > > The only other difference about the two flags is that both are cleared > > when the group leader becomes the last one in the group, then > > this leader degenerates as normal request, which way can simplify > > group leader freeing. > > > > > > > > > + /* > > > > + * Set linked leader as failed if any member is failed, so > > > > + * the remained link chain can be terminated > > > > + */ > > > > + if (unlikely((req->flags & REQ_F_FAIL) && > > > > + ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link))) > > > > + req_set_fail(lead); > > > > > > if (req->flags & REQ_F_FAIL) > > > req_set_fail(lead); > > > > > > REQ_F_FAIL is not specific to links, if a request fails we need > > > to mark it as such. > > > > It is for handling group failure. > > > > The following condition > > > > ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link)) > > > > means that this group is in one link-chain. > > > > If any member in this group is failed, we need to fail this group(lead), > > then the remained requests in this chain can be failed. > > > > Otherwise, it isn't necessary to fail group leader in case of any member > > io failure. > > What bad would happen if you do it like this? > > if (req->flags & REQ_F_FAIL) > req_set_fail(lead); > > I'm asking because if you rely on some particular combination > of F_FAIL and F_LINK somewhere, it's likely wrong, but otherwise > we F_FAIL a larger set of requests, which should never be an > issue. From dependency relation viewpoint it is not necessary to fail all, but it makes us easier to start with this more determinate behavior, will change to this way in V6. > > > > > + return !--lead->grp_refs; > > > > +} > > > > + > > > > +static inline bool leader_is_the_last(struct io_kiocb *lead) > > > > +{ > > > > + return lead->grp_refs == 1 && (lead->flags & REQ_F_SQE_GROUP); > > > > +} > > > > + > > > > +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))) > > > > + return; > > > > + > > > > + /* member CQE needs to be posted first */ > > > > + if (!(req->flags & REQ_F_CQE_SKIP)) > > > > + io_req_commit_cqe(req->ctx, req); > > > > + > > > > + if (__io_complete_group_member(req, lead)) { > > > > + /* > > > > + * SQE_GROUP flag is kept for the last member, so the leader > > > > + * can be retrieved & freed from this last member > > > > + */ > > > > + req->flags |= REQ_F_SQE_GROUP; > > > > 'req' is the last completed request, so mark it as the last one > > by reusing REQ_F_SQE_GROUP, so we can free group leader in > > io_free_batch_list() when observing the last flag. > > > > But it should have been documented. > > > > > > + if (!(lead->flags & REQ_F_CQE_SKIP)) > > > > + io_req_commit_cqe(lead->ctx, lead); > > > > + } else if (leader_is_the_last(lead)) { > > > > + /* leader will degenerate to plain req if it is the last */ > > > > + lead->flags &= ~(REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER); > > > > > > What's this chunk is about? > > > > The leader becomes the only request not completed in group, so it is > > degenerated as normal one by clearing the two flags. This way simplifies > > logic for completing group leader. > > > ... > > > > @@ -1388,11 +1501,33 @@ static void io_free_batch_list(struct io_ring_ctx *ctx, > > > > comp_list); > > > > if (unlikely(req->flags & IO_REQ_CLEAN_SLOW_FLAGS)) { > > > > + if (req->flags & (REQ_F_SQE_GROUP | > > > > + REQ_F_SQE_GROUP_LEADER)) { > > > > + struct io_kiocb *leader; > > > > + > > > > + /* Leader is freed via the last member */ > > > > + if (req_is_group_leader(req)) { > > > > + node = req->comp_list.next; > > > > + continue; > > > > + } > > > > + > > > > + /* > > > > + * Only the last member keeps GROUP flag, > > > > + * free leader and this member together > > > > + */ > > > > + leader = get_group_leader(req); > > > > + leader->flags &= ~REQ_F_SQE_GROUP_LEADER; > > > > + req->flags &= ~REQ_F_SQE_GROUP; > > > > + wq_stack_add_head(&leader->comp_list, > > > > + &req->comp_list); > > > > > > That's quite hacky, but at least we can replace it with > > > task work if it gets in the way later on. > > > > io_free_batch_list() is already called in task context, and it isn't > > necessary to schedule one extra tw, which hurts perf more or less. > > > > Another way is to store these leaders into one temp list, and > > call io_free_batch_list() for this temp list one more time. > > What I'm saying, it's fine to leave it as is for now. In the > future if it becomes a problem for ome reason or another, we can > do it the task_work like way. OK, got it, I will leave it as is, and document the potential risk with future changes. > > ... > > > > @@ -2101,6 +2251,62 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req, > > > > return def->prep(req, sqe); > > > > } > > > > +static struct io_kiocb *io_group_sqe(struct io_submit_link *group, > > > > + struct io_kiocb *req) > > > > +{ > > > > + /* > > > > + * Group chain is similar with link chain: starts with 1st sqe with > > > > + * REQ_F_SQE_GROUP, and ends with the 1st sqe without REQ_F_SQE_GROUP > > > > + */ > > > > + if (group->head) { > > > > + struct io_kiocb *lead = group->head; > > > > + > > > > + /* members can't be in link chain, can't be drained */ > > > > + if (req->flags & (IO_REQ_LINK_FLAGS | REQ_F_IO_DRAIN)) > > > > + req_fail_link_node(lead, -EINVAL); > > > > > > That should fail the entire link (if any) as well. > > > > Good catch, here we should fail link head by following the logic > > in io_submit_fail_init(). > > > > > > > > I have even more doubts we even want to mix links and groups. Apart > > > > Wrt. ublk, group provides zero copy, and the ublk io(group) is generic > > IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd, > > send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK > > for generic io_uring IO. > > > > > from nuances as such, which would be quite hard to track, the semantics > > > of IOSQE_CQE_SKIP_SUCCESS is unclear. > > > > IO group just follows every normal request. > > It tries to mimic but groups don't and essentially can't do it the > same way, at least in some aspects. E.g. IOSQE_CQE_SKIP_SUCCESS > usually means that all following will be silenced. What if a > member is CQE_SKIP, should it stop the leader from posting a CQE? > And whatever the answer is, it'll be different from the link's > behaviour. Here it looks easier than link's: - only leader's IOSQE_CQE_SKIP_SUCCESS follows linked request's rule - all members just respects the flag for its own, and not related with leader's > > Regardless, let's forbid IOSQE_CQE_SKIP_SUCCESS and linked timeouts > for groups, that can be discussed afterwards. It should easy to forbid IOSQE_CQE_SKIP_SUCCESS which is per-sqe, will do it in V6. I am not sure if it is easy to disallow IORING_OP_LINK_TIMEOUT, which covers all linked sqes, and group leader could be just one of them. Can you share any idea about the implementation to forbid LINK_TIMEOUT for sqe group? > > > 1) fail in linked chain > > - follows IO_LINK's behavior since io_fail_links() covers io group > > > > 2) otherwise > > - just respect IOSQE_CQE_SKIP_SUCCESS > > > > > And also it doen't work with IORING_OP_LINK_TIMEOUT. > > > > REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I > > will document it in V6. > > It would still be troublesome. When a linked timeout fires it searches > for the request it's attached to and cancels it, however, group leaders > that queued up their members are discoverable. But let's say you can find > them in some way, then the only sensbile thing to do is cancel members, > which should be doable by checking req->grp_leader, but might be easier > to leave it to follow up patches. We have changed sqe group to start queuing members after leader is completed. link timeout will cancel leader with all its members via leader->grp_link, this behavior should respect IORING_OP_LINK_TIMEOUT completely. Please see io_fail_links() and io_cancel_group_members(). > > > > > > + > > > > + lead->grp_refs += 1; > > > > + group->last->grp_link = req; > > > > + group->last = req; > > > > + > > > > + if (req->flags & REQ_F_SQE_GROUP) > > > > + return NULL; > > > > + > > > > + req->grp_link = NULL; > > > > + req->flags |= REQ_F_SQE_GROUP; > > > > + group->head = NULL; > > > > + if (lead->flags & REQ_F_FAIL) { > > > > + io_queue_sqe_fallback(lead); > > > > > > Let's say the group was in the middle of a link, it'll > > > complete that group and continue with assembling / executing > > > the link when it should've failed it and honoured the > > > request order. > > > > OK, here we can simply remove the above two lines, and link submit > > state can handle this failure in link chain. > > If you just delete then nobody would check for REQ_F_FAIL and > fail the request. io_link_assembling() & io_link_sqe() checks for REQ_F_FAIL and call io_queue_sqe_fallback() either if it is in link chain or not. > Assuming you'd also set the fail flag to the > link head when appropriate, how about deleting these two line > and do like below? (can be further prettified) > > > bool io_group_assembling() > { > return state->group.head || (req->flags & REQ_F_SQE_GROUP); > } > bool io_link_assembling() > { > return state->link.head || (req->flags & IO_REQ_LINK_FLAGS); > } > > static inline int io_submit_sqe() > { > ... > if (unlikely(io_link_assembling(state, req) || > io_group_assembling(state, req) || > req->flags & REQ_F_FAIL)) { > if (io_group_assembling(state, req)) { > req = io_group_sqe(&state->group, req); > if (!req) > return 0; > } > if (io_link_assembling(state, req)) { > req = io_link_sqe(&state->link, req); > if (!req) > return 0; > } > if (req->flags & REQ_F_FAIL) { > io_queue_sqe_fallback(req); > return 0; As I mentioned above, io_link_assembling() & io_link_sqe() covers the failure handling. thanks, Ming
On 9/7/24 10:36, Ming Lei wrote: ... >>> Wrt. ublk, group provides zero copy, and the ublk io(group) is generic >>> IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd, >>> send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK >>> for generic io_uring IO. >>> >>>> from nuances as such, which would be quite hard to track, the semantics >>>> of IOSQE_CQE_SKIP_SUCCESS is unclear. >>> >>> IO group just follows every normal request. >> >> It tries to mimic but groups don't and essentially can't do it the >> same way, at least in some aspects. E.g. IOSQE_CQE_SKIP_SUCCESS >> usually means that all following will be silenced. What if a >> member is CQE_SKIP, should it stop the leader from posting a CQE? >> And whatever the answer is, it'll be different from the link's >> behaviour. > > Here it looks easier than link's: > > - only leader's IOSQE_CQE_SKIP_SUCCESS follows linked request's rule > - all members just respects the flag for its own, and not related with > leader's > >> >> Regardless, let's forbid IOSQE_CQE_SKIP_SUCCESS and linked timeouts >> for groups, that can be discussed afterwards. > > It should easy to forbid IOSQE_CQE_SKIP_SUCCESS which is per-sqe, will do > it in V6. > > I am not sure if it is easy to disallow IORING_OP_LINK_TIMEOUT, which > covers all linked sqes, and group leader could be just one of them. > Can you share any idea about the implementation to forbid LINK_TIMEOUT > for sqe group? diff --git a/io_uring/timeout.c b/io_uring/timeout.c index 671d6093bf36..83b5fd64b4e9 100644 --- a/io_uring/timeout.c +++ b/io_uring/timeout.c @@ -542,6 +542,9 @@ static int __io_timeout_prep(struct io_kiocb *req, data->mode = io_translate_timeout_mode(flags); hrtimer_init(&data->timer, io_timeout_get_clock(data), data->mode); + if (is_timeout_link && req->ctx->submit_state.group.head) + return -EINVAL; + if (is_timeout_link) { struct io_submit_link *link = &req->ctx->submit_state.link; This should do, they already look into the ctx's link list. Just move it into the "if (is_timeout_link)" block. >>> 1) fail in linked chain >>> - follows IO_LINK's behavior since io_fail_links() covers io group >>> >>> 2) otherwise >>> - just respect IOSQE_CQE_SKIP_SUCCESS >>> >>>> And also it doen't work with IORING_OP_LINK_TIMEOUT. >>> >>> REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I >>> will document it in V6. >> >> It would still be troublesome. When a linked timeout fires it searches >> for the request it's attached to and cancels it, however, group leaders >> that queued up their members are discoverable. But let's say you can find >> them in some way, then the only sensbile thing to do is cancel members, >> which should be doable by checking req->grp_leader, but might be easier >> to leave it to follow up patches. > > We have changed sqe group to start queuing members after leader is > completed. link timeout will cancel leader with all its members via > leader->grp_link, this behavior should respect IORING_OP_LINK_TIMEOUT > completely. > > Please see io_fail_links() and io_cancel_group_members(). > >> >> >>>>> + >>>>> + lead->grp_refs += 1; >>>>> + group->last->grp_link = req; >>>>> + group->last = req; >>>>> + >>>>> + if (req->flags & REQ_F_SQE_GROUP) >>>>> + return NULL; >>>>> + >>>>> + req->grp_link = NULL; >>>>> + req->flags |= REQ_F_SQE_GROUP; >>>>> + group->head = NULL; >>>>> + if (lead->flags & REQ_F_FAIL) { >>>>> + io_queue_sqe_fallback(lead); >>>> >>>> Let's say the group was in the middle of a link, it'll >>>> complete that group and continue with assembling / executing >>>> the link when it should've failed it and honoured the >>>> request order. >>> >>> OK, here we can simply remove the above two lines, and link submit >>> state can handle this failure in link chain. >> >> If you just delete then nobody would check for REQ_F_FAIL and >> fail the request. > > io_link_assembling() & io_link_sqe() checks for REQ_F_FAIL and call > io_queue_sqe_fallback() either if it is in link chain or > not. The case we're talking about is failing a group, which is also in the middle of a link. LINK_HEAD -> {GROUP_LEAD, GROUP_MEMBER} Let's say GROUP_MEMBER fails and sets REQ_F_FAIL to the lead, then in v5 does: if (lead->flags & REQ_F_FAIL) { io_queue_sqe_fallback(lead); return NULL; } In which case it posts cqes for GROUP_LEAD and GROUP_MEMBER, and then try to execute LINK_HEAD (without failing it), which is wrong. So first we need: if (state.linked_link.head) req_fail_link_node(state.linked_link.head); And then we can't just remove io_queue_sqe_fallback(), because when a group is not linked there would be no io_link_sqe() to fail it. You can do: io_group_sqe() { if ((lead->flags & REQ_F_FAIL) && !ctx->state.link.head) { io_queue_sqe_fallback(lead); return NULL; } ... } but it's much cleaner to move REQ_F_FAIL out of group assembling. We'd also want to move same REQ_F_FAIL / io_queue_sqe_fallback() out of io_link_sqe(), but I didn't mentioned because it's not strictly required for your set AFAIR. >> Assuming you'd also set the fail flag to the >> link head when appropriate, how about deleting these two line >> and do like below? (can be further prettified) >> >> >> bool io_group_assembling() >> { >> return state->group.head || (req->flags & REQ_F_SQE_GROUP); >> } >> bool io_link_assembling() >> { >> return state->link.head || (req->flags & IO_REQ_LINK_FLAGS); >> } >> >> static inline int io_submit_sqe() >> { >> ... >> if (unlikely(io_link_assembling(state, req) || >> io_group_assembling(state, req) || >> req->flags & REQ_F_FAIL)) { >> if (io_group_assembling(state, req)) { >> req = io_group_sqe(&state->group, req); >> if (!req) >> return 0; >> } >> if (io_link_assembling(state, req)) { >> req = io_link_sqe(&state->link, req); >> if (!req) >> return 0; >> } >> if (req->flags & REQ_F_FAIL) { >> io_queue_sqe_fallback(req); >> return 0; > > As I mentioned above, io_link_assembling() & io_link_sqe() covers > the failure handling.
On Tue, Sep 10, 2024 at 02:12:53PM +0100, Pavel Begunkov wrote: > On 9/7/24 10:36, Ming Lei wrote: > ... > > > > Wrt. ublk, group provides zero copy, and the ublk io(group) is generic > > > > IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd, > > > > send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK > > > > for generic io_uring IO. > > > > > > > > > from nuances as such, which would be quite hard to track, the semantics > > > > > of IOSQE_CQE_SKIP_SUCCESS is unclear. > > > > > > > > IO group just follows every normal request. > > > > > > It tries to mimic but groups don't and essentially can't do it the > > > same way, at least in some aspects. E.g. IOSQE_CQE_SKIP_SUCCESS > > > usually means that all following will be silenced. What if a > > > member is CQE_SKIP, should it stop the leader from posting a CQE? > > > And whatever the answer is, it'll be different from the link's > > > behaviour. > > > > Here it looks easier than link's: > > > > - only leader's IOSQE_CQE_SKIP_SUCCESS follows linked request's rule > > - all members just respects the flag for its own, and not related with > > leader's > > > > > > > > Regardless, let's forbid IOSQE_CQE_SKIP_SUCCESS and linked timeouts > > > for groups, that can be discussed afterwards. > > > > It should easy to forbid IOSQE_CQE_SKIP_SUCCESS which is per-sqe, will do > > it in V6. > > > > I am not sure if it is easy to disallow IORING_OP_LINK_TIMEOUT, which > > covers all linked sqes, and group leader could be just one of them. > > Can you share any idea about the implementation to forbid LINK_TIMEOUT > > for sqe group? > > diff --git a/io_uring/timeout.c b/io_uring/timeout.c > index 671d6093bf36..83b5fd64b4e9 100644 > --- a/io_uring/timeout.c > +++ b/io_uring/timeout.c > @@ -542,6 +542,9 @@ static int __io_timeout_prep(struct io_kiocb *req, > data->mode = io_translate_timeout_mode(flags); > hrtimer_init(&data->timer, io_timeout_get_clock(data), data->mode); > + if (is_timeout_link && req->ctx->submit_state.group.head) > + return -EINVAL; > + > if (is_timeout_link) { > struct io_submit_link *link = &req->ctx->submit_state.link; > > This should do, they already look into the ctx's link list. Just move > it into the "if (is_timeout_link)" block. OK. > > > > > > 1) fail in linked chain > > > > - follows IO_LINK's behavior since io_fail_links() covers io group > > > > > > > > 2) otherwise > > > > - just respect IOSQE_CQE_SKIP_SUCCESS > > > > > > > > > And also it doen't work with IORING_OP_LINK_TIMEOUT. > > > > > > > > REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I > > > > will document it in V6. > > > > > > It would still be troublesome. When a linked timeout fires it searches > > > for the request it's attached to and cancels it, however, group leaders > > > that queued up their members are discoverable. But let's say you can find > > > them in some way, then the only sensbile thing to do is cancel members, > > > which should be doable by checking req->grp_leader, but might be easier > > > to leave it to follow up patches. > > > > We have changed sqe group to start queuing members after leader is > > completed. link timeout will cancel leader with all its members via > > leader->grp_link, this behavior should respect IORING_OP_LINK_TIMEOUT > > completely. > > > > Please see io_fail_links() and io_cancel_group_members(). > > > > > > > > > > > > > > + > > > > > > + lead->grp_refs += 1; > > > > > > + group->last->grp_link = req; > > > > > > + group->last = req; > > > > > > + > > > > > > + if (req->flags & REQ_F_SQE_GROUP) > > > > > > + return NULL; > > > > > > + > > > > > > + req->grp_link = NULL; > > > > > > + req->flags |= REQ_F_SQE_GROUP; > > > > > > + group->head = NULL; > > > > > > + if (lead->flags & REQ_F_FAIL) { > > > > > > + io_queue_sqe_fallback(lead); > > > > > > > > > > Let's say the group was in the middle of a link, it'll > > > > > complete that group and continue with assembling / executing > > > > > the link when it should've failed it and honoured the > > > > > request order. > > > > > > > > OK, here we can simply remove the above two lines, and link submit > > > > state can handle this failure in link chain. > > > > > > If you just delete then nobody would check for REQ_F_FAIL and > > > fail the request. > > > > io_link_assembling() & io_link_sqe() checks for REQ_F_FAIL and call > > io_queue_sqe_fallback() either if it is in link chain or > > not. > > The case we're talking about is failing a group, which is > also in the middle of a link. > > LINK_HEAD -> {GROUP_LEAD, GROUP_MEMBER} > > Let's say GROUP_MEMBER fails and sets REQ_F_FAIL to the lead, > then in v5 does: > > if (lead->flags & REQ_F_FAIL) { > io_queue_sqe_fallback(lead); > return NULL; > } > > In which case it posts cqes for GROUP_LEAD and GROUP_MEMBER, > and then try to execute LINK_HEAD (without failing it), which > is wrong. So first we need: > > if (state.linked_link.head) > req_fail_link_node(state.linked_link.head); For group leader, link advancing is always done via io_queue_next(), in which io_disarm_next() is called for failing the whole remained link if the current request is marked as FAIL. > > And then we can't just remove io_queue_sqe_fallback(), because > when a group is not linked there would be no io_link_sqe() > to fail it. You can do: If one request in group is marked as FAIL, io_link_assembling() will return true, and io_link_sqe() will fail it. Thanks, Ming
On 9/10/24 16:04, Ming Lei wrote: > On Tue, Sep 10, 2024 at 02:12:53PM +0100, Pavel Begunkov wrote: >> On 9/7/24 10:36, Ming Lei wrote: >> ... >>>>> Wrt. ublk, group provides zero copy, and the ublk io(group) is generic >>>>> IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd, >>>>> send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK >>>>> for generic io_uring IO. >>>>> >>>>>> from nuances as such, which would be quite hard to track, the semantics >>>>>> of IOSQE_CQE_SKIP_SUCCESS is unclear. >>>>> >>>>> IO group just follows every normal request. >>>> >>>> It tries to mimic but groups don't and essentially can't do it the >>>> same way, at least in some aspects. E.g. IOSQE_CQE_SKIP_SUCCESS >>>> usually means that all following will be silenced. What if a >>>> member is CQE_SKIP, should it stop the leader from posting a CQE? >>>> And whatever the answer is, it'll be different from the link's >>>> behaviour. >>> >>> Here it looks easier than link's: >>> >>> - only leader's IOSQE_CQE_SKIP_SUCCESS follows linked request's rule >>> - all members just respects the flag for its own, and not related with >>> leader's >>> >>>> >>>> Regardless, let's forbid IOSQE_CQE_SKIP_SUCCESS and linked timeouts >>>> for groups, that can be discussed afterwards. >>> >>> It should easy to forbid IOSQE_CQE_SKIP_SUCCESS which is per-sqe, will do >>> it in V6. >>> >>> I am not sure if it is easy to disallow IORING_OP_LINK_TIMEOUT, which >>> covers all linked sqes, and group leader could be just one of them. >>> Can you share any idea about the implementation to forbid LINK_TIMEOUT >>> for sqe group? >> >> diff --git a/io_uring/timeout.c b/io_uring/timeout.c >> index 671d6093bf36..83b5fd64b4e9 100644 >> --- a/io_uring/timeout.c >> +++ b/io_uring/timeout.c >> @@ -542,6 +542,9 @@ static int __io_timeout_prep(struct io_kiocb *req, >> data->mode = io_translate_timeout_mode(flags); >> hrtimer_init(&data->timer, io_timeout_get_clock(data), data->mode); >> + if (is_timeout_link && req->ctx->submit_state.group.head) >> + return -EINVAL; >> + >> if (is_timeout_link) { >> struct io_submit_link *link = &req->ctx->submit_state.link; >> >> This should do, they already look into the ctx's link list. Just move >> it into the "if (is_timeout_link)" block. > > OK. > >> >> >>>>> 1) fail in linked chain >>>>> - follows IO_LINK's behavior since io_fail_links() covers io group >>>>> >>>>> 2) otherwise >>>>> - just respect IOSQE_CQE_SKIP_SUCCESS >>>>> >>>>>> And also it doen't work with IORING_OP_LINK_TIMEOUT. >>>>> >>>>> REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I >>>>> will document it in V6. >>>> >>>> It would still be troublesome. When a linked timeout fires it searches >>>> for the request it's attached to and cancels it, however, group leaders >>>> that queued up their members are discoverable. But let's say you can find >>>> them in some way, then the only sensbile thing to do is cancel members, >>>> which should be doable by checking req->grp_leader, but might be easier >>>> to leave it to follow up patches. >>> >>> We have changed sqe group to start queuing members after leader is >>> completed. link timeout will cancel leader with all its members via >>> leader->grp_link, this behavior should respect IORING_OP_LINK_TIMEOUT >>> completely. >>> >>> Please see io_fail_links() and io_cancel_group_members(). >>> >>>> >>>> >>>>>>> + >>>>>>> + lead->grp_refs += 1; >>>>>>> + group->last->grp_link = req; >>>>>>> + group->last = req; >>>>>>> + >>>>>>> + if (req->flags & REQ_F_SQE_GROUP) >>>>>>> + return NULL; >>>>>>> + >>>>>>> + req->grp_link = NULL; >>>>>>> + req->flags |= REQ_F_SQE_GROUP; >>>>>>> + group->head = NULL; >>>>>>> + if (lead->flags & REQ_F_FAIL) { >>>>>>> + io_queue_sqe_fallback(lead); >>>>>> >>>>>> Let's say the group was in the middle of a link, it'll >>>>>> complete that group and continue with assembling / executing >>>>>> the link when it should've failed it and honoured the >>>>>> request order. >>>>> >>>>> OK, here we can simply remove the above two lines, and link submit >>>>> state can handle this failure in link chain. >>>> >>>> If you just delete then nobody would check for REQ_F_FAIL and >>>> fail the request. >>> >>> io_link_assembling() & io_link_sqe() checks for REQ_F_FAIL and call >>> io_queue_sqe_fallback() either if it is in link chain or >>> not. >> >> The case we're talking about is failing a group, which is >> also in the middle of a link. >> >> LINK_HEAD -> {GROUP_LEAD, GROUP_MEMBER} >> >> Let's say GROUP_MEMBER fails and sets REQ_F_FAIL to the lead, >> then in v5 does: >> >> if (lead->flags & REQ_F_FAIL) { >> io_queue_sqe_fallback(lead); >> return NULL; >> } >> >> In which case it posts cqes for GROUP_LEAD and GROUP_MEMBER, >> and then try to execute LINK_HEAD (without failing it), which >> is wrong. So first we need: >> >> if (state.linked_link.head) >> req_fail_link_node(state.linked_link.head); > > For group leader, link advancing is always done via io_queue_next(), in > which io_disarm_next() is called for failing the whole remained link > if the current request is marked as FAIL. > >> >> And then we can't just remove io_queue_sqe_fallback(), because >> when a group is not linked there would be no io_link_sqe() >> to fail it. You can do: > > If one request in group is marked as FAIL, io_link_assembling() > will return true, and io_link_sqe() will fail it. Hmm, you're right, even though it's not a great way of doing it, i.e. pushing a req into io_link_sqe() even when linking has never been requested, but that's fine. I can drop a quick patch on top if it bothers me.
On Tue, Sep 10, 2024 at 09:31:45PM +0100, Pavel Begunkov wrote: > On 9/10/24 16:04, Ming Lei wrote: > > On Tue, Sep 10, 2024 at 02:12:53PM +0100, Pavel Begunkov wrote: > > > On 9/7/24 10:36, Ming Lei wrote: > > > ... > > > > > > Wrt. ublk, group provides zero copy, and the ublk io(group) is generic > > > > > > IO, sometime IO_LINK is really needed & helpful, such as in ublk-nbd, > > > > > > send(tcp) requests need to be linked & zc. And we shouldn't limit IO_LINK > > > > > > for generic io_uring IO. > > > > > > > > > > > > > from nuances as such, which would be quite hard to track, the semantics > > > > > > > of IOSQE_CQE_SKIP_SUCCESS is unclear. > > > > > > > > > > > > IO group just follows every normal request. > > > > > > > > > > It tries to mimic but groups don't and essentially can't do it the > > > > > same way, at least in some aspects. E.g. IOSQE_CQE_SKIP_SUCCESS > > > > > usually means that all following will be silenced. What if a > > > > > member is CQE_SKIP, should it stop the leader from posting a CQE? > > > > > And whatever the answer is, it'll be different from the link's > > > > > behaviour. > > > > > > > > Here it looks easier than link's: > > > > > > > > - only leader's IOSQE_CQE_SKIP_SUCCESS follows linked request's rule > > > > - all members just respects the flag for its own, and not related with > > > > leader's > > > > > > > > > > > > > > Regardless, let's forbid IOSQE_CQE_SKIP_SUCCESS and linked timeouts > > > > > for groups, that can be discussed afterwards. > > > > > > > > It should easy to forbid IOSQE_CQE_SKIP_SUCCESS which is per-sqe, will do > > > > it in V6. > > > > > > > > I am not sure if it is easy to disallow IORING_OP_LINK_TIMEOUT, which > > > > covers all linked sqes, and group leader could be just one of them. > > > > Can you share any idea about the implementation to forbid LINK_TIMEOUT > > > > for sqe group? > > > > > > diff --git a/io_uring/timeout.c b/io_uring/timeout.c > > > index 671d6093bf36..83b5fd64b4e9 100644 > > > --- a/io_uring/timeout.c > > > +++ b/io_uring/timeout.c > > > @@ -542,6 +542,9 @@ static int __io_timeout_prep(struct io_kiocb *req, > > > data->mode = io_translate_timeout_mode(flags); > > > hrtimer_init(&data->timer, io_timeout_get_clock(data), data->mode); > > > + if (is_timeout_link && req->ctx->submit_state.group.head) > > > + return -EINVAL; > > > + > > > if (is_timeout_link) { > > > struct io_submit_link *link = &req->ctx->submit_state.link; > > > > > > This should do, they already look into the ctx's link list. Just move > > > it into the "if (is_timeout_link)" block. > > > > OK. > > > > > > > > > > > > > > 1) fail in linked chain > > > > > > - follows IO_LINK's behavior since io_fail_links() covers io group > > > > > > > > > > > > 2) otherwise > > > > > > - just respect IOSQE_CQE_SKIP_SUCCESS > > > > > > > > > > > > > And also it doen't work with IORING_OP_LINK_TIMEOUT. > > > > > > > > > > > > REQ_F_LINK_TIMEOUT can work on whole group(or group leader) only, and I > > > > > > will document it in V6. > > > > > > > > > > It would still be troublesome. When a linked timeout fires it searches > > > > > for the request it's attached to and cancels it, however, group leaders > > > > > that queued up their members are discoverable. But let's say you can find > > > > > them in some way, then the only sensbile thing to do is cancel members, > > > > > which should be doable by checking req->grp_leader, but might be easier > > > > > to leave it to follow up patches. > > > > > > > > We have changed sqe group to start queuing members after leader is > > > > completed. link timeout will cancel leader with all its members via > > > > leader->grp_link, this behavior should respect IORING_OP_LINK_TIMEOUT > > > > completely. > > > > > > > > Please see io_fail_links() and io_cancel_group_members(). > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > + lead->grp_refs += 1; > > > > > > > > + group->last->grp_link = req; > > > > > > > > + group->last = req; > > > > > > > > + > > > > > > > > + if (req->flags & REQ_F_SQE_GROUP) > > > > > > > > + return NULL; > > > > > > > > + > > > > > > > > + req->grp_link = NULL; > > > > > > > > + req->flags |= REQ_F_SQE_GROUP; > > > > > > > > + group->head = NULL; > > > > > > > > + if (lead->flags & REQ_F_FAIL) { > > > > > > > > + io_queue_sqe_fallback(lead); > > > > > > > > > > > > > > Let's say the group was in the middle of a link, it'll > > > > > > > complete that group and continue with assembling / executing > > > > > > > the link when it should've failed it and honoured the > > > > > > > request order. > > > > > > > > > > > > OK, here we can simply remove the above two lines, and link submit > > > > > > state can handle this failure in link chain. > > > > > > > > > > If you just delete then nobody would check for REQ_F_FAIL and > > > > > fail the request. > > > > > > > > io_link_assembling() & io_link_sqe() checks for REQ_F_FAIL and call > > > > io_queue_sqe_fallback() either if it is in link chain or > > > > not. > > > > > > The case we're talking about is failing a group, which is > > > also in the middle of a link. > > > > > > LINK_HEAD -> {GROUP_LEAD, GROUP_MEMBER} > > > > > > Let's say GROUP_MEMBER fails and sets REQ_F_FAIL to the lead, > > > then in v5 does: > > > > > > if (lead->flags & REQ_F_FAIL) { > > > io_queue_sqe_fallback(lead); > > > return NULL; > > > } > > > > > > In which case it posts cqes for GROUP_LEAD and GROUP_MEMBER, > > > and then try to execute LINK_HEAD (without failing it), which > > > is wrong. So first we need: > > > > > > if (state.linked_link.head) > > > req_fail_link_node(state.linked_link.head); > > > > For group leader, link advancing is always done via io_queue_next(), in > > which io_disarm_next() is called for failing the whole remained link > > if the current request is marked as FAIL. > > > > > > > > And then we can't just remove io_queue_sqe_fallback(), because > > > when a group is not linked there would be no io_link_sqe() > > > to fail it. You can do: > > > > If one request in group is marked as FAIL, io_link_assembling() > > will return true, and io_link_sqe() will fail it. > > Hmm, you're right, even though it's not a great way of doing it, > i.e. pushing a req into io_link_sqe() even when linking has never > been requested, but that's fine. I can drop a quick patch on > top if it bothers me. Yeah, it isn't very readable, but following the original logic. Anyway, I will comment that non-linked request is covered by the code block. Thanks, Ming
diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h index 3315005df117..c5250e585289 100644 --- a/include/linux/io_uring_types.h +++ b/include/linux/io_uring_types.h @@ -202,6 +202,8 @@ struct io_submit_state { /* batch completion logic */ struct io_wq_work_list compl_reqs; struct io_submit_link link; + /* points to current group */ + struct io_submit_link group; bool plug_started; bool need_plug; @@ -435,6 +437,7 @@ enum { REQ_F_FORCE_ASYNC_BIT = IOSQE_ASYNC_BIT, REQ_F_BUFFER_SELECT_BIT = IOSQE_BUFFER_SELECT_BIT, REQ_F_CQE_SKIP_BIT = IOSQE_CQE_SKIP_SUCCESS_BIT, + REQ_F_SQE_GROUP_BIT = IOSQE_SQE_GROUP_BIT, /* first byte is taken by user flags, shift it to not overlap */ REQ_F_FAIL_BIT = 8, @@ -465,6 +468,7 @@ enum { REQ_F_BL_EMPTY_BIT, REQ_F_BL_NO_RECYCLE_BIT, REQ_F_BUFFERS_COMMIT_BIT, + REQ_F_SQE_GROUP_LEADER_BIT, /* not a real bit, just to check we're not overflowing the space */ __REQ_F_LAST_BIT, @@ -488,6 +492,8 @@ enum { REQ_F_BUFFER_SELECT = IO_REQ_FLAG(REQ_F_BUFFER_SELECT_BIT), /* IOSQE_CQE_SKIP_SUCCESS */ REQ_F_CQE_SKIP = IO_REQ_FLAG(REQ_F_CQE_SKIP_BIT), + /* IOSQE_SQE_GROUP */ + REQ_F_SQE_GROUP = IO_REQ_FLAG(REQ_F_SQE_GROUP_BIT), /* fail rest of links */ REQ_F_FAIL = IO_REQ_FLAG(REQ_F_FAIL_BIT), @@ -543,6 +549,8 @@ enum { REQ_F_BL_NO_RECYCLE = IO_REQ_FLAG(REQ_F_BL_NO_RECYCLE_BIT), /* buffer ring head needs incrementing on put */ REQ_F_BUFFERS_COMMIT = IO_REQ_FLAG(REQ_F_BUFFERS_COMMIT_BIT), + /* sqe group lead */ + REQ_F_SQE_GROUP_LEADER = IO_REQ_FLAG(REQ_F_SQE_GROUP_LEADER_BIT), }; typedef void (*io_req_tw_func_t)(struct io_kiocb *req, struct io_tw_state *ts); @@ -648,6 +656,8 @@ struct io_kiocb { void *async_data; /* linked requests, IFF REQ_F_HARDLINK or REQ_F_LINK are set */ atomic_t poll_refs; + /* reference for group leader requests */ + int grp_refs; struct io_kiocb *link; /* custom credentials, valid IFF REQ_F_CREDS is set */ const struct cred *creds; @@ -657,6 +667,14 @@ struct io_kiocb { u64 extra1; u64 extra2; } big_cqe; + + union { + /* links all group members for leader */ + struct io_kiocb *grp_link; + + /* points to group leader for member */ + struct io_kiocb *grp_leader; + }; }; struct io_overflow_cqe { diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index 2aaf7ee256ac..e6d321b3add7 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -124,6 +124,7 @@ enum io_uring_sqe_flags_bit { IOSQE_ASYNC_BIT, IOSQE_BUFFER_SELECT_BIT, IOSQE_CQE_SKIP_SUCCESS_BIT, + IOSQE_SQE_GROUP_BIT, }; /* @@ -143,6 +144,8 @@ enum io_uring_sqe_flags_bit { #define IOSQE_BUFFER_SELECT (1U << IOSQE_BUFFER_SELECT_BIT) /* don't post CQE if request succeeded */ #define IOSQE_CQE_SKIP_SUCCESS (1U << IOSQE_CQE_SKIP_SUCCESS_BIT) +/* defines sqe group */ +#define IOSQE_SQE_GROUP (1U << IOSQE_SQE_GROUP_BIT) /* * io_uring_setup() flags @@ -542,6 +545,7 @@ struct io_uring_params { #define IORING_FEAT_LINKED_FILE (1U << 12) #define IORING_FEAT_REG_REG_RING (1U << 13) #define IORING_FEAT_RECVSEND_BUNDLE (1U << 14) +#define IORING_FEAT_SQE_GROUP (1U << 15) /* * io_uring_register(2) opcodes and arguments diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index f112e9fa90d8..45a292445b18 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -111,13 +111,15 @@ IOSQE_IO_HARDLINK | IOSQE_ASYNC) #define SQE_VALID_FLAGS (SQE_COMMON_FLAGS | IOSQE_BUFFER_SELECT | \ - IOSQE_IO_DRAIN | IOSQE_CQE_SKIP_SUCCESS) + IOSQE_IO_DRAIN | IOSQE_CQE_SKIP_SUCCESS | \ + IOSQE_SQE_GROUP) #define IO_REQ_CLEAN_FLAGS (REQ_F_BUFFER_SELECTED | REQ_F_NEED_CLEANUP | \ REQ_F_POLLED | REQ_F_INFLIGHT | REQ_F_CREDS | \ REQ_F_ASYNC_DATA) #define IO_REQ_CLEAN_SLOW_FLAGS (REQ_F_REFCOUNT | REQ_F_LINK | REQ_F_HARDLINK |\ + REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER | \ IO_REQ_CLEAN_FLAGS) #define IO_TCTX_REFS_CACHE_NR (1U << 10) @@ -875,6 +877,116 @@ static __always_inline void io_req_commit_cqe(struct io_ring_ctx *ctx, } } +/* Can only be called after this request is issued */ +static inline struct io_kiocb *get_group_leader(struct io_kiocb *req) +{ + if (req->flags & REQ_F_SQE_GROUP) { + if (req_is_group_leader(req)) + return req; + return req->grp_leader; + } + return NULL; +} + +void io_cancel_group_members(struct io_kiocb *req, bool ignore_cqes) +{ + struct io_kiocb *member = req->grp_link; + + while (member) { + struct io_kiocb *next = member->grp_link; + + if (ignore_cqes) + member->flags |= REQ_F_CQE_SKIP; + if (!(member->flags & REQ_F_FAIL)) { + req_set_fail(member); + io_req_set_res(member, -ECANCELED, 0); + } + member = next; + } +} + +static void io_queue_group_members(struct io_kiocb *req) +{ + struct io_kiocb *member = req->grp_link; + + if (!member) + return; + + req->grp_link = NULL; + while (member) { + struct io_kiocb *next = member->grp_link; + + member->grp_leader = req; + if (unlikely(member->flags & REQ_F_FAIL)) { + io_req_task_queue_fail(member, member->cqe.res); + } else if (unlikely(req->flags & REQ_F_FAIL)) { + io_req_task_queue_fail(member, -ECANCELED); + } else { + io_req_task_queue(member); + } + member = next; + } +} + +static inline bool __io_complete_group_member(struct io_kiocb *req, + struct io_kiocb *lead) +{ + if (WARN_ON_ONCE(lead->grp_refs <= 0)) + return false; + + req->flags &= ~REQ_F_SQE_GROUP; + /* + * Set linked leader as failed if any member is failed, so + * the remained link chain can be terminated + */ + if (unlikely((req->flags & REQ_F_FAIL) && + ((lead->flags & IO_REQ_LINK_FLAGS) && lead->link))) + req_set_fail(lead); + return !--lead->grp_refs; +} + +static inline bool leader_is_the_last(struct io_kiocb *lead) +{ + return lead->grp_refs == 1 && (lead->flags & REQ_F_SQE_GROUP); +} + +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))) + return; + + /* member CQE needs to be posted first */ + if (!(req->flags & REQ_F_CQE_SKIP)) + io_req_commit_cqe(req->ctx, req); + + if (__io_complete_group_member(req, lead)) { + /* + * SQE_GROUP flag is kept for the last member, so the leader + * can be retrieved & freed from this last member + */ + req->flags |= REQ_F_SQE_GROUP; + if (!(lead->flags & REQ_F_CQE_SKIP)) + io_req_commit_cqe(lead->ctx, lead); + } else if (leader_is_the_last(lead)) { + /* leader will degenerate to plain req if it is the last */ + lead->flags &= ~(REQ_F_SQE_GROUP | REQ_F_SQE_GROUP_LEADER); + } +} + +static void io_complete_group_leader(struct io_kiocb *req) +{ + WARN_ON_ONCE(req->grp_refs <= 0); + req->flags &= ~REQ_F_SQE_GROUP; + req->grp_refs -= 1; + WARN_ON_ONCE(req->grp_refs == 0); + + /* TODO: queue members with leader in parallel */ + if (req->grp_link) + io_queue_group_members(req); +} + static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags) { struct io_ring_ctx *ctx = req->ctx; @@ -890,7 +1002,8 @@ static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags) * Handle special CQ sync cases via task_work. DEFER_TASKRUN requires * the submitter task context, IOPOLL protects with uring_lock. */ - if (ctx->task_complete || (ctx->flags & IORING_SETUP_IOPOLL)) { + if (ctx->task_complete || (ctx->flags & IORING_SETUP_IOPOLL) || + req_is_group_leader(req)) { req->io_task_work.func = io_req_task_complete; io_req_task_work_add(req); return; @@ -1388,11 +1501,33 @@ static void io_free_batch_list(struct io_ring_ctx *ctx, comp_list); if (unlikely(req->flags & IO_REQ_CLEAN_SLOW_FLAGS)) { + if (req->flags & (REQ_F_SQE_GROUP | + REQ_F_SQE_GROUP_LEADER)) { + struct io_kiocb *leader; + + /* Leader is freed via the last member */ + if (req_is_group_leader(req)) { + node = req->comp_list.next; + continue; + } + + /* + * Only the last member keeps GROUP flag, + * free leader and this member together + */ + leader = get_group_leader(req); + leader->flags &= ~REQ_F_SQE_GROUP_LEADER; + req->flags &= ~REQ_F_SQE_GROUP; + wq_stack_add_head(&leader->comp_list, + &req->comp_list); + } + if (req->flags & REQ_F_REFCOUNT) { node = req->comp_list.next; if (!req_ref_put_and_test(req)) continue; } + if ((req->flags & REQ_F_POLLED) && req->apoll) { struct async_poll *apoll = req->apoll; @@ -1427,8 +1562,19 @@ void __io_submit_flush_completions(struct io_ring_ctx *ctx) struct io_kiocb *req = container_of(node, struct io_kiocb, comp_list); - if (!(req->flags & REQ_F_CQE_SKIP)) - io_req_commit_cqe(ctx, req); + if (unlikely(req->flags & (REQ_F_CQE_SKIP | REQ_F_SQE_GROUP))) { + if (req->flags & REQ_F_SQE_GROUP) { + if (req_is_group_leader(req)) + io_complete_group_leader(req); + else + io_complete_group_member(req); + continue; + } + + if (req->flags & REQ_F_CQE_SKIP) + continue; + } + io_req_commit_cqe(ctx, req); } __io_cq_unlock_post(ctx); @@ -1638,8 +1784,12 @@ static u32 io_get_sequence(struct io_kiocb *req) struct io_kiocb *cur; /* need original cached_sq_head, but it was increased for each req */ - io_for_each_link(cur, req) - seq--; + io_for_each_link(cur, req) { + if (req_is_group_leader(cur)) + seq -= cur->grp_refs; + else + seq--; + } return seq; } @@ -2101,6 +2251,62 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req, return def->prep(req, sqe); } +static struct io_kiocb *io_group_sqe(struct io_submit_link *group, + struct io_kiocb *req) +{ + /* + * Group chain is similar with link chain: starts with 1st sqe with + * REQ_F_SQE_GROUP, and ends with the 1st sqe without REQ_F_SQE_GROUP + */ + if (group->head) { + struct io_kiocb *lead = group->head; + + /* members can't be in link chain, can't be drained */ + if (req->flags & (IO_REQ_LINK_FLAGS | REQ_F_IO_DRAIN)) + req_fail_link_node(lead, -EINVAL); + + lead->grp_refs += 1; + group->last->grp_link = req; + group->last = req; + + if (req->flags & REQ_F_SQE_GROUP) + return NULL; + + req->grp_link = NULL; + req->flags |= REQ_F_SQE_GROUP; + group->head = NULL; + if (lead->flags & REQ_F_FAIL) { + io_queue_sqe_fallback(lead); + return NULL; + } + return lead; + } else if (req->flags & REQ_F_SQE_GROUP) { + group->head = req; + group->last = req; + req->grp_refs = 1; + req->flags |= REQ_F_SQE_GROUP_LEADER; + return NULL; + } else { + return req; + } +} + +static __cold struct io_kiocb *io_submit_fail_group( + struct io_submit_link *link, struct io_kiocb *req) +{ + struct io_kiocb *lead = link->head; + + /* + * Instead of failing eagerly, continue assembling the group link + * if applicable and mark the leader with REQ_F_FAIL. The group + * flushing code should find the flag and handle the rest + */ + if (lead && (lead->flags & IO_REQ_LINK_FLAGS) && !(lead->flags & REQ_F_FAIL)) + req_fail_link_node(lead, -ECANCELED); + + return io_group_sqe(link, req); +} + static __cold int io_submit_fail_link(struct io_submit_link *link, struct io_kiocb *req, int ret) { @@ -2139,11 +2345,18 @@ static __cold int io_submit_fail_init(const struct io_uring_sqe *sqe, { struct io_ring_ctx *ctx = req->ctx; struct io_submit_link *link = &ctx->submit_state.link; + struct io_submit_link *group = &ctx->submit_state.group; trace_io_uring_req_failed(sqe, req, ret); req_fail_link_node(req, ret); + if (group->head || (req->flags & REQ_F_SQE_GROUP)) { + req = io_submit_fail_group(group, req); + if (!req) + return 0; + } + /* cover both linked and non-linked request */ return io_submit_fail_link(link, req, ret); } @@ -2187,11 +2400,28 @@ static struct io_kiocb *io_link_sqe(struct io_submit_link *link, return req; } +static inline bool io_group_assembling(const struct io_submit_state *state, + const struct io_kiocb *req) +{ + if (state->group.head || req->flags & REQ_F_SQE_GROUP) + return true; + return false; +} + +static inline bool io_link_assembling(const struct io_submit_state *state, + const struct io_kiocb *req) +{ + if (state->link.head || (req->flags & (IO_REQ_LINK_FLAGS | + REQ_F_FORCE_ASYNC | REQ_F_FAIL))) + return true; + return false; +} + static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req, const struct io_uring_sqe *sqe) __must_hold(&ctx->uring_lock) { - struct io_submit_link *link = &ctx->submit_state.link; + struct io_submit_state *state = &ctx->submit_state; int ret; ret = io_init_req(ctx, req, sqe); @@ -2200,11 +2430,18 @@ static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req, trace_io_uring_submit_req(req); - if (unlikely(link->head || (req->flags & (IO_REQ_LINK_FLAGS | - REQ_F_FORCE_ASYNC | REQ_F_FAIL)))) { - req = io_link_sqe(link, req); - if (!req) - return 0; + if (unlikely(io_link_assembling(state, req) || + io_group_assembling(state, req))) { + if (io_group_assembling(state, req)) { + req = io_group_sqe(&state->group, req); + if (!req) + return 0; + } + if (io_link_assembling(state, req)) { + req = io_link_sqe(&state->link, req); + if (!req) + return 0; + } } io_queue_sqe(req); return 0; @@ -2217,8 +2454,22 @@ static void io_submit_state_end(struct io_ring_ctx *ctx) { struct io_submit_state *state = &ctx->submit_state; - if (unlikely(state->link.head)) - io_queue_sqe_fallback(state->link.head); + if (unlikely(state->group.head || state->link.head)) { + /* the last member must set REQ_F_SQE_GROUP */ + if (state->group.head) { + struct io_kiocb *lead = state->group.head; + + state->group.last->grp_link = NULL; + if (lead->flags & IO_REQ_LINK_FLAGS) + io_link_sqe(&state->link, lead); + else + io_queue_sqe_fallback(lead); + } + + if (unlikely(state->link.head)) + io_queue_sqe_fallback(state->link.head); + } + /* flush only after queuing links as they can generate completions */ io_submit_flush_completions(ctx); if (state->plug_started) @@ -2236,6 +2487,7 @@ static void io_submit_state_start(struct io_submit_state *state, state->submit_nr = max_ios; /* set only head, no need to init link_last in advance */ state->link.head = NULL; + state->group.head = NULL; } static void io_commit_sqring(struct io_ring_ctx *ctx) @@ -3564,7 +3816,7 @@ static __cold int io_uring_create(unsigned entries, struct io_uring_params *p, IORING_FEAT_EXT_ARG | IORING_FEAT_NATIVE_WORKERS | IORING_FEAT_RSRC_TAGS | IORING_FEAT_CQE_SKIP | IORING_FEAT_LINKED_FILE | IORING_FEAT_REG_REG_RING | - IORING_FEAT_RECVSEND_BUNDLE; + IORING_FEAT_RECVSEND_BUNDLE | IORING_FEAT_SQE_GROUP; if (copy_to_user(params, p, sizeof(*p))) { ret = -EFAULT; diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h index c2acf6180845..0125daa8dfe7 100644 --- a/io_uring/io_uring.h +++ b/io_uring/io_uring.h @@ -68,6 +68,7 @@ bool io_post_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags void io_add_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags); bool io_req_post_cqe(struct io_kiocb *req, s32 res, u32 cflags); void __io_commit_cqring_flush(struct io_ring_ctx *ctx); +void io_cancel_group_members(struct io_kiocb *req, bool ignore_cqes); struct file *io_file_get_normal(struct io_kiocb *req, int fd); struct file *io_file_get_fixed(struct io_kiocb *req, int fd, @@ -339,6 +340,11 @@ static inline void io_tw_lock(struct io_ring_ctx *ctx, struct io_tw_state *ts) lockdep_assert_held(&ctx->uring_lock); } +static inline bool req_is_group_leader(struct io_kiocb *req) +{ + return req->flags & REQ_F_SQE_GROUP_LEADER; +} + /* * Don't complete immediately but use deferred completion infrastructure. * Protected by ->uring_lock and can only be used either with diff --git a/io_uring/timeout.c b/io_uring/timeout.c index 9973876d91b0..671d6093bf36 100644 --- a/io_uring/timeout.c +++ b/io_uring/timeout.c @@ -168,6 +168,8 @@ static void io_fail_links(struct io_kiocb *req) link->flags |= REQ_F_CQE_SKIP; else link->flags &= ~REQ_F_CQE_SKIP; + if (req_is_group_leader(link)) + io_cancel_group_members(link, ignore_cqes); trace_io_uring_fail_link(req, link); link = link->link; }
SQE group is defined as one chain of SQEs starting with the first SQE that has IOSQE_SQE_GROUP set, and ending with the first subsequent SQE that doesn't have it set, and it is similar with chain of linked SQEs. Not like linked SQEs, each sqe is issued after the previous one is completed. All SQEs in one group can be submitted in parallel. To simplify the implementation from beginning, all members are queued after the leader is completed, however, this way may be changed and leader and members may be issued concurrently in future. The 1st SQE is group leader, and the other SQEs are group member. The whole group share single IOSQE_IO_LINK and IOSQE_IO_DRAIN from group leader, and the two flags can't be set for group members. When the group is in one link chain, this group isn't submitted until the previous SQE or group is completed. And the following SQE or group can't be started if this group isn't completed. Failure from any group member will fail the group leader, then the link chain can be terminated. When IOSQE_IO_DRAIN is set for group leader, all requests in this group and previous requests submitted are drained. Given IOSQE_IO_DRAIN can be set for group leader only, we respect IO_DRAIN by always completing group leader as the last one in the group. Meantime it is natural to post leader's CQE as the last one from application viewpoint. Working together with IOSQE_IO_LINK, SQE group provides flexible way to support N:M dependency, such as: - group A is chained with group B together - group A has N SQEs - group B has M SQEs then M SQEs in group B depend on N SQEs in group A. N:M dependency can support some interesting use cases in efficient way: 1) read from multiple files, then write the read data into single file 2) read from single file, and write the read data into multiple files 3) write same data into multiple files, and read data from multiple files and compare if correct data is written Also IOSQE_SQE_GROUP takes the last bit in sqe->flags, but we still can extend sqe->flags with one uring context flag, such as use __pad3 for non-uring_cmd OPs and part of uring_cmd_flags for uring_cmd OP. Suggested-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- include/linux/io_uring_types.h | 18 +++ include/uapi/linux/io_uring.h | 4 + io_uring/io_uring.c | 282 +++++++++++++++++++++++++++++++-- io_uring/io_uring.h | 6 + io_uring/timeout.c | 2 + 5 files changed, 297 insertions(+), 15 deletions(-)