[PATCH 2/3] nvme-core: delete the dependency on blk status

[PATCH 2/3] nvme-core: delete the dependency on blk status

[Chao Leng]

nvme should not depend on blk status, just need check nvme status.
Just need do translating nvme status to blk status for returning error.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 52d19a4d3bc8..246988091c05 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -278,7 +278,7 @@ static void nvme_retry_req(struct request *req)
 
 void nvme_complete_rq(struct request *req)
 {
-	blk_status_t status = nvme_error_status(nvme_req(req)->status);
+	blk_status_t status;
 
 	trace_nvme_complete_rq(req);
 
@@ -287,7 +287,8 @@ void nvme_complete_rq(struct request *req)
 	if (nvme_req(req)->ctrl->kas)
 		nvme_req(req)->ctrl->comp_seen = true;
 
-	if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) {
+	if (unlikely(nvme_req(req)->status != NVME_SC_SUCCESS &&
+		nvme_req_needs_retry(req))) {
 		if (nvme_req_path_error(req)) {
 			if (req->cmd_flags & REQ_NVME_MPATH) {
 				nvme_failover_req(req);
@@ -299,6 +300,7 @@ void nvme_complete_rq(struct request *req)
 		}
 	}
 
+	status = nvme_error_status(nvme_req(req)->status);
 	nvme_trace_bio_complete(req, status);
 	blk_mq_end_request(req, status);
 }
-- 
2.16.4

Re: [PATCH 2/3] nvme-core: delete the dependency on blk status

[Christoph Hellwig]

On Wed, Aug 12, 2020 at 04:18:44PM +0800, Chao Leng wrote:
> nvme should not depend on blk status, just need check nvme status.
> Just need do translating nvme status to blk status for returning error.

While this doesn't look wrong it also doesn't save us a single
instruction and actually adds more lines of code.  Do you have a good
reason for this change?

Re: [PATCH 2/3] nvme-core: delete the dependency on blk status

[Mike Snitzer]

On Wed, Aug 12 2020 at 11:10am -0400,
Christoph Hellwig <hch@lst.de> wrote:

> On Wed, Aug 12, 2020 at 04:18:44PM +0800, Chao Leng wrote:
> > nvme should not depend on blk status, just need check nvme status.
> > Just need do translating nvme status to blk status for returning error.
> 
> While this doesn't look wrong it also doesn't save us a single
> instruction and actually adds more lines of code.  Do you have a good
> reason for this change?

It certainly saves nvme_error_status(nvme_req(req)->status if
nvme_req_needs_retry().

Re: [PATCH 2/3] nvme-core: delete the dependency on blk status

[Chao Leng]

On 2020/8/12 23:10, Christoph Hellwig wrote:
> On Wed, Aug 12, 2020 at 04:18:44PM +0800, Chao Leng wrote:
>> nvme should not depend on blk status, just need check nvme status.
>> Just need do translating nvme status to blk status for returning error.
> 
> While this doesn't look wrong it also doesn't save us a single
> instruction and actually adds more lines of code.  Do you have a good
> reason for this change?

If need retry, save nvme_error_status. Of course, it doesn't matter.
There's no logical change, just want nvme do not care blk status when
check retry.
It is ok to change or not.

Re: [PATCH 2/3] nvme-core: delete the dependency on blk status

[Christoph Hellwig]

On Wed, Aug 12, 2020 at 04:28:30PM -0400, Mike Snitzer wrote:
> On Wed, Aug 12 2020 at 11:10am -0400,
> Christoph Hellwig <hch@lst.de> wrote:
> 
> > On Wed, Aug 12, 2020 at 04:18:44PM +0800, Chao Leng wrote:
> > > nvme should not depend on blk status, just need check nvme status.
> > > Just need do translating nvme status to blk status for returning error.
> > 
> > While this doesn't look wrong it also doesn't save us a single
> > instruction and actually adds more lines of code.  Do you have a good
> > reason for this change?
> 
> It certainly saves nvme_error_status(nvme_req(req)->status if
> nvme_req_needs_retry().

Yeah, but the retry path isn't exactly the fast path, is it?

Not that I really care too much about this one..