nvme/dm failover unification

PATCH Applied Keith Busch 4 participants 13 messages Jan 9, 2018 – Jan 10, 2018

Subsystems (3)

Block layer Device mapper NVMe

lore

Share & tools

lore

Keith Busch

author

2018-01-09 19:04 UTC #0 lore raw

v2 cover nvme/dm failover unification

Native nvme multipath provided a separate NVMe status decoder, complicating maintenance as new statuses need to be accounted for. This was already diverging from the generic nvme status decoder, which has implications for other components that rely on accurate generic block errors.

This series unifies common code among nvme and device-mapper multipath so user experience regarding the failover fate of a command is the same.

v1 -> v2:

Fixed blk_status_t used for NVME_SC_LBA_RANGE.

Fixed line break formatting.

Changed name of new block API for path related errors and added kernel doc for it.

Added reviews and acks.

Keith Busch (5):
nvme: Add more command status translation nvme/multipath: Consult blk_status_t for failover
block: Provide blk_status_t decoding for path errors nvme/multipath: Use blk_path_error
dm mpath: Use blk_path_error

 drivers/md/dm-mpath.c         | 19 ++-----------------
 drivers/nvme/host/core.c      | 16 ++++++++++++----
 drivers/nvme/host/multipath.c | 44 ++-----------------------------------------
 drivers/nvme/host/nvme.h      |  5 +++--
 include/linux/blk_types.h     | 28 +++++++++++++++++++++++++++
 5 files changed, 47 insertions(+), 65 deletions(-)

Signature · 1 line

2.13.6

Keith Busch

author

2018-01-09 19:04 UTC #1 lore raw

PATCH v2 1/5 nvme: Add more command status translation

This adds more NVMe status code translations to blk_status_t values, and captures all the current status codes NVMe multipath uses.

Acked-by Mike Snitzer <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 drivers/nvme/host/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/nvme/host/core.c +7 -0

index 2a69d735efbc..499f3141e365 100644
@@ -157,13 +157,20 @@ static blk_status_t nvme_error_status(struct request *req)
 		return BLK_STS_OK;
 	case NVME_SC_CAP_EXCEEDED:
 		return BLK_STS_NOSPC;
+	case NVME_SC_LBA_RANGE:
+		return BLK_STS_TARGET;
+	case NVME_SC_BAD_ATTRIBUTES:
 	case NVME_SC_ONCS_NOT_SUPPORTED:
+	case NVME_SC_INVALID_OPCODE:
+	case NVME_SC_INVALID_FIELD:
+	case NVME_SC_INVALID_NS:
 		return BLK_STS_NOTSUPP;
 	case NVME_SC_WRITE_FAULT:
 	case NVME_SC_READ_ERROR:
 	case NVME_SC_UNWRITTEN_BLOCK:
 	case NVME_SC_ACCESS_DENIED:
 	case NVME_SC_READ_ONLY:
+	case NVME_SC_COMPARE_FAILED:
 		return BLK_STS_MEDIUM;
 	case NVME_SC_GUARD_CHECK:
 	case NVME_SC_APPTAG_CHECK:

Signature · 1 line

2.13.6

Johannes Thumshirn

Reviewed-by on v2 1/5

2018-01-10 07:58 UTC #2 lore raw

Re: [dm-devel] [PATCHv2 1/5] nvme: Add more command status translation

Looks good,

Reviewed-by Johannes Thumshirn <[email protected]>

Signature · 6 lines

Johannes Thumshirn                                          Storage
[email protected]                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Keith Busch

author

2018-01-09 19:04 UTC #3 lore raw

PATCH v2 2/5 nvme/multipath: Consult blk_status_t for failover

In reply to Keith Busch

This removes nvme multipath's specific status decoding to see if failover is needed, using the generic blk_status_t that was decoded earlier. This abstraction from the raw NVMe status means all status decoding exists in one place.

Acked-by Mike Snitzer <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 drivers/nvme/host/core.c      |  9 +++++----
 drivers/nvme/host/multipath.c | 44 ++++++++-----------------------------------
 drivers/nvme/host/nvme.h      |  5 +++--
 3 files changed, 16 insertions(+), 42 deletions(-)

drivers/nvme/host/core.c +5 -4

index 499f3141e365..82fb5bcfbf91 100644
@@ -197,8 +197,10 @@ static inline bool nvme_req_needs_retry(struct request *req)
 
 void nvme_complete_rq(struct request *req)
 {
-	if (unlikely(nvme_req(req)->status && nvme_req_needs_retry(req))) {
-		if (nvme_req_needs_failover(req)) {
+	blk_status_t status = nvme_error_status(req);
+
+	if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) {
+		if (nvme_req_needs_failover(req, status)) {
 			nvme_failover_req(req);
 			return;
 		}

@@ -209,8 +211,7 @@ void nvme_complete_rq(struct request *req)
 			return;
 		}
 	}
-
-	blk_mq_end_request(req, nvme_error_status(req));
+	blk_mq_end_request(req, status);
 }
 EXPORT_SYMBOL_GPL(nvme_complete_rq);

drivers/nvme/host/multipath.c +8 ~~-36~~

54 lines not on this page · raw message

drivers/nvme/host/nvme.h +3 -2

index ea1aa5283e8e..8d18cfa36093 100644
@@ -401,7 +401,7 @@ extern const struct block_device_operations nvme_ns_head_ops;
 
 #ifdef CONFIG_NVME_MULTIPATH
 void nvme_failover_req(struct request *req);
-bool nvme_req_needs_failover(struct request *req);
+bool nvme_req_needs_failover(struct request *req, blk_status_t error);
 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl);
 int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl,struct nvme_ns_head *head);
 void nvme_mpath_add_disk(struct nvme_ns_head *head);

@@ -421,7 +421,8 @@ struct nvme_ns *nvme_find_path(struct nvme_ns_head *head);
 static inline void nvme_failover_req(struct request *req)
 {
 }
-static inline bool nvme_req_needs_failover(struct request *req)
+static inline bool nvme_req_needs_failover(struct request *req,
+					   blk_status_t error)
 {
 	return false;
 }

Signature · 1 line

2.13.6

Johannes Thumshirn

Reviewed-by on v2 2/5

2018-01-10 08:02 UTC #4 lore raw

Re: [dm-devel] [PATCHv2 2/5] nvme/multipath: Consult blk_status_t for failover

Looks good,

Reviewed-by Johannes Thumshirn <[email protected]>

Signature · 6 lines

Johannes Thumshirn                                          Storage
[email protected]                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Keith Busch

author

2018-01-09 19:04 UTC #5 lore raw

PATCH v2 3/5 block: Provide blk_status_t decoding for path errors

In reply to Keith Busch

This patch provides a common decoder for block status path related errors that may be retried so various entities wishing to consult this do not have to duplicate this decision.

Acked-by Mike Snitzer <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 include/linux/blk_types.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

include/linux/blk_types.h +28 -0

index a1e628e032da..2d973ac54b09 100644
@@ -39,6 +39,34 @@ typedef u8 __bitwise blk_status_t;
 
 #define BLK_STS_AGAIN		((__force blk_status_t)12)
 
+/**
+ * blk_path_error - returns true if error may be path related
+ * @error: status the request was completed with
+ *
+ * Description:
+ *     This classifies block error status into non-retryable errors and ones
+ *     that may be successful if retried on a failover path.
+ *
+ * Return:
+ *     %false - retrying failover path will not help
+ *     %true  - may succeed if retried
+ */
+static inline bool blk_path_error(blk_status_t error)
+{
+	switch (error) {
+	case BLK_STS_NOTSUPP:
+	case BLK_STS_NOSPC:
+	case BLK_STS_TARGET:
+	case BLK_STS_NEXUS:
+	case BLK_STS_MEDIUM:
+	case BLK_STS_PROTECTION:
+		return false;
+	}
+
+	/* Anything else could be a path failure, so should be retried */
+	return true;
+}
+
 struct blk_issue_stat {
 	u64 stat;
 };

Signature · 1 line

2.13.6

Johannes Thumshirn

Reviewed-by on v2 3/5

2018-01-10 08:06 UTC #6 lore raw

Re: [dm-devel] [PATCHv2 3/5] block: Provide blk_status_t decoding for path errors

Looks good,

Reviewed-by Johannes Thumshirn <[email protected]>

Signature · 6 lines

Johannes Thumshirn                                          Storage
[email protected]                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Keith Busch

author

2018-01-09 19:04 UTC #7 lore raw

PATCH v2 4/5 nvme/multipath: Use blk_path_error

In reply to Keith Busch

Uses common code for determining if an error should be retried on alternate path.

Acked-by Mike Snitzer <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 drivers/nvme/host/multipath.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

drivers/nvme/host/multipath.c +1 ~~-13~~

index ae9abb600c0f..3b211d9e58b8 100644
@@ -37,19 +37,7 @@ bool nvme_req_needs_failover(struct request *req, blk_status_t error)
 {
 	if (!(req->cmd_flags & REQ_NVME_MPATH))
 		return false;
-
-	switch (error) {
-	case BLK_STS_NOTSUPP:
-	case BLK_STS_NOSPC:
-	case BLK_STS_TARGET:
-	case BLK_STS_NEXUS:
-	case BLK_STS_MEDIUM:
-	case BLK_STS_PROTECTION:
-		return false;
-	}
-
-	/* Everything else could be a path failure, so should be retried */
-	return true;
+	return blk_path_error(error);
 }
 
 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)

Signature · 1 line

2.13.6

Johannes Thumshirn

Reviewed-by on v2 4/5

2018-01-10 08:06 UTC #8 lore raw

Re: [dm-devel] [PATCHv2 4/5] nvme/multipath: Use blk_path_error

Looks good,

Reviewed-by Johannes Thumshirn <[email protected]>

Signature · 6 lines

Johannes Thumshirn                                          Storage
[email protected]                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Keith Busch

author

2018-01-09 19:04 UTC #9 lore raw

PATCH v2 5/5 dm mpath: Use blk_path_error

In reply to Keith Busch

Uses common code for determining if an error should be retried on alternate path.

Acked-by Mike Snitzer <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Signed-off-by Keith Busch <[email protected]>

---
 drivers/md/dm-mpath.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

drivers/md/dm-mpath.c +2 ~~-17~~

index f7810cc869ac..ef57c6d1c887 100644
@@ -1475,21 +1475,6 @@ static void activate_path_work(struct work_struct *work)
 	activate_or_offline_path(pgpath);
 }
 
-static int noretry_error(blk_status_t error)
-{
-	switch (error) {
-	case BLK_STS_NOTSUPP:
-	case BLK_STS_NOSPC:
-	case BLK_STS_TARGET:
-	case BLK_STS_NEXUS:
-	case BLK_STS_MEDIUM:
-		return 1;
-	}
-
-	/* Anything else could be a path failure, so should be retried */
-	return 0;
-}
-
 static int multipath_end_io(struct dm_target *ti, struct request *clone,
 			    blk_status_t error, union map_info *map_context)
 {

@@ -1508,7 +1493,7 @@ static int multipath_end_io(struct dm_target *ti, struct request *clone,
 	 * request into dm core, which will remake a clone request and
 	 * clone bios for it and resubmit it later.
 	 */
-	if (error && !noretry_error(error)) {
+	if (error && blk_path_error(error)) {
 		struct multipath *m = ti->private;
 
 		r = DM_ENDIO_REQUEUE;

@@ -1544,7 +1529,7 @@ static int multipath_end_io_bio(struct dm_target *ti, struct bio *clone,
 	unsigned long flags;
 	int r = DM_ENDIO_DONE;
 
-	if (!*error || noretry_error(*error))
+	if (!*error || !blk_path_error(*error))
 		goto done;
 
 	if (pgpath)

Signature · 1 line

2.13.6

Johannes Thumshirn

Reviewed-by on v2 5/5

2018-01-10 08:07 UTC #10 lore raw

Re: [dm-devel] [PATCHv2 5/5] dm mpath: Use blk_path_error

Looks good,

Reviewed-by Johannes Thumshirn <[email protected]>

Signature · 6 lines

Johannes Thumshirn                                          Storage
[email protected]                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Christoph Hellwig

maintainer Reviewed-by on the v2 series

2018-01-10 08:28 UTC #11 lore raw

In reply to Keith Busch

The whole series looks fine to me:

Reviewed-by Christoph Hellwig <[email protected]>

Jens, do you want me to apply this to the nvme tree, or pick it up directly?

Jens Axboe

maintainer Applied

2018-01-10 17:52 UTC #12 lore raw

Quoted 6 lines · Christoph Hellwig

The whole series looks fine to me:
Reviewed-by Christoph Hellwig <[email protected]>
Jens, do you want me to apply this to the nvme tree, or pick it up directly?

I queued it up, thanks.

Signature · 1 line

Jens Axboe