On 2020/8/20 12:33, Sagi Grimberg wrote:
nvme_revalidate_disk translate return error to 0 if it is not a fatal
error, thus avoid false remove namespace. If return error less than 0,
now only ENOMEM be translated to 0, but other error except ENODEV,
such as EAGAIN or EBUSY etc, also need translate to 0.
Another reason for improving the error translation: If request timeout
when connect, __nvme_submit_sync_cmd will return
NVME_SC_HOST_ABORTED_CMD(>0). At this time, should terminate the
connect process, but falsely continue the connect process,
this may cause deadlock. Many functions which call
__nvme_submit_sync_cmd treat error code(> 0) as target not support and
continue, but NVME_SC_HOST_ABORTED_CMD and NVME_SC_HOST_PATH_ERROR both
are cancled io by host, to fix this bug, we need set the flag:
NVME_REQ_CANCELLED, thus __nvme_submit_sync_cmd will translate return
error to INTR. This is conflict with error translation of
nvme_revalidate_disk, may cause false remove namespace.
Signed-off-by: Chao Leng <lengchao@xxxxxxxxxx>
---
drivers/nvme/host/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 88cff309d8e4..43ac8a1ad65d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2130,10 +2130,10 @@ static int _nvme_revalidate_disk(struct gendisk *disk)
* Only fail the function if we got a fatal error back from the
* device, otherwise ignore the error and just move on.
*/
- if (ret == -ENOMEM || (ret > 0 && !(ret & NVME_SC_DNR)))
- ret = 0;
- else if (ret > 0)
+ if (ret > 0 && (ret & NVME_SC_DNR))
ret = blk_status_to_errno(nvme_error_status(ret));
+ else if (ret != -ENODEV)
+ ret = 0;
return ret;
We really need to take a step back here, I really don't like how
we are growing implicit assumptions on how statuses are interpreted.
agree.
Why don't we remove the -ENODEV error propagation back and instead
take care of it in the specific call-sites where we want to ignore
errors with proper quirks?
Maybe we can do like this.
---
drivers/nvme/host/core.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 88cff309d8e4..dc02208ff655 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2104,7 +2104,7 @@ static int _nvme_revalidate_disk(struct gendisk *disk)
ret = nvme_identify_ns(ctrl, ns->head->ns_id, &id);
if (ret)
- goto out;
+ return 0;
if (id->ncap == 0) {
ret = -ENODEV;
@@ -2112,8 +2112,10 @@ static int _nvme_revalidate_disk(struct gendisk *disk)
}
ret = nvme_report_ns_ids(ctrl, ns->head->ns_id, id, &ids);
- if (ret)
+ if (ret) {
+ ret = 0;
goto free_id;
+ }
if (!nvme_ns_ids_equal(&ns->head->ids, &ids)) {
dev_err(ctrl->device,
@@ -2125,15 +2127,6 @@ static int _nvme_revalidate_disk(struct gendisk *disk)
ret = __nvme_revalidate_disk(disk, id);
free_id:
kfree(id);
-out:
- /*
- * Only fail the function if we got a fatal error back from the
- * device, otherwise ignore the error and just move on.
- */
- if (ret == -ENOMEM || (ret > 0 && !(ret & NVME_SC_DNR)))
- ret = 0;
- else if (ret > 0)
- ret = blk_status_to_errno(nvme_error_status(ret));
return ret;
}
--
