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

[PATCH v3 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.

v3:
- Rework nvme_sc_to_pr_err so it returns directly instead of using a
local variable. Also do the same for scsi's converter.

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 v3 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>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 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

[PATCH v3 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>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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

[PATCH v3 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>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/scsi/sd.c   | 35 ++++++++++++++++++++++++++++++++++-
 include/scsi/scsi.h |  1 +
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index eb76ba055021..bc60ec91dc8f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1701,6 +1701,36 @@ static char sd_pr_type(enum pr_type type)
 	}
 };
 
+static int sd_scsi_to_pr_err(struct scsi_sense_hdr *sshdr, int result)
+{
+	switch (host_byte(result)) {
+	case DID_TRANSPORT_MARGINAL:
+	case DID_TRANSPORT_DISRUPTED:
+	case DID_BUS_BUSY:
+		return PR_STS_RETRY_PATH_FAILURE;
+	case DID_NO_CONNECT:
+		return PR_STS_PATH_FAILED;
+	case DID_TRANSPORT_FAILFAST:
+		return PR_STS_PATH_FAST_FAILED;
+	}
+
+	switch (status_byte(result)) {
+	case SAM_STAT_RESERVATION_CONFLICT:
+		return PR_STS_RESERVATION_CONFLICT;
+	case SAM_STAT_CHECK_CONDITION:
+		if (!scsi_sense_valid(sshdr))
+			return PR_STS_IOERR;
+
+		if (sshdr->sense_key == ILLEGAL_REQUEST &&
+		    (sshdr->asc == 0x26 || sshdr->asc == 0x24))
+			return -EINVAL;
+
+		fallthrough;
+	default:
+		return PR_STS_IOERR;
+	}
+}
+
 static int sd_pr_command(struct block_device *bdev, u8 sa,
 		u64 key, u64 sa_key, u8 type, u8 flags)
 {
@@ -1729,7 +1759,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

[PATCH v3 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>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index da55ce45ac70..6cd66f6d5e9b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2103,11 +2103,34 @@ 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)
+{
+	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;
+	}
+}
+
 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]);
@@ -2117,8 +2140,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 v3 0/4] block/scsi/nvme: Add error codes for PR ops

[Martin K. Petersen]

Mike,

> 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.

This looks OK to me. Not sure which tree makes the most sense to funnel
this through?

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Chaitanya Kulkarni]

On 11/25/22 18:48, Martin K. Petersen wrote:
> 
> Mike,
> 
>> 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.
> 
> This looks OK to me. Not sure which tree makes the most sense to funnel
> this through?
> 
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
> 

perhaps a block tree since it has block/scsi/nvme ?

-ck

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

[hch]

On Tue, Nov 29, 2022 at 04:18:19AM +0000, Chaitanya Kulkarni wrote:
> > Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
> > 
> 
> perhaps a block tree since it has block/scsi/nvme ?

I think Mike has SCSI work that builds on top of this, and reservations
ar originally a SCSI feature.  But either block or scsi is fine with
me.

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

[Jens Axboe]

On 11/29/22 6:28 AM, hch@lst.de wrote:
> On Tue, Nov 29, 2022 at 04:18:19AM +0000, Chaitanya Kulkarni wrote:
>>> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
>>>
>>
>> perhaps a block tree since it has block/scsi/nvme ?
> 
> I think Mike has SCSI work that builds on top of this, and reservations
> ar originally a SCSI feature.  But either block or scsi is fine with
> me.

I'm fine with scsi or block, I'm assuming we won't have any
conflicts from this on the block/nvme side?

If we're doing block just let me know and I can queue it up.

-- 
Jens Axboe

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

[Mike Christie]

On 11/29/22 8:00 AM, Jens Axboe wrote:
> On 11/29/22 6:28 AM, hch@lst.de wrote:
>> On Tue, Nov 29, 2022 at 04:18:19AM +0000, Chaitanya Kulkarni wrote:
>>>> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
>>>>
>>>
>>> perhaps a block tree since it has block/scsi/nvme ?
>>
>> I think Mike has SCSI work that builds on top of this, and reservations
>> ar originally a SCSI feature.  But either block or scsi is fine with
>> me.
> 
> I'm fine with scsi or block, I'm assuming we won't have any
> conflicts from this on the block/nvme side?

This patchset has no conflicts with anyone's trees right now.

I have more patchsets that also touch the block, scsi and nvme
layers that build on this set. The future patches are more
heavy on the scsi side if that makes a difference.

> 
> If we're doing block just let me know and I can queue it up.
> 

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

[Martin K. Petersen]

Mike,

> This patchset has no conflicts with anyone's trees right now.
>
> I have more patchsets that also touch the block, scsi and nvme
> layers that build on this set. The future patches are more
> heavy on the scsi side if that makes a difference.

OK. I merged this series into 6.2/scsi-staging. There really weren't any
non-SCSI changes except for the NVMe tweak.

-- 
Martin K. Petersen	Oracle Linux Engineering