[PATCH] nvme: fix REQ_NVME_MPATH retry logic

[PATCH] nvme: fix REQ_NVME_MPATH retry logic

[Meneghini, John]

From: John Meneghini <johnm@netapp.com>

 - Make nvme_complete_rq error handling logic
   equivalent between single and multipath controllers.
 - Non-multipathing related nvme errors will
   be retried on the same controller.
 - Avoid gratuitous controller reset while
   handling BLK_STS_IOERR when REQ_NVME_MPATH is set.

Signed-off-by: John Meneghini <johnm@netapp.com>
---
 drivers/nvme/host/core.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9696404a6182..4d1d44597a40 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -262,6 +262,25 @@ static void nvme_retry_req(struct request *req)
        blk_mq_delay_kick_requeue_list(req->q, delay);
 }
 
+static bool nvme_path_error(struct request *req, blk_status_t status)
+{
+
+       if (blk_path_error(status)) {
+               switch (nvme_req(req)->status & 0x7ff) {
+               case NVME_SC_ANA_TRANSITION:
+               case NVME_SC_ANA_INACCESSIBLE:
+               case NVME_SC_ANA_PERSISTENT_LOSS:
+               case NVME_SC_HOST_PATH_ERROR:
+               case NVME_SC_HOST_ABORTED_CMD:
+                       return true;
+               default:
+                       return false;
+               }
+       }
+
+       return false;
+}
+
 void nvme_complete_rq(struct request *req)
 {
        blk_status_t status = nvme_error_status(nvme_req(req)->status);
@@ -275,7 +294,7 @@ void nvme_complete_rq(struct request *req)
 
        if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) {
                if ((req->cmd_flags & REQ_NVME_MPATH) &&
-                   blk_path_error(status)) {
+                   nvme_path_error(req, status)) {
                        nvme_failover_req(req);
                        return;
                }
-- 
2.21.0

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

Re: [PATCH] nvme: fix REQ_NVME_MPATH retry logic

[Meneghini, John]

See my comments about this problem in:

http://lists.infradead.org/pipermail/linux-nvme/2019-December/028447.html

Keith, if this is at all interesting, let me know and I'll test it out.

Note: I left this in here to maintain backwards compatibility.  

    +       if (blk_path_error(status)) {

/John

On 12/13/19, 2:38 AM, "Meneghini, John" <John.Meneghini@netapp.com> wrote:

    From: John Meneghini <johnm@netapp.com>
    
     - Make nvme_complete_rq error handling logic
       equivalent between single and multipath controllers.
     - Non-multipathing related nvme errors will
       be retried on the same controller.
     - Avoid gratuitous controller reset while
       handling BLK_STS_IOERR when REQ_NVME_MPATH is set.
    
    Signed-off-by: John Meneghini <johnm@netapp.com>
    ---
     drivers/nvme/host/core.c | 21 ++++++++++++++++++++-
     1 file changed, 20 insertions(+), 1 deletion(-)
    
    diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
    index 9696404a6182..4d1d44597a40 100644
    --- a/drivers/nvme/host/core.c
    +++ b/drivers/nvme/host/core.c
    @@ -262,6 +262,25 @@ static void nvme_retry_req(struct request *req)
            blk_mq_delay_kick_requeue_list(req->q, delay);
     }
     
    +static bool nvme_path_error(struct request *req, blk_status_t status)
    +{
    +
    +       if (blk_path_error(status)) {
    +               switch (nvme_req(req)->status & 0x7ff) {
    +               case NVME_SC_ANA_TRANSITION:
    +               case NVME_SC_ANA_INACCESSIBLE:
    +               case NVME_SC_ANA_PERSISTENT_LOSS:
    +               case NVME_SC_HOST_PATH_ERROR:
    +               case NVME_SC_HOST_ABORTED_CMD:
    +                       return true;
    +               default:
    +                       return false;
    +               }
    +       }
    +
    +       return false;
    +}
    +
     void nvme_complete_rq(struct request *req)
     {
            blk_status_t status = nvme_error_status(nvme_req(req)->status);
    @@ -275,7 +294,7 @@ void nvme_complete_rq(struct request *req)
     
            if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) {
                    if ((req->cmd_flags & REQ_NVME_MPATH) &&
    -                   blk_path_error(status)) {
    +                   nvme_path_error(req, status)) {
                            nvme_failover_req(req);
                            return;
                    }
    -- 
    2.21.0
        

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

Re: [PATCH] nvme: fix REQ_NVME_MPATH retry logic

[Sagi Grimberg]

> From: John Meneghini <johnm@netapp.com>
> 
>   - Make nvme_complete_rq error handling logic
>     equivalent between single and multipath controllers.

The commit message should explain what breaks and what this
is fixing.

>   - Non-multipathing related nvme errors will
>     be retried on the same controller.

Any specific reason why we shouldn't? Or in other words,
what is the harm of doing it if we have another path?

>   - Avoid gratuitous controller reset while
>     handling BLK_STS_IOERR when REQ_NVME_MPATH is set.

This should change nvme_failover_req, not change the behavior by not
calling it. Requires an explanation what is broken with the existing
behavior.

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