[PATCH 0/5] Failover criteria unification

[PATCH 0/5] Failover criteria unification

[Keith Busch]

The nvme native 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 that user experience regarding the failover fate of a command is the
same.

Mike:

I split this up because I thought there'd be trouble merging the dm
mpath update with the inverted retry logic, but I think you may have
rebased the dm-4.16 without that patch, as I'm not seeing it in the most
current branch.

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

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

-- 
2.13.6

[PATCH 1/5] nvme: Add more command status translation

[Keith Busch]

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

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/nvme/host/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 2a69d735efbc..f1cf1f6c5b28 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -156,14 +156,20 @@ static blk_status_t nvme_error_status(struct request *req)
 	case NVME_SC_SUCCESS:
 		return BLK_STS_OK;
 	case NVME_SC_CAP_EXCEEDED:
+	case NVME_SC_LBA_RANGE:
 		return BLK_STS_NOSPC;
+	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:
-- 
2.13.6

[PATCH 2/5] nvme/multipath: Consult blk_status_t for failover

[Keith Busch]

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

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/nvme/host/core.c      |  9 +++++----
 drivers/nvme/host/multipath.c | 44 ++++++++-----------------------------------
 drivers/nvme/host/nvme.h      |  4 ++--
 3 files changed, 15 insertions(+), 42 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f1cf1f6c5b28..4afc681f7089 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -196,8 +196,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;
 		}
@@ -208,8 +210,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);
 
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 1218a9fca846..ae9abb600c0f 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -33,46 +33,18 @@ void nvme_failover_req(struct request *req)
 	kblockd_schedule_work(&ns->head->requeue_work);
 }
 
-bool nvme_req_needs_failover(struct request *req)
+bool nvme_req_needs_failover(struct request *req, blk_status_t error)
 {
 	if (!(req->cmd_flags & REQ_NVME_MPATH))
 		return false;
 
-	switch (nvme_req(req)->status & 0x7ff) {
-	/*
-	 * Generic command status:
-	 */
-	case NVME_SC_INVALID_OPCODE:
-	case NVME_SC_INVALID_FIELD:
-	case NVME_SC_INVALID_NS:
-	case NVME_SC_LBA_RANGE:
-	case NVME_SC_CAP_EXCEEDED:
-	case NVME_SC_RESERVATION_CONFLICT:
-		return false;
-
-	/*
-	 * I/O command set specific error.  Unfortunately these values are
-	 * reused for fabrics commands, but those should never get here.
-	 */
-	case NVME_SC_BAD_ATTRIBUTES:
-	case NVME_SC_INVALID_PI:
-	case NVME_SC_READ_ONLY:
-	case NVME_SC_ONCS_NOT_SUPPORTED:
-		WARN_ON_ONCE(nvme_req(req)->cmd->common.opcode ==
-			nvme_fabrics_command);
-		return false;
-
-	/*
-	 * Media and Data Integrity Errors:
-	 */
-	case NVME_SC_WRITE_FAULT:
-	case NVME_SC_READ_ERROR:
-	case NVME_SC_GUARD_CHECK:
-	case NVME_SC_APPTAG_CHECK:
-	case NVME_SC_REFTAG_CHECK:
-	case NVME_SC_COMPARE_FAILED:
-	case NVME_SC_ACCESS_DENIED:
-	case NVME_SC_UNWRITTEN_BLOCK:
+	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;
 	}
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index ea1aa5283e8e..cf8548c1d7ec 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -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,7 @@ 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;
 }
-- 
2.13.6

[PATCH 3/5] block: Provide blk_status_t decoding for retryable errors

[Keith Busch]

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

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 include/linux/blk_types.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index a1e628e032da..b6a8723b493c 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -39,6 +39,22 @@ typedef u8 __bitwise blk_status_t;
 
 #define BLK_STS_AGAIN		((__force blk_status_t)12)
 
+static inline bool blk_retryable(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;
 };
-- 
2.13.6

[PATCH 4/5] nvme/multipath: Use blk_retryable

[Keith Busch]

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

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/nvme/host/multipath.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index ae9abb600c0f..93bb72b6efb6 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -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_retryable(error);
 }
 
 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
-- 
2.13.6

[PATCH 5/5] dm mpath: Use blk_retryable

[Keith Busch]

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

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/md/dm-mpath.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index f7810cc869ac..3eb9500db1b3 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -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_retryable(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_retryable(*error))
 		goto done;
 
 	if (pgpath)
-- 
2.13.6

Re: [PATCH 0/5] Failover criteria unification

[Mike Snitzer]

On Thu, Jan 04 2018 at  5:46pm -0500,
Keith Busch <keith.busch@intel.com> wrote:

> The nvme native 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 that user experience regarding the failover fate of a command is the
> same.
> 
> Mike:
> 
> I split this up because I thought there'd be trouble merging the dm
> mpath update with the inverted retry logic, but I think you may have
> rebased the dm-4.16 without that patch, as I'm not seeing it in the most
> current branch.

Right, I dropped that patch since it'd have only resulted in conflicts
come merge time.  As such, this series can easily go through the nvme
tree to Jens.

Re: [PATCH 1/5] nvme: Add more command status translation

[Mike Snitzer]

On Thu, Jan 04 2018 at  5:46pm -0500,
Keith Busch <keith.busch@intel.com> wrote:

> This adds more NVMe status code translations to blk_status_t values,
> and captures all the current status codes NVMe multipath uses.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>

Acked-by: Mike Snitzer <snitzer@redhat.com>

Re: [PATCH 2/5] nvme/multipath: Consult blk_status_t for failover

[Mike Snitzer]

On Thu, Jan 04 2018 at  5:46pm -0500,
Keith Busch <keith.busch@intel.com> wrote:

> This removes nvme multipath's specific status decoding to see if failover
> is needed, using the generic blk_status_t that was translated earlier. This
> abstraction from the raw NVMe status means nvme status decoding exists
> in just one place.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>

Acked-by: Mike Snitzer <snitzer@redhat.com>

Re: [PATCH 3/5] block: Provide blk_status_t decoding for retryable errors

[Mike Snitzer]

On Thu, Jan 04 2018 at  5:46pm -0500,
Keith Busch <keith.busch@intel.com> wrote:

> This patch provides a common decoder for block status that may be retried
> so various entities wishing to consult this do not have to duplicate
> this decision.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>

Acked-by: Mike Snitzer <snitzer@redhat.com>

Re: [PATCH 4/5] nvme/multipath: Use blk_retryable

[Mike Snitzer]

On Thu, Jan 04 2018 at  5:46pm -0500,
Keith Busch <keith.busch@intel.com> wrote:

> Uses common code for determining if an error should be retried on
> alternate path.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>

Acked-by: Mike Snitzer <snitzer@redhat.com>

Re: [PATCH 5/5] dm mpath: Use blk_retryable

[Mike Snitzer]

On Thu, Jan 04 2018 at  5:46pm -0500,
Keith Busch <keith.busch@intel.com> wrote:

> Uses common code for determining if an error should be retried on
> alternate path.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>

Acked-by: Mike Snitzer <snitzer@redhat.com>

Re: [PATCH 0/5] Failover criteria unification

[Keith Busch]

On Thu, Jan 04, 2018 at 06:36:27PM -0500, Mike Snitzer wrote:
> Right, I dropped that patch since it'd have only resulted in conflicts
> come merge time.  As such, this series can easily go through the nvme
> tree to Jens.

It looks like you can also touch up dm to allow it to multipath nvme
even if CONFIG_NVME_MULTIPATH is set. It may be useful since native NVMe
doesn't multipath namespaces across subsystems, and some crack smoking
people want to create something that requires that.

Re: [PATCH 0/5] Failover criteria unification

[Mike Snitzer]

On Thu, Jan 04 2018 at  6:47pm -0500,
Keith Busch <keith.busch@intel.com> wrote:

> On Thu, Jan 04, 2018 at 06:36:27PM -0500, Mike Snitzer wrote:
> > Right, I dropped that patch since it'd have only resulted in conflicts
> > come merge time.  As such, this series can easily go through the nvme
> > tree to Jens.
> 
> It looks like you can also touch up dm to allow it to multipath nvme
> even if CONFIG_NVME_MULTIPATH is set. It may be useful since native NVMe
> doesn't multipath namespaces across subsystems, and some crack smoking
> people want to create something that requires that.

I had a "CONFIG_NVME_MULITIPATH=Y" typo in the dm-mpath.c commit that
added the CONFIG_NVME_MULTIPATH mutual exclussion in dm-4.16; so I
just dropped it via rebase.

So now the underlying nvme devices (already multipath'd by NVMe) should
be discoverable by multipathd.  Who am I to judge crack smokers?

Re: [PATCH 1/5] nvme: Add more command status translation

[Hannes Reinecke]

On 01/04/2018 11:46 PM, Keith Busch wrote:
> This adds more NVMe status code translations to blk_status_t values,
> and captures all the current status codes NVMe multipath uses.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/nvme/host/core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 2a69d735efbc..f1cf1f6c5b28 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -156,14 +156,20 @@ static blk_status_t nvme_error_status(struct request *req)
>  	case NVME_SC_SUCCESS:
>  		return BLK_STS_OK;
>  	case NVME_SC_CAP_EXCEEDED:
> +	case NVME_SC_LBA_RANGE:
>  		return BLK_STS_NOSPC;
> +	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:
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/5] nvme/multipath: Consult blk_status_t for failover

[Hannes Reinecke]

On 01/04/2018 11:46 PM, Keith Busch wrote:
> This removes nvme multipath's specific status decoding to see if failover
> is needed, using the generic blk_status_t that was translated earlier. This
> abstraction from the raw NVMe status means nvme status decoding exists
> in just one place.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/nvme/host/core.c      |  9 +++++----
>  drivers/nvme/host/multipath.c | 44 ++++++++-----------------------------------
>  drivers/nvme/host/nvme.h      |  4 ++--
>  3 files changed, 15 insertions(+), 42 deletions(-)
> Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/5] block: Provide blk_status_t decoding for retryable errors

[Hannes Reinecke]

On 01/04/2018 11:46 PM, Keith Busch wrote:
> This patch provides a common decoder for block status that may be retried
> so various entities wishing to consult this do not have to duplicate
> this decision.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  include/linux/blk_types.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index a1e628e032da..b6a8723b493c 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -39,6 +39,22 @@ typedef u8 __bitwise blk_status_t;
>  
>  #define BLK_STS_AGAIN		((__force blk_status_t)12)
>  
> +static inline bool blk_retryable(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;
>  };
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 4/5] nvme/multipath: Use blk_retryable

[Hannes Reinecke]

On 01/04/2018 11:46 PM, Keith Busch wrote:
> Uses common code for determining if an error should be retried on
> alternate path.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/nvme/host/multipath.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index ae9abb600c0f..93bb72b6efb6 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -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_retryable(error);
>  }
>  
>  void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 5/5] dm mpath: Use blk_retryable

[Hannes Reinecke]

On 01/04/2018 11:46 PM, Keith Busch wrote:
> Uses common code for determining if an error should be retried on
> alternate path.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/md/dm-mpath.c | 19 ++-----------------
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 1/5] nvme: Add more command status translation

[Christoph Hellwig]

On Thu, Jan 04, 2018 at 03:46:19PM -0700, Keith Busch wrote:
> This adds more NVMe status code translations to blk_status_t values,
> and captures all the current status codes NVMe multipath uses.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/nvme/host/core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 2a69d735efbc..f1cf1f6c5b28 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -156,14 +156,20 @@ static blk_status_t nvme_error_status(struct request *req)
>  	case NVME_SC_SUCCESS:
>  		return BLK_STS_OK;
>  	case NVME_SC_CAP_EXCEEDED:
> +	case NVME_SC_LBA_RANGE:
>  		return BLK_STS_NOSPC;

lba range isn't really enospc.  It is returned when the lba in
the command is outside the logical size of the namespace.

Re: [PATCH 2/5] nvme/multipath: Consult blk_status_t for failover

[Christoph Hellwig]

> -	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)) {

We don't really need to call nvme_error_status if nvme_req(req)->status
is zero.

> -static inline bool nvme_req_needs_failover(struct request *req)
> +static inline bool nvme_req_needs_failover(struct request *req, blk_status_t error)

line break after 80 characters, please.

Re: [PATCH 3/5] block: Provide blk_status_t decoding for retryable errors

[Christoph Hellwig]

> +static inline bool blk_retryable(blk_status_t error)

The naming isn't really useful - it is about the fact that it's
worth retrying on another path.  So please chose a better name,
and add a kerneldoc comment describing it.

Re: [PATCH 0/5] Failover criteria unification

[Christoph Hellwig]

On Thu, Jan 04, 2018 at 04:47:27PM -0700, Keith Busch wrote:
> It looks like you can also touch up dm to allow it to multipath nvme
> even if CONFIG_NVME_MULTIPATH is set. It may be useful since native NVMe
> doesn't multipath namespaces across subsystems, and some crack smoking
> people want to create something that requires that.

Right now that is simply not allowed by the spec.  If Dave comes up
with a palatable way to retrofit that into the spec we'll support it.

Re: [PATCH 1/5] nvme: Add more command status translation

[Hannes Reinecke]

On 01/08/2018 10:55 AM, Christoph Hellwig wrote:
> On Thu, Jan 04, 2018 at 03:46:19PM -0700, Keith Busch wrote:
>> This adds more NVMe status code translations to blk_status_t values,
>> and captures all the current status codes NVMe multipath uses.
>>
>> Signed-off-by: Keith Busch <keith.busch@intel.com>
>> ---
>>  drivers/nvme/host/core.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 2a69d735efbc..f1cf1f6c5b28 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -156,14 +156,20 @@ static blk_status_t nvme_error_status(struct request *req)
>>  	case NVME_SC_SUCCESS:
>>  		return BLK_STS_OK;
>>  	case NVME_SC_CAP_EXCEEDED:
>> +	case NVME_SC_LBA_RANGE:
>>  		return BLK_STS_NOSPC;
> 
> lba range isn't really enospc.  It is returned when the lba in
> the command is outside the logical size of the namespace.
> 
Isn't that distinction pretty academic?
The entire block-to-POSIX error mapping is pretty much ad-hoc anyway...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 1/5] nvme: Add more command status translation

[Christoph Hellwig]

On Mon, Jan 08, 2018 at 11:09:03AM +0100, Hannes Reinecke wrote:
> >>  	case NVME_SC_SUCCESS:
> >>  		return BLK_STS_OK;
> >>  	case NVME_SC_CAP_EXCEEDED:
> >> +	case NVME_SC_LBA_RANGE:
> >>  		return BLK_STS_NOSPC;
> > 
> > lba range isn't really enospc.  It is returned when the lba in
> > the command is outside the logical size of the namespace.
> > 
> Isn't that distinction pretty academic?
> The entire block-to-POSIX error mapping is pretty much ad-hoc anyway...

Yes, BLK_STS_NOSPC matters.  And the fix is pretty trivial, so there is
no point in arguing.

Re: [PATCH 1/5] nvme: Add more command status translation

[Mike Snitzer]

On Mon, Jan 08 2018 at  5:19am -0500,
Christoph Hellwig <hch@lst.de> wrote:

> On Mon, Jan 08, 2018 at 11:09:03AM +0100, Hannes Reinecke wrote:
> > >>  	case NVME_SC_SUCCESS:
> > >>  		return BLK_STS_OK;
> > >>  	case NVME_SC_CAP_EXCEEDED:
> > >> +	case NVME_SC_LBA_RANGE:
> > >>  		return BLK_STS_NOSPC;
> > > 
> > > lba range isn't really enospc.  It is returned when the lba in
> > > the command is outside the logical size of the namespace.
> > > 
> > Isn't that distinction pretty academic?
> > The entire block-to-POSIX error mapping is pretty much ad-hoc anyway...
> 
> Yes, BLK_STS_NOSPC matters.  And the fix is pretty trivial, so there is
> no point in arguing.

No argument needed.  Definitely needs fixing.  Too many upper layers
consider BLK_STS_NOSPC retryable (XFS, ext4, dm-thinp, etc).  Which
NVME_SC_LBA_RANGE absolutely isn't.

When I backfilled NVME_SC_LBA_RANGE handling I categorized it as
BLK_STS_TARGET.  Do you have a better suggestion for how
NVME_SC_LBA_RANGE should be categorized?

Mike

Re: [PATCH 1/5] nvme: Add more command status translation

[Christoph Hellwig]

On Mon, Jan 08, 2018 at 10:29:33AM -0500, Mike Snitzer wrote:
> No argument needed.  Definitely needs fixing.  Too many upper layers
> consider BLK_STS_NOSPC retryable (XFS, ext4, dm-thinp, etc).  Which
> NVME_SC_LBA_RANGE absolutely isn't.
> 
> When I backfilled NVME_SC_LBA_RANGE handling I categorized it as
> BLK_STS_TARGET.  Do you have a better suggestion for how
> NVME_SC_LBA_RANGE should be categorized?

It's basically a kernel bug as it tries to access lbas that do not
exist.  BLK_STS_TARGET should be fine.

Re: [PATCH 1/5] nvme: Add more command status translation

[Keith Busch]

On Mon, Jan 08, 2018 at 04:34:36PM +0100, Christoph Hellwig wrote:
> It's basically a kernel bug as it tries to access lbas that do not
> exist.  BLK_STS_TARGET should be fine.

Okay, I'll fix this and address your other comments, and resend. Thanks
for the feedback.

Re: [PATCH 2/5] nvme/multipath: Consult blk_status_t for failover

[Keith Busch]

On Mon, Jan 08, 2018 at 01:57:07AM -0800, Christoph Hellwig wrote:
> > -	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)) {
> 
> We don't really need to call nvme_error_status if nvme_req(req)->status
> is zero.

We are already calling nvme_error_status unconditionally for
blk_mq_end_request, so we currently read nvme_req(req)->status multiple
times in the completion path. I think we'd prefer to read it just once.

Re: [PATCH 2/5] nvme/multipath: Consult blk_status_t for failover

[Christoph Hellwig]

On Tue, Jan 09, 2018 at 10:38:58AM -0700, Keith Busch wrote:
> On Mon, Jan 08, 2018 at 01:57:07AM -0800, Christoph Hellwig wrote:
> > > -	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)) {
> > 
> > We don't really need to call nvme_error_status if nvme_req(req)->status
> > is zero.
> 
> We are already calling nvme_error_status unconditionally for
> blk_mq_end_request, so we currently read nvme_req(req)->status multiple
> times in the completion path. I think we'd prefer to read it just once.

Indeed.  Objection retracted.