[PATCH V3 3/3] nvme: retry commands based on ACRE flag

[Minwoo Im]

On 21-01-15 09:04:12, Keith Busch wrote:
> On Thu, Jan 14, 2021 at 10:31:10PM +0900, Minwoo Im wrote:
> > @@ -317,6 +316,13 @@ static inline enum nvme_disposition nvme_decide_disposition(struct request *req)
> >  			return COMPLETE;
> >  	}
> >  
> > +	if (nvme_req(req)->ctrl->acre &&
> > +	    !nvme_is_path_error(nvme_req(req)->status) &&
> > +	    !blk_queue_dying(req->q))
> > +		return RETRY;
> 
> Is there really any reason to tie this to acre?
> 
> > +	else if (blk_noretry_request(req))
> > +		return COMPLETE;
> 
> I am not sure we should ignore the FAILFAST for non-path errors. If we
> need retryable admin commands, we should let the driver provide a way
> for callers to dispatch requests without that flag.

Understood.  I thought the opposite way about FAILFAST in case with
acre, if device is enabled with acre, all commands would be retried
regardless to FAILFAST...  Thanks for pointing that out!

How do you think which one is right choice to go with if a user-space
application(e.g., nvme-cli) wants a command to be retired in case of
ACRE && Error && !DNR:

  - User-space application should figure out !DNR and retry the command.
    (Maybe we are not able to easily figure out exact status code from
    the user-space application by the return value).

  - Driver should retry the command right before putting result up to
    the user-space even it's a FAILFAST request.

Thanks,