[PATCH V2 0/2] nvme: fixes for command retry

[PATCH V2 0/2] nvme: fixes for command retry

[Minwoo Im]

Hello,

This patch series is V2 for Advanced Command Retry Enable(ACRE) feature.

The first patch checks all CRDT(Command Retry Delay Time) [0..2] to
figure out whether CRDT is configured or not.

The other patch has been updated to take internal flag in the controller
instance instead of the module parameter which has been posted in the
previous series [1].  This is based on the review by Chao: Thanks Chao!

Please review.

Thanks,

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)

[1] https://lore.kernel.org/linux-nvme/20210108144659.7126-1-minwoo.im.dev@gmail.com/T/#t

Minwoo Im (2):
  nvme: check all retry delay times in Identify Controller
  nvme: retry commands based on ACRE result

 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 V2 1/2] 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 V2 2/2] nvme: retry commands based on ACRE result

[Minwoo Im]

Introduce acre flag for Advanced Command Retry Enable to controller
instance to decide whether to retry or not in error cases.  This flag is
set if Set Features for Host Behavior Support with ACRE bit set is
successfully done during reset_work.

This patch also fixes nvme_decide_disposition() because all the nvme
requests are initialized with REQ_FAILFAST_DRIVER to req->cmd_flags so
that blk_noretry_request(req) is always true.  Check ctrl->acre first
and if it's not host path error, then it will be retried.

Cc: Chao Leng <lengchao@huawei.com>
Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 drivers/nvme/host/core.c | 16 ++++++++++++++--
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a8cee380b3c0..391bb2d08d0f 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) {
+		if (!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;
 }
 
@@ -2498,6 +2504,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 +2517,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

Re: [PATCH V2 2/2] nvme: retry commands based on ACRE result

[Sagi Grimberg]

> Introduce acre flag for Advanced Command Retry Enable to controller
> instance to decide whether to retry or not in error cases.  This flag is
> set if Set Features for Host Behavior Support with ACRE bit set is
> successfully done during reset_work.
> 
> This patch also fixes nvme_decide_disposition() because all the nvme
> requests are initialized with REQ_FAILFAST_DRIVER to req->cmd_flags

You mean for passthru commands, nvme doesn't control this for other
commands.

> so
> that blk_noretry_request(req) is always true.  Check ctrl->acre first
> and if it's not host path error, then it will be retried.

I suggest to split out this part to its own patch.

> 
> Cc: Chao Leng <lengchao@huawei.com>
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>   drivers/nvme/host/core.c | 16 ++++++++++++++--
>   drivers/nvme/host/nvme.h |  1 +
>   2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index a8cee380b3c0..391bb2d08d0f 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) {
> +		if (!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;
>   }
>   
> @@ -2498,6 +2504,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 +2517,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 V2 2/2] nvme: retry commands based on ACRE result

[Minwoo Im]

Hello,

Thanks for your time!

On 21-01-13 14:20:34, Sagi Grimberg wrote:
> > Introduce acre flag for Advanced Command Retry Enable to controller
> > instance to decide whether to retry or not in error cases.  This flag is
> > set if Set Features for Host Behavior Support with ACRE bit set is
> > successfully done during reset_work.
> > 
> > This patch also fixes nvme_decide_disposition() because all the nvme
> > requests are initialized with REQ_FAILFAST_DRIVER to req->cmd_flags
> 
> You mean for passthru commands, nvme doesn't control this for other
> commands.
> 

Oh, I forgot to mention... Thanks for catching this.

> > so
> > that blk_noretry_request(req) is always true.  Check ctrl->acre first
> > and if it's not host path error, then it will be retried.
> 
> I suggest to split out this part to its own patch.
> 

Okay.  I will split it up to two patches:

  - Introducing ACRE flag to controller
  - Check ACRE flag in nvme_decide_disposition()

Thanks!

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

Re: [PATCH V2 2/2] nvme: retry commands based on ACRE result

[Chao Leng]

On 2021/1/13 22:35, Minwoo Im wrote:
> Introduce acre flag for Advanced Command Retry Enable to controller
> instance to decide whether to retry or not in error cases.  This flag is
> set if Set Features for Host Behavior Support with ACRE bit set is
> successfully done during reset_work.
> 
> This patch also fixes nvme_decide_disposition() because all the nvme
> requests are initialized with REQ_FAILFAST_DRIVER to req->cmd_flags so
> that blk_noretry_request(req) is always true.  Check ctrl->acre first
> and if it's not host path error, then it will be retried.
> 
> Cc: Chao Leng <lengchao@huawei.com>
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>   drivers/nvme/host/core.c | 16 ++++++++++++++--
>   drivers/nvme/host/nvme.h |  1 +
>   2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index a8cee380b3c0..391bb2d08d0f 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) {
> +		if (!nvme_is_path_error(nvme_req(req)->status) &&
> +				!blk_queue_dying(req->q))
> +			return RETRY;
> +	} else if (blk_noretry_request(req))
> +		return COMPLETE;
If acre is true and nvme_req(req)->status is nvme path error and
blk_noretry_request(req) is true, we need return complete for REQ_FAILFAST_XX.
Otherwise, connection process may takes long time in abnormal scenarios.
We can do like this:

+    if (nvme_req(req)->ctrl->acre &&
+        !nvme_is_path_error(nvme_req(req)->status) &&
+        !blk_queue_dying(req->q))
+        return RETRY;
+
+    if (blk_noretry_request(req))
+        return COMPLETE;
> +
>   	return RETRY;
>   }
>   
> @@ -2498,6 +2504,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 +2517,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 V2 2/2] nvme: retry commands based on ACRE result

[Keith Busch]

On Wed, Jan 13, 2021 at 11:35:38PM +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) {
> +		if (!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;
>  }

This is returning RETRY for path related errors when blk_notry_request()
is true. That will deadlock initialization on a command timeout. The
check should be something like this:

	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;



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

Re: [PATCH V2 2/2] nvme: retry commands based on ACRE result

[Minwoo Im]

Hello,

> If acre is true and nvme_req(req)->status is nvme path error and
> blk_noretry_request(req) is true, we need return complete for REQ_FAILFAST_XX.
> Otherwise, connection process may takes long time in abnormal scenarios.
> We can do like this:
> 
> +    if (nvme_req(req)->ctrl->acre &&
> +        !nvme_is_path_error(nvme_req(req)->status) &&
> +        !blk_queue_dying(req->q))
> +        return RETRY;
> +
> +    if (blk_noretry_request(req))
> +        return COMPLETE;

Ah.. You're right.. I really appreciate for your review!
I will prepare the next patches.

Thank you!

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

Re: [PATCH V2 2/2] nvme: retry commands based on ACRE result

[Minwoo Im]

Hello Keith,

On 21-01-13 19:11:02, Keith Busch wrote:
> On Wed, Jan 13, 2021 at 11:35:38PM +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) {
> > +		if (!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;
> >  }
> 
> This is returning RETRY for path related errors when blk_notry_request()
> is true. That will deadlock initialization on a command timeout. The
> check should be something like this:
> 
> 	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;
> 
> 

Thank you for chatching this!  As Chao pointed it out, I will prepare
the next version of this patch.  I really appreciate it.

Thanks!

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