Re: Reordering of ublk IO requests

[Andreas Hindborg <andreas.hindborg@wdc.com>]

Andreas Hindborg <andreas.hindborg@wdc.com> writes:

[...]
>>> >
>>> > oops, I miss the single queue depth point per zone, so ublk won't break
>>> > zoned write at all, and I agree order of batch IOs is one problem, but
>>> > not hard to solve.
>>>
>>> The current implementation _does_ break zoned write because it reverses
>>> batched writes. But if it is an easy fix, that is cool :)
>>
>> Please look at Damien's comment:
>>
>>>> That lock is already there and using it, mq-deadline will never dispatch
>>>> more than one write per zone at any time. This is to avoid write
>>>> reordering. So multi queue or not, for any zone, there is no possibility
>>>> of having writes reordered.
>>
>> For zoned write, mq-deadline is used to limit at most one inflight write
>> for each zone.
>>
>> So can you explain a bit how the current implementation breaks zoned
>> write?
>
> Like Damien wrote in another email, mq-deadline will only impose
> ordering for requests submitted in batch. The flow we have is the
> following:
>
>  - Userspace sends requests to ublk gendisk
>  - Requests go through block layer and is _not_ reordered when using
>    mq-deadline. They may be split.
>  - Requests hit ublk_drv and ublk_drv will reverse order of _all_
>    batched up requests (including split requests).
>  - ublk_drv sends request to ublksrv in _reverse_ order.
>  - ublksrv sends requests _not_ batched up to target device.
>  - Requests that enter mq-deadline at the same time are reordered in LBA
>    order, that is all good.
>  - Requests that enter the kernel in different batches are not reordered
>    in LBA order and end up missing the write pointer. This is bad.
>
> So, ublk_drv is not functional for zoned storage as is. Either we have
> to fix up the ordering in userspace in ublksrv, and that _will_ have a
> performance impact. Or we fix the bug in ublk_drv that causes batched
> requests to be _reversed_.

Here is a suggestion for a fix. It needs work, but it illustrates the
idea.

From 48f54a2a83daf19dda3c928e6518ce4a3e443fcd Mon Sep 17 00:00:00 2001
From: Andreas Hindborg <andreas.hindborg@wdc.com>
Date: Fri, 18 Nov 2022 13:44:45 +0100
Subject: [PATCH] wip: Do not reorder requests in ublk

---
 drivers/block/ublk_drv.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 6a4a94b4cdf4..4fb5ccd01202 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -9,6 +9,7 @@
  *
  * (part of code stolen from loop.c)
  */
+#include <linux/llist.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/sched.h>
@@ -55,6 +56,7 @@
 #define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD)
 
 struct ublk_rq_data {
+	struct llist_node llnode;
 	struct callback_head work;
 };
 
@@ -121,6 +123,7 @@ struct ublk_queue {
 	unsigned int max_io_sz;
 	bool abort_work_pending;
 	unsigned short nr_io_ready;	/* how many ios setup */
+	struct llist_head pdu_queue;
 	struct ublk_device *dev;
 	struct ublk_io ios[0];
 };
@@ -724,8 +727,15 @@ static void ublk_rq_task_work_fn(struct callback_head *work)
 	struct ublk_rq_data *data = container_of(work,
 			struct ublk_rq_data, work);
 	struct request *req = blk_mq_rq_from_pdu(data);
+	struct ublk_queue *ubq = req->mq_hctx->driver_data;
 
-	__ublk_rq_task_work(req);
+	/* Some times this list is empty, but that is OK */
+	struct llist_node *head = llist_del_all(&ubq->pdu_queue);
+	head = llist_reverse_order(head);
+	llist_for_each_entry(data, head, llnode) {
+		req = blk_mq_rq_from_pdu(data);
+		__ublk_rq_task_work(req);
+	}
 }
 
 static blk_status_t ublk_queue_rq(struct blk_mq_hw_ctx *hctx,
@@ -753,6 +763,7 @@ static blk_status_t ublk_queue_rq(struct blk_mq_hw_ctx *hctx,
 		enum task_work_notify_mode notify_mode = bd->last ?
 			TWA_SIGNAL_NO_IPI : TWA_NONE;
 
+		llist_add(&data->llnode, &ubq->pdu_queue);
 		if (task_work_add(ubq->ubq_daemon, &data->work, notify_mode))
 			goto fail;
 	} else {
@@ -1170,6 +1181,9 @@ static int ublk_init_queue(struct ublk_device *ub, int q_id)
 
 	ubq->io_cmd_buf = ptr;
 	ubq->dev = ub;
+
+	init_llist_head(&ubq->pdu_queue);
+
 	return 0;
 }
 
-- 
2.38.1