[PATCH] nvme-core: optimize process for error status NVME_SC_CMD_INTERRUPTED

[PATCH] nvme-core: optimize process for error status NVME_SC_CMD_INTERRUPTED

[Chao Leng]

The protocol NVM-Express-1.4 define:
Command Interrupted: Command processing was interrupted and the
controller is unable to successfully complete the command. The host
should retry the command. If this status code is returned, then
the controller shall clear the Do Not Retry bit to ‘0’ in the Status
field of the CQE (refer to Figure 124). The controller shall not return
this status code unless the host has set the Advanced Command Retry
Enable (ACRE) field to 1h in the Host Behavior Support feature(refer to
section 5.21.1.22).

According the protocol define, NVME_SC_CMD_INTERRUPTED need retry.
The error code NVME_SC_CMD_INTERRUPTED should not translate to
BLK_STS_TARGET, because if the error code translate to BLK_STS_TARGET,
dm-multipah will return error to application. NVME_SC_CMD_INTERRUPTED
should translate to BLK_STS_IOERR by default, dm-multipath will fail
over to other path retry the io.

We need a mechanism for retry in the current path first in some
scenarios such as the target is dealing with an important emergency and
need host retry the io after a defined time, because dm-multipath can
not get Command Retry Delay (CRD) to control retry delay time.
according the protocol define, the error code NVME_SC_CMD_INTERRUPTED
is a good choice.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 drivers/nvme/host/core.c | 3 +--
 drivers/nvme/host/nvme.h | 6 ++++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 66e6f7ca8947..b3307e317ece 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -211,7 +211,6 @@ static blk_status_t nvme_error_status(u16 status)
 	case NVME_SC_CAP_EXCEEDED:
 		return BLK_STS_NOSPC;
 	case NVME_SC_LBA_RANGE:
-	case NVME_SC_CMD_INTERRUPTED:
 	case NVME_SC_NS_NOT_READY:
 		return BLK_STS_TARGET;
 	case NVME_SC_BAD_ATTRIBUTES:
@@ -243,7 +242,7 @@ static blk_status_t nvme_error_status(u16 status)
 
 static inline bool nvme_req_needs_retry(struct request *req)
 {
-	if (blk_noretry_request(req))
+	if (!nvme_req_local_retry(req) && blk_noretry_request(req))
 		return false;
 	if (nvme_req(req)->status & NVME_SC_DNR)
 		return false;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1de3f9b827aa..3213aae8985c 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -725,4 +725,10 @@ void nvme_hwmon_init(struct nvme_ctrl *ctrl);
 static inline void nvme_hwmon_init(struct nvme_ctrl *ctrl) { }
 #endif
 
+static inline bool nvme_req_local_retry(struct request *req)
+{
+	if (nvme_req(req)->status == NVME_SC_CMD_INTERRUPTED)
+		return true;
+	return false;
+}
 #endif /* _NVME_H */
-- 
2.16.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-core: optimize process for error status NVME_SC_CMD_INTERRUPTED

[Sagi Grimberg]

> @@ -243,7 +242,7 @@ static blk_status_t nvme_error_status(u16 status)
>   
>   static inline bool nvme_req_needs_retry(struct request *req)
>   {
> -	if (blk_noretry_request(req))
> +	if (!nvme_req_local_retry(req) && blk_noretry_request(req))

Why is this needed?

>   		return false;
>   	if (nvme_req(req)->status & NVME_SC_DNR)
>   		return false;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 1de3f9b827aa..3213aae8985c 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -725,4 +725,10 @@ void nvme_hwmon_init(struct nvme_ctrl *ctrl);
>   static inline void nvme_hwmon_init(struct nvme_ctrl *ctrl) { }
>   #endif
>   
> +static inline bool nvme_req_local_retry(struct request *req)
> +{
> +	if (nvme_req(req)->status == NVME_SC_CMD_INTERRUPTED)
> +		return true;
> +	return false;
> +}
>   #endif /* _NVME_H */
> 

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-core: optimize process for error status NVME_SC_CMD_INTERRUPTED

[Chao Leng]

On 2020/7/25 6:01, Sagi Grimberg wrote:
>
>> @@ -243,7 +242,7 @@ static blk_status_t nvme_error_status(u16 status)
>>     static inline bool nvme_req_needs_retry(struct request *req)
>>   {
>> -    if (blk_noretry_request(req))
>> +    if (!nvme_req_local_retry(req) && blk_noretry_request(req))
>
> Why is this needed?

Because if work with dm-multipath or other multipath software,
the multipath software will set cmd_flags as REQ_FAILFAST_TRANSPORT.
If io return with any error, nvme will return io error to block layer,
the multipath will set the path fault and retry in other path,
but realy need retry in the current path in some scenarios.
So we need introduce nvme_req_local_retry to check if need retry
in the current path, if needed, do not need checking the cmd_flags
through blk_noretry_request. According the protocol define,
the error code NVME_SC_CMD_INTERRUPTED is a good choice.

The scenario which need retry in the current path:
Scenario 1: user configures a QoS policy on the storage system.
Storage system need to tell host to retry io after a defined time
when need control QoS. Host should retry the io in the current path
instead of return io to block layer.

Scenario 3:many hosts(more than 100) access the same storage device.
I/Os are not delivered at the same time in most cases,however a large
number of I/Os may be burst delivered at the same time sometimes.
In this case, the storage device can not treat all I/Os, needs host
retry in the current path after a defined time.

Scenario 3: storage software upgrade inline. The storage system may need
host retry in the current path after software upgrade completed.


>
>>           return false;
>>       if (nvme_req(req)->status & NVME_SC_DNR)
>>           return false;
>> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>> index 1de3f9b827aa..3213aae8985c 100644
>> --- a/drivers/nvme/host/nvme.h
>> +++ b/drivers/nvme/host/nvme.h
>> @@ -725,4 +725,10 @@ void nvme_hwmon_init(struct nvme_ctrl *ctrl);
>>   static inline void nvme_hwmon_init(struct nvme_ctrl *ctrl) { }
>>   #endif
>>   +static inline bool nvme_req_local_retry(struct request *req)
>> +{
>> +    if (nvme_req(req)->status == NVME_SC_CMD_INTERRUPTED)
>> +        return true;
>> +    return false;
>> +}
>>   #endif /* _NVME_H */
>>

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme