diff mbox series

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

Message ID 20221109031106.201324-4-michael.christie@oracle.com
State Superseded
Headers show
Series block/scsi/nvme: Add error codes for PR ops | expand

Commit Message

Mike Christie Nov. 9, 2022, 3:11 a.m. UTC
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(-)

Comments

Mike Christie Nov. 9, 2022, 5:20 p.m. UTC | #1
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.
Mike Christie Nov. 9, 2022, 5:35 p.m. UTC | #2
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?
Chao Leng Nov. 10, 2022, 12:58 a.m. UTC | #3
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.
Christoph Hellwig Nov. 15, 2022, 9:14 a.m. UTC | #4
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.
Mike Christie Nov. 15, 2022, 4:56 p.m. UTC | #5
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.
diff mbox series

Patch

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,