[PATCH V3 0/3] nvme: fixes for command retry

[PATCH V3 0/3] nvme: fixes for command retry

[Minwoo Im]

Hello,

Here's V3 patch series for Advanced Command Retery Enable (ACRE).  This
series enables command retry feature based on a flag introduced to nvme
controller instance in this series.

This patchset placed ACRE flag check first than REQ_FAILFAST_* cmd_flags
checking.  It makes Set Feature for Host Bevior in reset_work more
meaningful.

Please review.

Thanks!

Since V2:
  - Update commit description more informative. (Sagi)
  - Split [2/2] patch into two patches. (Sagi)
  - Fix condition to return COMPLETE in [2/2]. (Chao and Keith)

Since V1:
  - The first patch has been applied (Christoph).  Removed from this
    series.
  - Update the last patch to take acre flag from the controller
    instance instead of module parameter by updating
    nvme_decide_disposition().  (Chao)

Minwoo Im (3):
  nvme: check all retry delay times in Identify Controller
  nvme: introduce acre flag in nvme_ctrl
  nvme: retry commands based on ACRE flag

 drivers/nvme/host/core.c | 18 +++++++++++++++---
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

-- 
2.17.1


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

[PATCH V3 1/3] nvme: check all retry delay times in Identify Controller

[Minwoo Im]

If none of Command Retry Delay Time(CRDT)s are set, then we can simply
skip to configure Advanced Command Retry Enable(ACRE).

CRDT selection will be made in Completion Queue Entry that controller
has responsibility to decide.

This patch checks all CRDT[0, 1, and 2] in Identify Controller data
structrure instead of checking the first one: CRDT[0] for cases that:

	- CRDT[0] == 0, but CRDT[1 or 2] != 0

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index fff49e544fdf..a8cee380b3c0 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2499,7 +2499,7 @@ static int nvme_configure_acre(struct nvme_ctrl *ctrl)
 	int ret;
 
 	/* Don't bother enabling the feature if retry delay is not reported */
-	if (!ctrl->crdt[0])
+	if (!ctrl->crdt[0] && !ctrl->crdt[1] && !ctrl->crdt[2])
 		return 0;
 
 	host = kzalloc(sizeof(*host), GFP_KERNEL);
-- 
2.17.1


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

[PATCH V3 2/3] nvme: introduce acre flag in nvme_ctrl

[Minwoo Im]

Advanced Command Retry Enable (ACRE) flag is added in nvme_ctrl
instance to indicate that Set Features for Host Behavior Support with
ACRE enabled is whether successfully done or not during reset_work.

This flag will be used to decide to retry commands that fail or not in
the followed patch.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 drivers/nvme/host/core.c | 6 ++++++
 drivers/nvme/host/nvme.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a8cee380b3c0..a286e3422c61 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2498,6 +2498,8 @@ static int nvme_configure_acre(struct nvme_ctrl *ctrl)
 	struct nvme_feat_host_behavior *host;
 	int ret;
 
+	ctrl->acre = false;
+
 	/* Don't bother enabling the feature if retry delay is not reported */
 	if (!ctrl->crdt[0] && !ctrl->crdt[1] && !ctrl->crdt[2])
 		return 0;
@@ -2509,6 +2511,10 @@ static int nvme_configure_acre(struct nvme_ctrl *ctrl)
 	host->acre = NVME_ENABLE_ACRE;
 	ret = nvme_set_features(ctrl, NVME_FEAT_HOST_BEHAVIOR, 0,
 				host, sizeof(*host), NULL);
+
+	if (!ret)
+		ctrl->acre = true;
+
 	kfree(host);
 	return ret;
 }
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 88a6b97247f5..db8b45e8ffde 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -270,6 +270,7 @@ struct nvme_ctrl {
 #ifdef CONFIG_BLK_DEV_ZONED
 	u32 max_zone_append;
 #endif
+	bool acre;
 	u16 crdt[3];
 	u16 oncs;
 	u16 oacs;
-- 
2.17.1


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

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

[Minwoo Im]

Allow nvme_decide_disposition() to retry commands with error by checking
Advanced Command Retry Enable (ACRE) flag of controller first than
REQ_FAILFAST_* cmd_flags.  But, we just can't allow all command failures
to be retried due to reasons like connecting may take too long due to
the retries.  This patch only allows non-host path error commands to be
retried.

Cc: Chao Leng <lengchao@huawei.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Keith Busch <kbusch@kernel.org>
Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 drivers/nvme/host/core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a286e3422c61..e4f7c49c2f39 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -303,8 +303,7 @@ static inline enum nvme_disposition nvme_decide_disposition(struct request *req)
 	if (likely(nvme_req(req)->status == 0))
 		return COMPLETE;
 
-	if (blk_noretry_request(req) ||
-	    (nvme_req(req)->status & NVME_SC_DNR) ||
+	if ((nvme_req(req)->status & NVME_SC_DNR) ||
 	    nvme_req(req)->retries >= nvme_max_retries)
 		return COMPLETE;
 
@@ -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;
+	else if (blk_noretry_request(req))
+		return COMPLETE;
+
 	return RETRY;
 }
 
-- 
2.17.1


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

Re: [PATCH V3 2/3] nvme: introduce acre flag in nvme_ctrl

[Chao Leng]

Reviewed-by: Chao Leng <lengchao@huawei.com>

On 2021/1/14 21:31, Minwoo Im wrote:
> Advanced Command Retry Enable (ACRE) flag is added in nvme_ctrl
> instance to indicate that Set Features for Host Behavior Support with
> ACRE enabled is whether successfully done or not during reset_work.
> 
> This flag will be used to decide to retry commands that fail or not in
> the followed patch.
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>   drivers/nvme/host/core.c | 6 ++++++
>   drivers/nvme/host/nvme.h | 1 +
>   2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index a8cee380b3c0..a286e3422c61 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2498,6 +2498,8 @@ static int nvme_configure_acre(struct nvme_ctrl *ctrl)
>   	struct nvme_feat_host_behavior *host;
>   	int ret;
>   
> +	ctrl->acre = false;
> +
>   	/* Don't bother enabling the feature if retry delay is not reported */
>   	if (!ctrl->crdt[0] && !ctrl->crdt[1] && !ctrl->crdt[2])
>   		return 0;
> @@ -2509,6 +2511,10 @@ static int nvme_configure_acre(struct nvme_ctrl *ctrl)
>   	host->acre = NVME_ENABLE_ACRE;
>   	ret = nvme_set_features(ctrl, NVME_FEAT_HOST_BEHAVIOR, 0,
>   				host, sizeof(*host), NULL);
> +
> +	if (!ret)
> +		ctrl->acre = true;
> +
>   	kfree(host);
>   	return ret;
>   }
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 88a6b97247f5..db8b45e8ffde 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -270,6 +270,7 @@ struct nvme_ctrl {
>   #ifdef CONFIG_BLK_DEV_ZONED
>   	u32 max_zone_append;
>   #endif
> +	bool acre;
>   	u16 crdt[3];
>   	u16 oncs;
>   	u16 oacs;
> 

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

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

[Chao Leng]

Reviewed-by: Chao Leng <lengchao@huawei.com>

On 2021/1/14 21:31, Minwoo Im wrote:
> Allow nvme_decide_disposition() to retry commands with error by checking
> Advanced Command Retry Enable (ACRE) flag of controller first than
> REQ_FAILFAST_* cmd_flags.  But, we just can't allow all command failures
> to be retried due to reasons like connecting may take too long due to
> the retries.  This patch only allows non-host path error commands to be
> retried.
> 
> Cc: Chao Leng <lengchao@huawei.com>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Keith Busch <kbusch@kernel.org>
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>   drivers/nvme/host/core.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index a286e3422c61..e4f7c49c2f39 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -303,8 +303,7 @@ static inline enum nvme_disposition nvme_decide_disposition(struct request *req)
>   	if (likely(nvme_req(req)->status == 0))
>   		return COMPLETE;
>   
> -	if (blk_noretry_request(req) ||
> -	    (nvme_req(req)->status & NVME_SC_DNR) ||
> +	if ((nvme_req(req)->status & NVME_SC_DNR) ||
>   	    nvme_req(req)->retries >= nvme_max_retries)
>   		return COMPLETE;
>   
> @@ -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;
> +	else if (blk_noretry_request(req))
> +		return COMPLETE;
> +
>   	return RETRY;
>   }
>   
> 

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

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

[Keith Busch]

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.

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

Re: [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,

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

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

[Christoph Hellwig]

On Sat, Jan 16, 2021 at 03:26:02AM +0900, Minwoo Im wrote:
> > 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,

We could come up with a version of the ioctls that use the normal
retry mechanisms.  nvme_passthru_cmd64 has two reserved fields we
could use for UAPI flags like this.

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

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

[Minwoo Im]

On 21-01-18 18:40:51, Christoph Hellwig wrote:
> On Sat, Jan 16, 2021 at 03:26:02AM +0900, Minwoo Im wrote:
> > > 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,
> 
> We could come up with a version of the ioctls that use the normal
> retry mechanisms.  nvme_passthru_cmd64 has two reserved fields we
> could use for UAPI flags like this.

Thank you for your feedback on this!

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

Re: [PATCH V3 1/3] nvme: check all retry delay times in Identify Controller

[Minwoo Im]

On 21-01-14 22:31:08, Minwoo Im wrote:
> If none of Command Retry Delay Time(CRDT)s are set, then we can simply
> skip to configure Advanced Command Retry Enable(ACRE).
> 
> CRDT selection will be made in Completion Queue Entry that controller
> has responsibility to decide.
> 
> This patch checks all CRDT[0, 1, and 2] in Identify Controller data
> structrure instead of checking the first one: CRDT[0] for cases that:
> 
> 	- CRDT[0] == 0, but CRDT[1 or 2] != 0
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>  drivers/nvme/host/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index fff49e544fdf..a8cee380b3c0 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2499,7 +2499,7 @@ static int nvme_configure_acre(struct nvme_ctrl *ctrl)
>  	int ret;
>  
>  	/* Don't bother enabling the feature if retry delay is not reported */
> -	if (!ctrl->crdt[0])
> +	if (!ctrl->crdt[0] && !ctrl->crdt[1] && !ctrl->crdt[2])
>  		return 0;
>  
>  	host = kzalloc(sizeof(*host), GFP_KERNEL);
> -- 
> 2.17.1
> 

Hello Christoph, and Keith,

May I get your feedback on this?  Is there any background reason not to
configure ACRE without checking CRDT2[1] and CRDT3[2] ?

Thanks!

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

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

[Keith Busch]

On Mon, Jan 18, 2021 at 06:40:51PM +0100, Christoph Hellwig wrote:
> On Sat, Jan 16, 2021 at 03:26:02AM +0900, Minwoo Im wrote:
> > > 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,
> 
> We could come up with a version of the ioctls that use the normal
> retry mechanisms.  nvme_passthru_cmd64 has two reserved fields we
> could use for UAPI flags like this.

Yeah, I'm totally okay with adding driver control flags for this sort of
behavior.

For the moment, the ioctl interface has always been 1:1 for commands
submitted to completions received, so all user space applications using
this should have been prepared to receive unfiltered status back and
handle retries on their own.

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

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

[Minwoo Im]

On 21-01-20 03:28:38, Keith Busch wrote:
> On Mon, Jan 18, 2021 at 06:40:51PM +0100, Christoph Hellwig wrote:
> > On Sat, Jan 16, 2021 at 03:26:02AM +0900, Minwoo Im wrote:
> > > > 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,
> > 
> > We could come up with a version of the ioctls that use the normal
> > retry mechanisms.  nvme_passthru_cmd64 has two reserved fields we
> > could use for UAPI flags like this.
> 
> Yeah, I'm totally okay with adding driver control flags for this sort of
> behavior.
> 
> For the moment, the ioctl interface has always been 1:1 for commands
> submitted to completions received, so all user space applications using
> this should have been prepared to receive unfiltered status back and
> handle retries on their own.

Thanks Keith for your feedback!

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