[PATCH] nvme: make nvme error status codes converted to errno detailed

[PATCH] nvme: make nvme error status codes converted to errno detailed

[Guan Junxiong]

From: Junxiong Guan <guanjunxiong@huawei.com>

For more detailed information about nvme error status when ending
blk_mq request,some of nvme error status codes can be categorized into
different errnos explicitly. For example, NVME_SC_ACCESS_DENIED and
NVME_SC_CONNECT_CTRL_BUSY can be converted to EACCESS and EBUSY repec-
tively.This patch makes conversion from those nvme error status to errno
detailed.
---
 drivers/nvme/host/nvme.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a3da1e90b99d..be699ca38cc8 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -255,7 +255,37 @@ static inline int nvme_error_status(u16 status)
 	case NVME_SC_SUCCESS:
 		return 0;
 	case NVME_SC_CAP_EXCEEDED:
+	case NVME_SC_NS_INSUFFICENT_CAP:
 		return -ENOSPC;
+	case NVME_SC_READ_ONLY:
+	case NVME_SC_ACCESS_DENIED:
+		return -EACCES;
+	case NVME_SC_LBA_RANGE:
+		return -EFAULT;
+	case NVME_SC_CONNECT_CTRL_BUSY:
+		return -EBUSY;
+	case NVME_SC_INVALID_OPCODE:
+	case NVME_SC_INVALID_FIELD:
+	case NVME_SC_INVALID_NS:
+	case NVME_SC_SGL_INVALID_LAST:
+	case NVME_SC_SGL_INVALID_COUNT:
+	case NVME_SC_SGL_INVALID_DATA:
+	case NVME_SC_SGL_INVALID_METADATA:
+	case NVME_SC_SGL_INVALID_TYPE:
+	case NVME_SC_SGL_INVALID_OFFSET:
+	case NVME_SC_SGL_INVALID_SUBTYPE:
+	case NVME_SC_CQ_INVALID:
+	case NVME_SC_QID_INVALID:
+	case NVME_SC_QUEUE_SIZE:
+	case NVME_SC_FIRMWARE_SLOT:
+	case NVME_SC_FIRMWARE_IMAGE:
+	case NVME_SC_INVALID_VECTOR:
+	case NVME_SC_INVALID_LOG_PAGE:
+	case NVME_SC_INVALID_FORMAT:
+	case NVME_SC_CTRL_LIST_INVALID:
+		return -EINVAL;
+	case NVME_SC_CMD_SEQ_ERROR:
+		return -EPROTO;
 	default:
 		return -EIO;
 	}
-- 
2.11.1

[PATCH] nvme: make nvme error status codes converted to errno detailed

[Sagi Grimberg]

On 29/03/17 13:08, Guan Junxiong wrote:
> From: Junxiong Guan <guanjunxiong at huawei.com>
>
> For more detailed information about nvme error status when ending
> blk_mq request,some of nvme error status codes can be categorized into
> different errnos explicitly. For example, NVME_SC_ACCESS_DENIED and
> NVME_SC_CONNECT_CTRL_BUSY can be converted to EACCESS and EBUSY repec-
> tively.This patch makes conversion from those nvme error status to errno
> detailed.

This makes sense to me, see comments below.

> ---
>  drivers/nvme/host/nvme.h | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index a3da1e90b99d..be699ca38cc8 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -255,7 +255,37 @@ static inline int nvme_error_status(u16 status)
>  	case NVME_SC_SUCCESS:
>  		return 0;
>  	case NVME_SC_CAP_EXCEEDED:
> +	case NVME_SC_NS_INSUFFICENT_CAP:
>  		return -ENOSPC;

why is this ENOSPC?

Maybe we can add also some fabrics status codes?

> +	case NVME_SC_READ_ONLY:
> +	case NVME_SC_ACCESS_DENIED:
> +		return -EACCES;
> +	case NVME_SC_LBA_RANGE:
> +		return -EFAULT;
> +	case NVME_SC_CONNECT_CTRL_BUSY:
case NVME_SC_CONNECT_CTRL_BUSY:

> +		return -EBUSY;



> +	case NVME_SC_INVALID_OPCODE:
> +	case NVME_SC_INVALID_FIELD:
> +	case NVME_SC_INVALID_NS:
> +	case NVME_SC_SGL_INVALID_LAST:
> +	case NVME_SC_SGL_INVALID_COUNT:
> +	case NVME_SC_SGL_INVALID_DATA:
> +	case NVME_SC_SGL_INVALID_METADATA:
> +	case NVME_SC_SGL_INVALID_TYPE:
> +	case NVME_SC_SGL_INVALID_OFFSET:
> +	case NVME_SC_SGL_INVALID_SUBTYPE:
> +	case NVME_SC_CQ_INVALID:
> +	case NVME_SC_QID_INVALID:
> +	case NVME_SC_QUEUE_SIZE:
> +	case NVME_SC_FIRMWARE_SLOT:
> +	case NVME_SC_FIRMWARE_IMAGE:
> +	case NVME_SC_INVALID_VECTOR:
> +	case NVME_SC_INVALID_LOG_PAGE:
> +	case NVME_SC_INVALID_FORMAT:
> +	case NVME_SC_CTRL_LIST_INVALID:
case NVME_SC_BAD_ATTRIBUTES:
case NVME_SC_INVALID_PI:
case NVME_SC_CONNECT_FORMAT:
case NVME_SC_CONNECT_INVALID_PARAM:
case NVME_SC_CONNECT_INVALID_HOST:

> +		return -EINVAL;
> +	case NVME_SC_CMD_SEQ_ERROR:
> +		return -EPROTO;
>  	default:
>  		return -EIO;
>  	}
>

It'll also be useful if we can log the specific error (as a human
readable string).

[PATCH] nvme: make nvme error status codes converted to errno detailed

[guanjunxiong]

>> --- a/drivers/nvme/host/nvme.h
>> +++ b/drivers/nvme/host/nvme.h
>> @@ -255,7 +255,37 @@ static inline int nvme_error_status(u16 status)
>>      case NVME_SC_SUCCESS:
>>          return 0;
>>      case NVME_SC_CAP_EXCEEDED:
>> +    case NVME_SC_NS_INSUFFICENT_CAP:
>>          return -ENOSPC;
>
> why is this ENOSPC?

In the NVMe Spec,  Namespace Insufficient Capacity (NVME_SC_NS_INSUFFICENT_CAP) means
that creating the namespace requires more free space than is currently available. However
ENOSPC means no space is left on the device.
If there is a little free space on the device,NVME_SC_NS_INSUFFICENT_CAP will be returned.
Your question is reasonble, so I will categorize NVME_SC_NS_INSUFFICENT_CAP into default EIO.


>> +    case NVME_SC_INVALID_OPCODE:
>> +    case NVME_SC_INVALID_FIELD:
>> +    case NVME_SC_INVALID_NS:
>> +    case NVME_SC_SGL_INVALID_LAST:
>> +    case NVME_SC_SGL_INVALID_COUNT:
>> +    case NVME_SC_SGL_INVALID_DATA:
>> +    case NVME_SC_SGL_INVALID_METADATA:
>> +    case NVME_SC_SGL_INVALID_TYPE:
>> +    case NVME_SC_SGL_INVALID_OFFSET:
>> +    case NVME_SC_SGL_INVALID_SUBTYPE:
>> +    case NVME_SC_CQ_INVALID:
>> +    case NVME_SC_QID_INVALID:
>> +    case NVME_SC_QUEUE_SIZE:
>> +    case NVME_SC_FIRMWARE_SLOT:
>> +    case NVME_SC_FIRMWARE_IMAGE:
>> +    case NVME_SC_INVALID_VECTOR:
>> +    case NVME_SC_INVALID_LOG_PAGE:
>> +    case NVME_SC_INVALID_FORMAT:
>> +    case NVME_SC_CTRL_LIST_INVALID:
> case NVME_SC_BAD_ATTRIBUTES:
> case NVME_SC_INVALID_PI:
> case NVME_SC_CONNECT_FORMAT:
> case NVME_SC_CONNECT_INVALID_PARAM:
> case NVME_SC_CONNECT_INVALID_HOST:
>
>> +        return -EINVAL;

> Maybe we can add also some fabrics status codes?

Nice. In addition, because the value of some fabrics status codes are the same as
the value of the PCIe status code, such as:
	/*
	 * I/O Command Set Specific - NVM commands:
	 */
	NVME_SC_BAD_ATTRIBUTES		= 0x180,
	NVME_SC_INVALID_PI		= 0x181,
	NVME_SC_READ_ONLY		= 0x182,

	/*
	 * I/O Command Set Specific - Fabrics commands:
	 */
	NVME_SC_CONNECT_FORMAT		= 0x180,
	NVME_SC_CONNECT_CTRL_BUSY	= 0x181,
	NVME_SC_CONNECT_INVALID_PARAM	= 0x182,

more consideration needs to be taken into the nvme_error_status(u16 status)
function. Therefore , funct nvme_error_status(u16 status) need to accept one more
parameter to distinguish the duplicated value of  the status codes above. Here is
the new prototype of nvme_error_status function:
	static inline int nvme_error_status(u16 status, bool is_fabrics_cmd);

If command in the request is fabrics command, then is_fabrics_cmd is set to true.
Or else set to false


> It'll also be useful if we can log the specific error (as a human
> readable string).
>

Nice advice. The nvme_status_to_string() function in the nvme-cli will be reused for this.

Thanks.


On 2017/3/30 1:39, Sagi Grimberg wrote:
> 
> 
> On 29/03/17 13:08, Guan Junxiong wrote:
>> From: Junxiong Guan <guanjunxiong at huawei.com>
>>
>> For more detailed information about nvme error status when ending
>> blk_mq request,some of nvme error status codes can be categorized into
>> different errnos explicitly. For example, NVME_SC_ACCESS_DENIED and
>> NVME_SC_CONNECT_CTRL_BUSY can be converted to EACCESS and EBUSY repec-
>> tively.This patch makes conversion from those nvme error status to errno
>> detailed.
> 
> This makes sense to me, see comments below.
> 
>> ---
>>  drivers/nvme/host/nvme.h | 30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>> index a3da1e90b99d..be699ca38cc8 100644
>> --- a/drivers/nvme/host/nvme.h
>> +++ b/drivers/nvme/host/nvme.h
>> @@ -255,7 +255,37 @@ static inline int nvme_error_status(u16 status)
>>      case NVME_SC_SUCCESS:
>>          return 0;
>>      case NVME_SC_CAP_EXCEEDED:
>> +    case NVME_SC_NS_INSUFFICENT_CAP:
>>          return -ENOSPC;
> 
> why is this ENOSPC?
> 
> Maybe we can add also some fabrics status codes?
> 
>> +    case NVME_SC_READ_ONLY:
>> +    case NVME_SC_ACCESS_DENIED:
>> +        return -EACCES;
>> +    case NVME_SC_LBA_RANGE:
>> +        return -EFAULT;
>> +    case NVME_SC_CONNECT_CTRL_BUSY:
> case NVME_SC_CONNECT_CTRL_BUSY:
> 
>> +        return -EBUSY;
> 
> 
> 
>> +    case NVME_SC_INVALID_OPCODE:
>> +    case NVME_SC_INVALID_FIELD:
>> +    case NVME_SC_INVALID_NS:
>> +    case NVME_SC_SGL_INVALID_LAST:
>> +    case NVME_SC_SGL_INVALID_COUNT:
>> +    case NVME_SC_SGL_INVALID_DATA:
>> +    case NVME_SC_SGL_INVALID_METADATA:
>> +    case NVME_SC_SGL_INVALID_TYPE:
>> +    case NVME_SC_SGL_INVALID_OFFSET:
>> +    case NVME_SC_SGL_INVALID_SUBTYPE:
>> +    case NVME_SC_CQ_INVALID:
>> +    case NVME_SC_QID_INVALID:
>> +    case NVME_SC_QUEUE_SIZE:
>> +    case NVME_SC_FIRMWARE_SLOT:
>> +    case NVME_SC_FIRMWARE_IMAGE:
>> +    case NVME_SC_INVALID_VECTOR:
>> +    case NVME_SC_INVALID_LOG_PAGE:
>> +    case NVME_SC_INVALID_FORMAT:
>> +    case NVME_SC_CTRL_LIST_INVALID:
> case NVME_SC_BAD_ATTRIBUTES:
> case NVME_SC_INVALID_PI:
> case NVME_SC_CONNECT_FORMAT:
> case NVME_SC_CONNECT_INVALID_PARAM:
> case NVME_SC_CONNECT_INVALID_HOST:
> 
>> +        return -EINVAL;
>> +    case NVME_SC_CMD_SEQ_ERROR:
>> +        return -EPROTO;
>>      default:
>>          return -EIO;
>>      }
>>
> 
> It'll also be useful if we can log the specific error (as a human
> readable string).
> 
> .
> 

[PATCH] nvme: make nvme error status codes converted to errno detailed

[Christoph Hellwig]

On Wed, Mar 29, 2017@06:08:23PM +0800, Guan Junxiong wrote:
> From: Junxiong Guan <guanjunxiong at huawei.com>
> 
> For more detailed information about nvme error status when ending
> blk_mq request,some of nvme error status codes can be categorized into
> different errnos explicitly. For example, NVME_SC_ACCESS_DENIED and
> NVME_SC_CONNECT_CTRL_BUSY can be converted to EACCESS and EBUSY repec-
> tively.This patch makes conversion from those nvme error status to errno
> detailed.

NAK.  Most of these codes arw not defined for the block layer and don't
make any sense.  Also I have a major rework in this area pending that
would conflict with this patch.

[PATCH] nvme: make nvme error status codes converted to errno detailed

[guanjunxiong]

On 2017/3/30 16:11, Christoph Hellwig wrote:
> On Wed, Mar 29, 2017@06:08:23PM +0800, Guan Junxiong wrote:
>> From: Junxiong Guan <guanjunxiong at huawei.com>
>>
>> For more detailed information about nvme error status when ending
>> blk_mq request,some of nvme error status codes can be categorized into
>> different errnos explicitly. For example, NVME_SC_ACCESS_DENIED and
>> NVME_SC_CONNECT_CTRL_BUSY can be converted to EACCESS and EBUSY repec-
>> tively.This patch makes conversion from those nvme error status to errno
>> detailed.
> 
> NAK.  Most of these codes arw not defined for the block layer and don't
> make any sense.  Also I have a major rework in this area pending that
> would conflict with this patch.
> 
> .
> 
Hi,Christoph
   Current block layer ends the requests with errors and logs different errno in a
human readable string. So I think the detailed conversion above make sense.Also ,
as Sagi said, It'll be useful if we can log the specific error (as a human readable
string).
   Anyway, since this patch would conflict with your rework in this area,
I decide to suspend this patch.
   BTW, what's the plan of your rework in this area?

[PATCH] nvme: make nvme error status codes converted to errno detailed

[Christoph Hellwig]

On Thu, Mar 30, 2017@01:22:37PM -0400, Keith Busch wrote:
> I don't see a problem having more appropriate errno's here. The block
> layer looks okay to accept any value, and the spec folks might be happy
> to see their thought out status codes don't all become EIO. :)

The block layer accepts all and treats a few magically.  That's why I
want to get rid of errno values in the block layer entirely and replace
the with a few status codes that make the special casing obvious.

> But just so we all know, the majority of those statuses are only possible
> to encounter with passthrough, and those are not translated to errno.

Exactly.

[PATCH] nvme: make nvme error status codes converted to errno detailed

[Keith Busch]

On Thu, Mar 30, 2017@07:32:50PM +0800, guanjunxiong wrote:
>    Current block layer ends the requests with errors and logs different errno in a
> human readable string. So I think the detailed conversion above make sense.Also ,
> as Sagi said, It'll be useful if we can log the specific error (as a human readable
> string).
>    Anyway, since this patch would conflict with your rework in this area,
> I decide to suspend this patch.
>    BTW, what's the plan of your rework in this area?

I don't see a problem having more appropriate errno's here. The block
layer looks okay to accept any value, and the spec folks might be happy
to see their thought out status codes don't all become EIO. :)

But just so we all know, the majority of those statuses are only possible
to encounter with passthrough, and those are not translated to errno.