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

[PATCH 0/3] 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.

[PATCH 1/3] 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>
---
 include/uapi/linux/pr.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/uapi/linux/pr.h b/include/uapi/linux/pr.h
index ccc78cbf1221..16b856fb8053 100644
--- a/include/uapi/linux/pr.h
+++ b/include/uapi/linux/pr.h
@@ -4,6 +4,30 @@
 
 #include <linux/types.h>
 
+enum pr_status {
+	PR_STS_SUCCESS			= 0x0,
+	/*
+	 * These error codes have no mappings to existing SCSI errors.
+	 */
+	/* The request is not supported. */
+	PR_STS_OP_NOT_SUPP		= 0x7fffffff,
+	/* The request is invalid/illegal. */
+	PR_STS_OP_INVALID		= 0x7ffffffe,
+	/*
+	 * 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 1/3] block: Add error codes for common PR failures

[Christoph Hellwig]

On Tue, Nov 08, 2022 at 09:11:04PM -0600, 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
> 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>
> ---
>  include/uapi/linux/pr.h | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/include/uapi/linux/pr.h b/include/uapi/linux/pr.h
> index ccc78cbf1221..16b856fb8053 100644
> --- a/include/uapi/linux/pr.h
> +++ b/include/uapi/linux/pr.h
> @@ -4,6 +4,30 @@
>  
>  #include <linux/types.h>
>  
> +enum pr_status {
> +	PR_STS_SUCCESS			= 0x0,
> +	/*
> +	 * These error codes have no mappings to existing SCSI errors.
> +	 */
> +	/* The request is not supported. */
> +	PR_STS_OP_NOT_SUPP		= 0x7fffffff,
> +	/* The request is invalid/illegal. */
> +	PR_STS_OP_INVALID		= 0x7ffffffe,
> +	/*
> +	 * 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,

Nit: I'd movee the NOT_SUPP/INVALID to the end to follow the numerical
order.

Otherwise looks good:

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

[PATCH 2/3] scsi: Convert SCSI errors to PR_STS errors

[Mike Christie]

This converts the SCSI errors we commonly see during PR handling to PT_STS
errors, so pr_ops callers can handle scsi and nvme errors without knowing
the device types.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/sd.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index eb76ba055021..667477d1e4b7 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1701,6 +1701,44 @@ static char sd_pr_type(enum pr_type type)
 	}
 };
 
+static enum pr_status sd_scsi_to_block_pr_err(struct scsi_sense_hdr *sshdr,
+					      int result)
+{
+	enum pr_status sts = PR_STS_IOERR;
+
+	switch host_byte(result) {
+	case DID_TRANSPORT_MARGINAL:
+	case DID_TRANSPORT_DISRUPTED:
+	case DID_BUS_BUSY:
+		sts = PR_STS_RETRY_PATH_FAILURE;
+		goto done;
+	case DID_NO_CONNECT:
+		sts = PR_STS_PATH_FAILED;
+		goto done;
+	case DID_TRANSPORT_FAILFAST:
+		sts = PR_STS_PATH_FAST_FAILED;
+		goto done;
+	}
+
+	switch (__get_status_byte(result)) {
+	case SAM_STAT_RESERVATION_CONFLICT:
+		sts = 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)) {
+			sts = PR_STS_OP_INVALID;
+			goto done;
+		}
+	}
+
+done:
+	return sts;
+}
+
 static int sd_pr_command(struct block_device *bdev, u8 sa,
 		u64 key, u64 sa_key, u8 type, u8 flags)
 {
@@ -1729,7 +1767,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_block_pr_err(&sshdr, result);
 }
 
 static int sd_pr_register(struct block_device *bdev, u64 old_key, u64 new_key,
-- 
2.25.1

Re: [PATCH 2/3] scsi: Convert SCSI errors to PR_STS errors

[Christoph Hellwig]

On Tue, Nov 08, 2022 at 09:11:05PM -0600, Mike Christie wrote:
> This converts the SCSI errors we commonly see during PR handling to PT_STS
> errors, so pr_ops callers can handle scsi and nvme errors without knowing
> the device types.

Looks good:

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

[PATCH 3/3] nvme: Convert NVMe errors to PT_STS errors

[Mike Christie]

This converts the NVMe errors we could see during PR handling to PT_STS
errors, so pr_ops callers can 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..8f0177045a2f 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 enum pr_status nvme_sc_to_pr_status(int nvme_sc)
+{
+	enum pr_status sts;
+
+	switch (nvme_sc) {
+	case NVME_SC_SUCCESS:
+		sts = PR_STS_SUCCESS;
+		break;
+	case NVME_SC_RESERVATION_CONFLICT:
+		sts = PR_STS_RESERVATION_CONFLICT;
+		break;
+	case NVME_SC_HOST_PATH_ERROR:
+		sts = PR_STS_PATH_FAILED;
+		break;
+	case NVME_SC_ONCS_NOT_SUPPORTED:
+		sts = PR_STS_OP_NOT_SUPP;
+		break;
+	case NVME_SC_BAD_ATTRIBUTES:
+	case NVME_SC_INVALID_OPCODE:
+	case NVME_SC_INVALID_FIELD:
+	case NVME_SC_INVALID_NS:
+		sts = PR_STS_OP_INVALID;
+		break;
+	default:
+		sts = PR_STS_IOERR;
+		break;
+	}
+
+	return sts;
+}
+
 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_status(ret);
 }
 
 static int nvme_pr_register(struct block_device *bdev, u64 old,
-- 
2.25.1

Re: [PATCH 3/3] nvme: Convert NVMe errors to PT_STS errors

[Christoph Hellwig]

On Tue, Nov 08, 2022 at 09:11:06PM -0600, Mike Christie wrote:
> +	case NVME_SC_ONCS_NOT_SUPPORTED:
> +		sts = PR_STS_OP_NOT_SUPP;
> +		break;
> +	case NVME_SC_BAD_ATTRIBUTES:
> +	case NVME_SC_INVALID_OPCODE:
> +	case NVME_SC_INVALID_FIELD:
> +	case NVME_SC_INVALID_NS:
> +		sts = PR_STS_OP_INVALID;
> +		break;

Second thoughts on these: shouldn't we just return negative Linux
errnos here?

Re: [PATCH 3/3] nvme: Convert NVMe errors to PT_STS errors

[Mike Christie]

On 11/9/22 12:53 AM, Christoph Hellwig wrote:
> On Tue, Nov 08, 2022 at 09:11:06PM -0600, Mike Christie wrote:
>> +	case NVME_SC_ONCS_NOT_SUPPORTED:
>> +		sts = PR_STS_OP_NOT_SUPP;
>> +		break;
>> +	case NVME_SC_BAD_ATTRIBUTES:
>> +	case NVME_SC_INVALID_OPCODE:
>> +	case NVME_SC_INVALID_FIELD:
>> +	case NVME_SC_INVALID_NS:
>> +		sts = PR_STS_OP_INVALID;
>> +		break;
> 
> Second thoughts on these: shouldn't we just return negative Linux
> errnos here?

I wasn't sure. I might have over thought it.

I added the PR_STS error codes for those cases so a user could
distinguish if the command was sent to the device and it
reported it didn't support the command or the device determined it
had an invalid field set.

-EINVAL/-EOPNOTSUP would continue to work like it does now where
we can get those errors if the drivers determined it didn't support
a operation or field or it thought we had an invalid setting.

There is no specific error case I was hitting. I was just thinking
it's nice for userspace to be able to do a PR op and if it got
-EOPNOTSUP the driver didn't support the command and if it got
PR_STS_OP_NOT_SUPP then the device didn't support it.

Re: [PATCH 3/3] nvme: Convert NVMe errors to PT_STS errors

[Christoph Hellwig]

On Wed, Nov 09, 2022 at 11:20:07AM -0600, Mike Christie wrote:
> >> +	case NVME_SC_BAD_ATTRIBUTES:
> >> +	case NVME_SC_INVALID_OPCODE:
> >> +	case NVME_SC_INVALID_FIELD:
> >> +	case NVME_SC_INVALID_NS:
> >> +		sts = PR_STS_OP_INVALID;
> >> +		break;
> > 
> > Second thoughts on these: shouldn't we just return negative Linux
> > errnos here?
> 
> I wasn't sure. I might have over thought it.
> 
> I added the PR_STS error codes for those cases so a user could
> distinguish if the command was sent to the device and it
> reported it didn't support the command or the device determined it
> had an invalid field set.

But does it matter if the device or the kernel doesn't support
them?  The result for the users is very much the same.

Re: [PATCH 3/3] nvme: Convert NVMe errors to PT_STS errors

[Mike Christie]

On 11/15/22 3:14 AM, Christoph Hellwig wrote:
> On Wed, Nov 09, 2022 at 11:20:07AM -0600, Mike Christie wrote:
>>>> +	case NVME_SC_BAD_ATTRIBUTES:
>>>> +	case NVME_SC_INVALID_OPCODE:
>>>> +	case NVME_SC_INVALID_FIELD:
>>>> +	case NVME_SC_INVALID_NS:
>>>> +		sts = PR_STS_OP_INVALID;
>>>> +		break;
>>>
>>> Second thoughts on these: shouldn't we just return negative Linux
>>> errnos here?
>>
>> I wasn't sure. I might have over thought it.
>>
>> I added the PR_STS error codes for those cases so a user could
>> distinguish if the command was sent to the device and it
>> reported it didn't support the command or the device determined it
>> had an invalid field set.
> 
> But does it matter if the device or the kernel doesn't support
> them?  The result for the users is very much the same.

Yeah, makes sense.

Re: [PATCH 3/3] nvme: Convert NVMe errors to PT_STS errors

[Chao Leng]

On 2022/11/9 11:11, Mike Christie wrote:
> This converts the NVMe errors we could see during PR handling to PT_STS
> errors, so pr_ops callers can 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..8f0177045a2f 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 enum pr_status nvme_sc_to_pr_status(int nvme_sc)
> +{
> +	enum pr_status sts;
> +
> +	switch (nvme_sc) {
> +	case NVME_SC_SUCCESS:
> +		sts = PR_STS_SUCCESS;
> +		break;
> +	case NVME_SC_RESERVATION_CONFLICT:
> +		sts = PR_STS_RESERVATION_CONFLICT;
> +		break;
> +	case NVME_SC_HOST_PATH_ERROR:
> +		sts = PR_STS_PATH_FAILED;
All path-related errors should be considered.

Re: [PATCH 3/3] nvme: Convert NVMe errors to PT_STS errors

[Mike Christie]

On 11/9/22 2:28 AM, Chao Leng wrote:
> 
> 
> On 2022/11/9 11:11, Mike Christie wrote:
>> This converts the NVMe errors we could see during PR handling to PT_STS
>> errors, so pr_ops callers can 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..8f0177045a2f 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 enum pr_status nvme_sc_to_pr_status(int nvme_sc)
>> +{
>> +    enum pr_status sts;
>> +
>> +    switch (nvme_sc) {
>> +    case NVME_SC_SUCCESS:
>> +        sts = PR_STS_SUCCESS;
>> +        break;
>> +    case NVME_SC_RESERVATION_CONFLICT:
>> +        sts = PR_STS_RESERVATION_CONFLICT;
>> +        break;
>> +    case NVME_SC_HOST_PATH_ERROR:
>> +        sts = PR_STS_PATH_FAILED;
> All path-related errors should be considered.

Will do. Just one question.

I didn't see NVME_SC_CTRL_PATH_ERROR and NVME_SC_INTERNAL_PATH_ERROR
being used. Are they retryable errors?

Re: [PATCH 3/3] nvme: Convert NVMe errors to PT_STS errors

[Chao Leng]

On 2022/11/10 1:35, Mike Christie wrote:
> On 11/9/22 2:28 AM, Chao Leng wrote:
>>
>>
>> On 2022/11/9 11:11, Mike Christie wrote:
>>> This converts the NVMe errors we could see during PR handling to PT_STS
>>> errors, so pr_ops callers can 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..8f0177045a2f 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 enum pr_status nvme_sc_to_pr_status(int nvme_sc)
>>> +{
>>> +    enum pr_status sts;
>>> +
>>> +    switch (nvme_sc) {
>>> +    case NVME_SC_SUCCESS:
>>> +        sts = PR_STS_SUCCESS;
>>> +        break;
>>> +    case NVME_SC_RESERVATION_CONFLICT:
>>> +        sts = PR_STS_RESERVATION_CONFLICT;
>>> +        break;
>>> +    case NVME_SC_HOST_PATH_ERROR:
>>> +        sts = PR_STS_PATH_FAILED;
>> All path-related errors should be considered.
> 
> Will do. Just one question.
> 
> I didn't see NVME_SC_CTRL_PATH_ERROR and NVME_SC_INTERNAL_PATH_ERROR
> being used. Are they retryable errors?
These two types of errors depend on the implementation of the target.
All in all, the request with path-related error should fail over to retry successfully.