On Wed, Oct 09, 2024 at 01:37:00PM -0600, Uday Shankar wrote:
> Currently, ublk_drv maintains a per-hctx queue of requests awaiting
> dispatch to the ublk server, and pokes the ubq_daemon to come pick them
> up via the task_work mechanism when needed. But task_work already
> supports internal (lockless) queueing. Reuse this queueing mechanism
> (i.e. have one task_work queue item per request awaiting dispatch)
> instead of maintaining our own queue in ublk_drv.
>
> Signed-off-by: Uday Shankar <ushankar@xxxxxxxxxxxxxxx>
> ---
> drivers/block/ublk_drv.c | 34 ++++++----------------------------
> 1 file changed, 6 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 60f6d86ea1e6..2ea108347ec4 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -80,6 +80,7 @@ struct ublk_rq_data {
>
> struct ublk_uring_cmd_pdu {
> struct ublk_queue *ubq;
> + struct request *req;
> u16 tag;
> };
I'd suggest to add the following build check in init function since there is
only 32bytes in uring_cmd pdu:
BUILD_BUG_ON(sizeof(ublk_uring_cmd_pdu) < sizeof_field(io_uring_cmd, pud))
>
> @@ -141,8 +142,6 @@ struct ublk_queue {
> struct task_struct *ubq_daemon;
> char *io_cmd_buf;
>
> - struct llist_head io_cmds;
> -
> unsigned long io_addr; /* mapped vm address */
> unsigned int max_io_sz;
> bool force_abort;
> @@ -1132,9 +1131,10 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq,
> blk_mq_end_request(rq, BLK_STS_IOERR);
> }
>
> -static inline void __ublk_rq_task_work(struct request *req,
> +static inline void __ublk_rq_task_work(struct io_uring_cmd *cmd,
> unsigned issue_flags)
`inline` can be removed and __ublk_rq_task_work() can be named as
ublk_rq_task_work() now.
> {
> + struct request *req = ublk_get_uring_cmd_pdu(cmd)->req;
> struct ublk_queue *ubq = req->mq_hctx->driver_data;
> int tag = req->tag;
> struct ublk_io *io = &ubq->ios[tag];
> @@ -1211,34 +1211,12 @@ static inline void __ublk_rq_task_work(struct request *req,
> ubq_complete_io_cmd(io, UBLK_IO_RES_OK, issue_flags);
> }
>
> -static inline void ublk_forward_io_cmds(struct ublk_queue *ubq,
> - unsigned issue_flags)
> -{
> - struct llist_node *io_cmds = llist_del_all(&ubq->io_cmds);
> - struct ublk_rq_data *data, *tmp;
> -
> - io_cmds = llist_reverse_order(io_cmds);
> - llist_for_each_entry_safe(data, tmp, io_cmds, node)
> - __ublk_rq_task_work(blk_mq_rq_from_pdu(data), issue_flags);
> -}
> -
> -static void ublk_rq_task_work_cb(struct io_uring_cmd *cmd, unsigned issue_flags)
> -{
> - struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
> - struct ublk_queue *ubq = pdu->ubq;
> -
> - ublk_forward_io_cmds(ubq, issue_flags);
> -}
> -
> static void ublk_queue_cmd(struct ublk_queue *ubq, struct request *rq)
> {
> - struct ublk_rq_data *data = blk_mq_rq_to_pdu(rq);
> -
> - if (llist_add(&data->node, &ubq->io_cmds)) {
> - struct ublk_io *io = &ubq->ios[rq->tag];
> + struct ublk_io *io = &ubq->ios[rq->tag];
>
> - io_uring_cmd_complete_in_task(io->cmd, ublk_rq_task_work_cb);
> - }
> + ublk_get_uring_cmd_pdu(io->cmd)->req = rq;
> + io_uring_cmd_complete_in_task(io->cmd, __ublk_rq_task_work);
> }
I'd suggest to comment that io_uring_cmd_complete_in_task() needs to
maintain io command order.
Otherwise this patch looks fine.
Thanks,
Ming
