Re: [PATCH] nvme: Translate more status codes to blk_status_t

[Sagi Grimberg <sagi@grimberg.me>]

>      Let me test this out and I’ll see what happens.
>      
> Keith, I've tested this out, using CRD with both NVME_SC_CMD_INTERRUPTED and NVME_SC_NS_NOT_READY.
> 
> It works well enough, but I think the problem goes a little deeper than this.
> 
>> These are not generic IO errors and should use a non-path
>>   specific error so that it can use the non-failover retry path.
> 
> Yes, agreed.  But we have this problem with every/any other NVMe status that gets returned as well.
> It doesn't make sense to just keep overloading the half a dozen errors you have in blk_path_error();
> 
> I think the real problem is here:
> 
>   276         if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) {
>   277                 if ((req->cmd_flags & REQ_NVME_MPATH) &&
>   278                     blk_path_error(status)) {
>   279                         nvme_failover_req(req);
>   280                         return;
>   281                 }
> "nvme/drivers/nvme/host/core.c" line 281 of 4267 --6%-- col 3-17
> 
> If we are really not allowed to change the blk_path_error() routine because it's a part of
> the block layer, then why do we have it stuck in the middle of our multipathing policy
> logic?
> 
> Maybe we should create an nvme_path_error() function to replace the blk_path_error()
> function here.
> 
> The other problem is: setting REQ_NVME_MPATH completely changes the error
> error handling logic.  If my controller has a single path it happily returns all kinds
> of NVMe errors not handled by the nvme_error_status() white list.  Those
> errors all fall through your retry logic and end up returning  BLK_STS_IOERR.
> 
> However, as soon as we add another path to that same controller, and turn on
> REQ_NVME_MPATH, all of a sudden the controller gets a reset for returning
> the very same errors that it retuned before.

I agree we should lose this controller reset and only do this for
specific error cases where its needed (not thinking of any from the
top of my head).

> And that happens before even a single retry is attempted - unless it's an NVMe pathing error.
> 
> 105         default:
> 106                 /*
> 107                  * Reset the controller for any non-ANA error as we don't know
> 108                  * what caused the error.
> 109                  */
> 110                 nvme_reset_ctrl(ns->ctrl);
> 111                 break;
> 112         }
> "nvme/drivers/nvme/host/multipath.c" line 112 of 739 --15%-- col 1-8
> 
> This makes no sense.

So lets remove this reset. Have the transport take care of this, or do
it only when it is clearly needed.

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