[PATCHv3 0/4] queue_rqs error handling

[PATCHv3 0/4] queue_rqs error handling

[Keith Busch]

The only real change since v2 is a prep patch that relocates the rq list
macros to blk-mq.h since that's where 'struct request' is defined.

Patch 3 removes the 'next' parameter since it is trivially obtainable
via 'rq->rq_next' anyway.

Otherwise, the series is the same as v2 and tested with lots of random
error injection in the prep path. The same errors would have lost
requests in the current driver, but is successful with this series.

Keith Busch (4):
  block: move rq_list macros to blk-mq.h
  block: introduce rq_list_for_each_safe macro
  block: introduce rq_list_move
  nvme-pci: fix queue_rqs list splitting

 drivers/nvme/host/pci.c | 28 +++++++++++------------
 fs/io_uring.c           |  2 +-
 include/linux/blk-mq.h  | 50 +++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h  | 29 ------------------------
 4 files changed, 65 insertions(+), 44 deletions(-)

-- 
2.25.4

[PATCHv3 1/4] block: move rq_list macros to blk-mq.h

[Keith Busch]

Move the request list macros to the header file that defines that struct
they operate on.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 fs/io_uring.c          |  2 +-
 include/linux/blk-mq.h | 29 +++++++++++++++++++++++++++++
 include/linux/blkdev.h | 29 -----------------------------
 3 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 90002bb3fdf4..33f72b3b302c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -57,7 +57,7 @@
 #include <linux/mman.h>
 #include <linux/percpu.h>
 #include <linux/slab.h>
-#include <linux/blkdev.h>
+#include <linux/blk-mq.h>
 #include <linux/bvec.h>
 #include <linux/net.h>
 #include <net/sock.h>
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 550996cf419c..bf64b94cd64e 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -216,6 +216,35 @@ static inline unsigned short req_get_ioprio(struct request *req)
 #define rq_dma_dir(rq) \
 	(op_is_write(req_op(rq)) ? DMA_TO_DEVICE : DMA_FROM_DEVICE)
 
+#define rq_list_add(listptr, rq)	do {		\
+	(rq)->rq_next = *(listptr);			\
+	*(listptr) = rq;				\
+} while (0)
+
+#define rq_list_pop(listptr)				\
+({							\
+	struct request *__req = NULL;			\
+	if ((listptr) && *(listptr))	{		\
+		__req = *(listptr);			\
+		*(listptr) = __req->rq_next;		\
+	}						\
+	__req;						\
+})
+
+#define rq_list_peek(listptr)				\
+({							\
+	struct request *__req = NULL;			\
+	if ((listptr) && *(listptr))			\
+		__req = *(listptr);			\
+	__req;						\
+})
+
+#define rq_list_for_each(listptr, pos)			\
+	for (pos = rq_list_peek((listptr)); pos; pos = rq_list_next(pos))
+
+#define rq_list_next(rq)	(rq)->rq_next
+#define rq_list_empty(list)	((list) == (struct request *) NULL)
+
 enum blk_eh_timer_return {
 	BLK_EH_DONE,		/* drivers has completed the command */
 	BLK_EH_RESET_TIMER,	/* reset timer and try again */
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 22746b2d6825..9c95df26fc26 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1339,33 +1339,4 @@ struct io_comp_batch {
 
 #define DEFINE_IO_COMP_BATCH(name)	struct io_comp_batch name = { }
 
-#define rq_list_add(listptr, rq)	do {		\
-	(rq)->rq_next = *(listptr);			\
-	*(listptr) = rq;				\
-} while (0)
-
-#define rq_list_pop(listptr)				\
-({							\
-	struct request *__req = NULL;			\
-	if ((listptr) && *(listptr))	{		\
-		__req = *(listptr);			\
-		*(listptr) = __req->rq_next;		\
-	}						\
-	__req;						\
-})
-
-#define rq_list_peek(listptr)				\
-({							\
-	struct request *__req = NULL;			\
-	if ((listptr) && *(listptr))			\
-		__req = *(listptr);			\
-	__req;						\
-})
-
-#define rq_list_for_each(listptr, pos)			\
-	for (pos = rq_list_peek((listptr)); pos; pos = rq_list_next(pos))
-
-#define rq_list_next(rq)	(rq)->rq_next
-#define rq_list_empty(list)	((list) == (struct request *) NULL)
-
 #endif /* _LINUX_BLKDEV_H */
-- 
2.25.4

Re: [PATCHv3 1/4] block: move rq_list macros to blk-mq.h

[Christoph Hellwig]

On Wed, Jan 05, 2022 at 09:05:15AM -0800, Keith Busch wrote:
> Move the request list macros to the header file that defines that struct
> they operate on.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv3 2/4] block: introduce rq_list_for_each_safe macro

[Keith Busch]

While iterating a list, a particular request may need to be removed for
special handling. Provide an iterator that can safely handle that.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 include/linux/blk-mq.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index bf64b94cd64e..1467f0fa2142 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -242,6 +242,10 @@ static inline unsigned short req_get_ioprio(struct request *req)
 #define rq_list_for_each(listptr, pos)			\
 	for (pos = rq_list_peek((listptr)); pos; pos = rq_list_next(pos))
 
+#define rq_list_for_each_safe(listptr, pos, nxt)			\
+	for (pos = rq_list_peek((listptr)), nxt = rq_list_next(pos);	\
+		pos; pos = nxt, nxt = pos ? rq_list_next(pos) : NULL)
+
 #define rq_list_next(rq)	(rq)->rq_next
 #define rq_list_empty(list)	((list) == (struct request *) NULL)
 
-- 
2.25.4

[PATCHv3 3/4] block: introduce rq_list_move

[Keith Busch]

When iterating a list, a particular request may need to be moved for
special handling. Provide a helper function to achieve that so drivers
don't need to reimplement rqlist manipulation.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 include/linux/blk-mq.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 1467f0fa2142..f40a05ecca4a 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -249,6 +249,23 @@ static inline unsigned short req_get_ioprio(struct request *req)
 #define rq_list_next(rq)	(rq)->rq_next
 #define rq_list_empty(list)	((list) == (struct request *) NULL)
 
+/**
+ * rq_list_move() - move a struct request from one list to another
+ * @src: The source list @rq is currently in
+ * @dst: The destination list that @rq will be appended to
+ * @rq: The request to move
+ * @prev: The request preceding @rq in @src (NULL if @rq is the head)
+ */
+static void inline rq_list_move(struct request **src, struct request **dst,
+				struct request *rq, struct request *prev)
+{
+	if (prev)
+		prev->rq_next = rq->rq_next;
+	else
+		*src = rq->rq_next;
+	rq_list_add(dst, rq);
+}
+
 enum blk_eh_timer_return {
 	BLK_EH_DONE,		/* drivers has completed the command */
 	BLK_EH_RESET_TIMER,	/* reset timer and try again */
-- 
2.25.4

Re: [PATCHv3 3/4] block: introduce rq_list_move

[Christoph Hellwig]

On Wed, Jan 05, 2022 at 09:05:17AM -0800, Keith Busch wrote:
> When iterating a list, a particular request may need to be moved for
> special handling. Provide a helper function to achieve that so drivers
> don't need to reimplement rqlist manipulation.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv3 4/4] nvme-pci: fix queue_rqs list splitting

[Keith Busch]

If command prep fails, current handling will orphan subsequent requests
in the list. Consider a simple example:

  rqlist = [ 1 -> 2 ]

When prep for request '1' fails, it will be appended to the
'requeue_list', leaving request '2' disconnected from the original
rqlist and no longer tracked. Meanwhile, rqlist is still pointing to the
failed request '1' and will attempt to submit the unprepped command.

Fix this by updating the rqlist accordingly using the request list
helper functions.

Fixes: d62cbcf62f2f ("nvme: add support for mq_ops->queue_rqs()")
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/pci.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 50deb8b69c40..d8585df2c2fd 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -999,30 +999,30 @@ static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req)
 
 static void nvme_queue_rqs(struct request **rqlist)
 {
-	struct request *req = rq_list_peek(rqlist), *prev = NULL;
+	struct request *req, *next, *prev = NULL;
 	struct request *requeue_list = NULL;
 
-	do {
+	rq_list_for_each_safe(rqlist, req, next) {
 		struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
 
 		if (!nvme_prep_rq_batch(nvmeq, req)) {
 			/* detach 'req' and add to remainder list */
-			if (prev)
-				prev->rq_next = req->rq_next;
-			rq_list_add(&requeue_list, req);
-		} else {
-			prev = req;
+			rq_list_move(rqlist, &requeue_list, req, prev);
+
+			req = prev;
+			if (!req)
+				continue;
 		}
 
-		req = rq_list_next(req);
-		if (!req || (prev && req->mq_hctx != prev->mq_hctx)) {
+		if (!next || req->mq_hctx != next->mq_hctx) {
 			/* detach rest of list, and submit */
-			if (prev)
-				prev->rq_next = NULL;
+			req->rq_next = NULL;
 			nvme_submit_cmds(nvmeq, rqlist);
-			*rqlist = req;
-		}
-	} while (req);
+			*rqlist = next;
+			prev = NULL;
+		} else
+			prev = req;
+	}
 
 	*rqlist = requeue_list;
 }
-- 
2.25.4

Re: [PATCHv3 4/4] nvme-pci: fix queue_rqs list splitting

[Christoph Hellwig]

On Wed, Jan 05, 2022 at 09:05:18AM -0800, Keith Busch wrote:
> If command prep fails, current handling will orphan subsequent requests
> in the list. Consider a simple example:
> 
>   rqlist = [ 1 -> 2 ]
> 
> When prep for request '1' fails, it will be appended to the
> 'requeue_list', leaving request '2' disconnected from the original
> rqlist and no longer tracked. Meanwhile, rqlist is still pointing to the
> failed request '1' and will attempt to submit the unprepped command.
> 
> Fix this by updating the rqlist accordingly using the request list
> helper functions.
> 
> Fixes: d62cbcf62f2f ("nvme: add support for mq_ops->queue_rqs()")
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv3 0/4] queue_rqs error handling

[Jens Axboe]

On Wed, 5 Jan 2022 09:05:14 -0800, Keith Busch wrote:
> The only real change since v2 is a prep patch that relocates the rq list
> macros to blk-mq.h since that's where 'struct request' is defined.
> 
> Patch 3 removes the 'next' parameter since it is trivially obtainable
> via 'rq->rq_next' anyway.
> 
> Otherwise, the series is the same as v2 and tested with lots of random
> error injection in the prep path. The same errors would have lost
> requests in the current driver, but is successful with this series.
> 
> [...]

Applied, thanks!

[1/4] block: move rq_list macros to blk-mq.h
      commit: edce22e19bfa86efa2522d041d6367f2f099e8ed
[2/4] block: introduce rq_list_for_each_safe macro
      commit: 3764fd05e1f89530e2ee5cbff0b638f2b1141b90
[3/4] block: introduce rq_list_move
      commit: d2528be7a8b09af9796a270debd14101a72bb552
[4/4] nvme-pci: fix queue_rqs list splitting
      commit: 6bfec7992ec79b63fb07330ae97f3fb43120aa37

Best regards,
-- 
Jens Axboe