[PATCH v2 0/4] block/scsi/nvme: Add error codes for PR ops

[PATCH v2 0/4] block/scsi/nvme: Add error codes for PR ops

[Mike Christie]

The following patches were made over Linus's tree and allow the PR/pr_ops
users to handle errors without having to know the device type and also
for SCSI handle devices that require the sense code. Currently, we return
a -Exyz type of error code if the PR call fails before the drivers can
send the command and a device specific error code if it's queued. The
problem is that the callers don't always know the device type so they
can't check for specific errors like reservation conflicts, or transport
errors or invalid operations.

These patches add common error codes which callers can check for.

v2:
- Drop PR_STS_OP_NOT_SUPP and PR_STS_OP_INVALID.
- Drop dependence on scsi_exeucte patchset and include status_byte
patch in this patchset.
- Check for all nvme path errors with nvme_is_path_error.

[PATCH v2 1/4] block: Add error codes for common PR failures

[Mike Christie]

If a PR operation fails we can return a device specific error which is
impossible to handle in some cases because we could have a mix of devices
when DM is used, or future users like lio only know it's interacting with
a block device so it doesn't know the type.

This patch adds a new pr_status enum so drivers can convert errors to a
common type which can be handled by the caller.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 include/uapi/linux/pr.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/uapi/linux/pr.h b/include/uapi/linux/pr.h
index ccc78cbf1221..d8126415966f 100644
--- a/include/uapi/linux/pr.h
+++ b/include/uapi/linux/pr.h
@@ -4,6 +4,23 @@
 
 #include <linux/types.h>
 
+enum pr_status {
+	PR_STS_SUCCESS			= 0x0,
+	/*
+	 * The following error codes are based on SCSI, because the interface
+	 * was originally created for it and has existing users.
+	 */
+	/* Generic device failure. */
+	PR_STS_IOERR			= 0x2,
+	PR_STS_RESERVATION_CONFLICT	= 0x18,
+	/* Temporary path failure that can be retried. */
+	PR_STS_RETRY_PATH_FAILURE	= 0xe0000,
+	/* The request was failed due to a fast failure timer. */
+	PR_STS_PATH_FAST_FAILED		= 0xf0000,
+	/* The path cannot be reached and has been marked as failed. */
+	PR_STS_PATH_FAILED		= 0x10000,
+};
+
 enum pr_type {
 	PR_WRITE_EXCLUSIVE		= 1,
 	PR_EXCLUSIVE_ACCESS		= 2,
-- 
2.25.1

Re: [PATCH v2 1/4] block: Add error codes for common PR failures

[Chaitanya Kulkarni]

On 11/15/22 13:28, Mike Christie wrote:
> If a PR operation fails we can return a device specific error which is
> impossible to handle in some cases because we could have a mix of devices
> when DM is used, or future users like lio only know it's interacting with

long lines above ? just check

> a block device so it doesn't know the type.
> 
> This patch adds a new pr_status enum so drivers can convert errors to a
> common type which can be handled by the caller.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH v2 1/4] block: Add error codes for common PR failures

[Christoph Hellwig]

Looks good:

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

[PATCH v2 2/4] scsi: Rename status_byte to sg_status_byte

[Mike Christie]

The next patch adds a helper status_byte function that works like
host_byte, so this patch renames the old status_byte to sg_status_byte
since it's only used for SG IO.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/scsi_ioctl.c | 2 +-
 drivers/scsi/sg.c         | 2 +-
 include/scsi/sg.h         | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index 2d20da55fb64..8baff7edf7c3 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -376,7 +376,7 @@ static int scsi_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 	 * fill in all the output members
 	 */
 	hdr->status = scmd->result & 0xff;
-	hdr->masked_status = status_byte(scmd->result);
+	hdr->masked_status = sg_status_byte(scmd->result);
 	hdr->msg_status = COMMAND_COMPLETE;
 	hdr->host_status = host_byte(scmd->result);
 	hdr->driver_status = 0;
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index ce34a8ad53b4..d61d8d0d1658 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1349,7 +1349,7 @@ sg_rq_end_io(struct request *rq, blk_status_t status)
 		struct scsi_sense_hdr sshdr;
 
 		srp->header.status = 0xff & result;
-		srp->header.masked_status = status_byte(result);
+		srp->header.masked_status = sg_status_byte(result);
 		srp->header.msg_status = COMMAND_COMPLETE;
 		srp->header.host_status = host_byte(result);
 		srp->header.driver_status = driver_byte(result);
diff --git a/include/scsi/sg.h b/include/scsi/sg.h
index 068e35d36557..af31cecd9012 100644
--- a/include/scsi/sg.h
+++ b/include/scsi/sg.h
@@ -159,7 +159,7 @@ struct compat_sg_io_hdr {
 #define TASK_ABORTED         0x20
 
 /* Obsolete status_byte() declaration */
-#define status_byte(result) (((result) >> 1) & 0x7f)
+#define sg_status_byte(result) (((result) >> 1) & 0x7f)
 
 typedef struct sg_scsi_id { /* used by SG_GET_SCSI_ID ioctl() */
     int host_no;        /* as in "scsi<n>" where 'n' is one of 0, 1, 2 etc */
-- 
2.25.1

Re: [PATCH v2 2/4] scsi: Rename status_byte to sg_status_byte

[Chaitanya Kulkarni]

On 11/15/22 13:28, Mike Christie wrote:
> The next patch adds a helper status_byte function that works like
> host_byte, so this patch renames the old status_byte to sg_status_byte
> since it's only used for SG IO.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH v2 2/4] scsi: Rename status_byte to sg_status_byte

[Christoph Hellwig]

Looks good:

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

[PATCH v2 3/4] scsi: Convert SCSI errors to PR errors

[Mike Christie]

This converts the SCSI errors we commonly see during PR handling to PR_STS
errors or -Exyz errors. pr_ops callers can then handle scsi and nvme errors
without knowing the device types.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c   | 42 +++++++++++++++++++++++++++++++++++++++++-
 include/scsi/scsi.h |  1 +
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index eb76ba055021..00cc17fe769b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1701,6 +1701,43 @@ static char sd_pr_type(enum pr_type type)
 	}
 };
 
+static int sd_scsi_to_pr_err(struct scsi_sense_hdr *sshdr, int result)
+{
+	int err = PR_STS_IOERR;
+
+	switch host_byte(result) {
+	case DID_TRANSPORT_MARGINAL:
+	case DID_TRANSPORT_DISRUPTED:
+	case DID_BUS_BUSY:
+		err = PR_STS_RETRY_PATH_FAILURE;
+		goto done;
+	case DID_NO_CONNECT:
+		err = PR_STS_PATH_FAILED;
+		goto done;
+	case DID_TRANSPORT_FAILFAST:
+		err = PR_STS_PATH_FAST_FAILED;
+		goto done;
+	}
+
+	switch (status_byte(result)) {
+	case SAM_STAT_RESERVATION_CONFLICT:
+		err = PR_STS_RESERVATION_CONFLICT;
+		goto done;
+	case SAM_STAT_CHECK_CONDITION:
+		if (!scsi_sense_valid(sshdr))
+			goto done;
+
+		if (sshdr->sense_key == ILLEGAL_REQUEST &&
+		    (sshdr->asc == 0x26 || sshdr->asc == 0x24)) {
+			err = -EINVAL;
+			goto done;
+		}
+	}
+
+done:
+	return err;
+}
+
 static int sd_pr_command(struct block_device *bdev, u8 sa,
 		u64 key, u64 sa_key, u8 type, u8 flags)
 {
@@ -1729,7 +1766,10 @@ static int sd_pr_command(struct block_device *bdev, u8 sa,
 		scsi_print_sense_hdr(sdev, NULL, &sshdr);
 	}
 
-	return result;
+	if (result <= 0)
+		return result;
+
+	return sd_scsi_to_pr_err(&sshdr, result);
 }
 
 static int sd_pr_register(struct block_device *bdev, u64 old_key, u64 new_key,
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 3e46859774c8..ec093594ba53 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -121,6 +121,7 @@ enum scsi_disposition {
  *      msg_byte    (unused)
  *      host_byte   = set by low-level driver to indicate status.
  */
+#define status_byte(result) (result & 0xff)
 #define host_byte(result)   (((result) >> 16) & 0xff)
 
 #define sense_class(sense)  (((sense) >> 4) & 0x7)
-- 
2.25.1

Re: [PATCH v2 3/4] scsi: Convert SCSI errors to PR errors

[Chaitanya Kulkarni]

On 11/15/22 13:28, Mike Christie wrote:
> This converts the SCSI errors we commonly see during PR handling to PR_STS
> errors or -Exyz errors. pr_ops callers can then handle scsi and nvme errors
> without knowing the device types.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[PATCH v2 4/4] nvme: Convert NVMe errors to PR errors

[Mike Christie]

This converts the NVMe errors we commonly see during PR handling to PR_STS
errors or -Exyz errors. pr_ops callers can then handle scsi and nvme errors
without knowing the device types.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/nvme/host/core.c | 42 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index dc4220600585..811de141a7ee 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2104,11 +2104,43 @@ static int nvme_send_ns_pr_command(struct nvme_ns *ns, struct nvme_command *c,
 	return nvme_submit_sync_cmd(ns->queue, c, data, 16);
 }
 
+static int nvme_sc_to_pr_err(int nvme_sc)
+{
+	int err;
+
+	if (nvme_is_path_error(nvme_sc))
+		return PR_STS_PATH_FAILED;
+
+	switch (nvme_sc) {
+	case NVME_SC_SUCCESS:
+		err = PR_STS_SUCCESS;
+		break;
+	case NVME_SC_RESERVATION_CONFLICT:
+		err = PR_STS_RESERVATION_CONFLICT;
+		break;
+	case NVME_SC_ONCS_NOT_SUPPORTED:
+		err = -EOPNOTSUPP;
+		break;
+	case NVME_SC_BAD_ATTRIBUTES:
+	case NVME_SC_INVALID_OPCODE:
+	case NVME_SC_INVALID_FIELD:
+	case NVME_SC_INVALID_NS:
+		err = -EINVAL;
+		break;
+	default:
+		err = PR_STS_IOERR;
+		break;
+	}
+
+	return err;
+}
+
 static int nvme_pr_command(struct block_device *bdev, u32 cdw10,
 				u64 key, u64 sa_key, u8 op)
 {
 	struct nvme_command c = { };
 	u8 data[16] = { 0, };
+	int ret;
 
 	put_unaligned_le64(key, &data[0]);
 	put_unaligned_le64(sa_key, &data[8]);
@@ -2118,8 +2150,14 @@ static int nvme_pr_command(struct block_device *bdev, u32 cdw10,
 
 	if (IS_ENABLED(CONFIG_NVME_MULTIPATH) &&
 	    bdev->bd_disk->fops == &nvme_ns_head_ops)
-		return nvme_send_ns_head_pr_command(bdev, &c, data);
-	return nvme_send_ns_pr_command(bdev->bd_disk->private_data, &c, data);
+		ret = nvme_send_ns_head_pr_command(bdev, &c, data);
+	else
+		ret = nvme_send_ns_pr_command(bdev->bd_disk->private_data, &c,
+					      data);
+	if (ret < 0)
+		return ret;
+
+	return nvme_sc_to_pr_err(ret);
 }
 
 static int nvme_pr_register(struct block_device *bdev, u64 old,
-- 
2.25.1

Re: [PATCH v2 4/4] nvme: Convert NVMe errors to PR errors

[Chaitanya Kulkarni]

On 11/15/22 13:28, Mike Christie wrote:
> This converts the NVMe errors we commonly see during PR handling to PR_STS
> errors or -Exyz errors. pr_ops callers can then handle scsi and nvme errors
> without knowing the device types.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/nvme/host/core.c | 42 ++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index dc4220600585..811de141a7ee 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2104,11 +2104,43 @@ static int nvme_send_ns_pr_command(struct nvme_ns *ns, struct nvme_command *c,
>   	return nvme_submit_sync_cmd(ns->queue, c, data, 16);
>   }
>   
> +static int nvme_sc_to_pr_err(int nvme_sc)
> +{
> +	int err;
> +
> +	if (nvme_is_path_error(nvme_sc))
> +		return PR_STS_PATH_FAILED;
> +
> +	switch (nvme_sc) {
> +	case NVME_SC_SUCCESS:
> +		err = PR_STS_SUCCESS;
> +		break;
> +	case NVME_SC_RESERVATION_CONFLICT:
> +		err = PR_STS_RESERVATION_CONFLICT;
> +		break;
> +	case NVME_SC_ONCS_NOT_SUPPORTED:
> +		err = -EOPNOTSUPP;
> +		break;
> +	case NVME_SC_BAD_ATTRIBUTES:
> +	case NVME_SC_INVALID_OPCODE:
> +	case NVME_SC_INVALID_FIELD:
> +	case NVME_SC_INVALID_NS:
> +		err = -EINVAL;
> +		break;
> +	default:
> +		err = PR_STS_IOERR;
> +		break;
> +	}
> +
> +	return err;
> +}
> +
>   

Like nvmet_bdev_parse_io_cmd() if we return directly we can remove
the local variable and break for each case [1], no big deal feel free
to ignore this comment.

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[1]
static int nvme_sc_to_pr_err(int nvme_sc)
{
	if (nvme_is_path_error(nvme_sc))
		return PR_STS_PATH_FAILED;

	switch (nvme_sc) {
	case NVME_SC_SUCCESS:
		return PR_STS_SUCCESS;
	case NVME_SC_RESERVATION_CONFLICT:
		return PR_STS_RESERVATION_CONFLICT;
	case NVME_SC_ONCS_NOT_SUPPORTED:
		return -EOPNOTSUPP;
	case NVME_SC_BAD_ATTRIBUTES:
	case NVME_SC_INVALID_OPCODE:
	case NVME_SC_INVALID_FIELD:
	case NVME_SC_INVALID_NS:
		return -EINVAL;
	default:
		return PR_STS_IOERR;
	}
}

Re: [PATCH v2 4/4] nvme: Convert NVMe errors to PR errors

[Christoph Hellwig]

On Tue, Nov 15, 2022 at 03:28:25PM -0600, Mike Christie wrote:
> This converts the NVMe errors we commonly see during PR handling to PR_STS
> errors or -Exyz errors. pr_ops callers can then handle scsi and nvme errors
> without knowing the device types.

Looks fine, although the improvement suggested by Chaitanya would be
nice:

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