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

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

[Minwoo Im]

Hello,

This patchset fixes command retry feature in NVMe core driver.  The
first one supports command retry delay for admin commands also.
Earlier, we only supported I/O commands and this patch made it support
for admin commands too.  The second one fixed checks for whether to
enable Advanced Command Retry Enable(ACRE) or not by checking all
Command Retry Delay Time(CRDT)s from only a single one CRDT[0].  The
last one added a module parameter 'command_retry' to enable the command
retry feature in this driver.

Please review.

Thanks,

Minwoo Im (3):
  nvme: support command retry delay for admin command
  nvme: check all retry delay times in Identify Controller
  nvme: add parameter command_retry to enable retry

 drivers/nvme/host/core.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

-- 
2.17.1


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

[PATCH 1/3] nvme: support command retry delay for admin command

[Minwoo Im]

Once driver decides to retry a command that failed, it can have some
delay based on Command Retry Delay(CRD) in Completion Queue Entry.  If
CRD indicates Command Retry Delay Time 1, 2, or 3, driver calcualtes
delay in Identify Controller data structure.

This feature can be applied not only I/O queue commands, but also admin
queue commands.  Admin command during the initialization may fail due to
some un-ready state of a controller, so it would be better to have this
feature for admin commands also.

This patch simply removed getting namespace instance(struct nvme_ns) from
the struct request and get a controller instance(struct nvme_ctrl) by
nvme_req() helper.  Now, nvme_retry_req() calculates the delay
regardless to namespace instance which meant this request is for I/O
command.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ce1b61519441..d0c99a9400ea 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -280,14 +280,13 @@ static blk_status_t nvme_error_status(u16 status)
 
 static void nvme_retry_req(struct request *req)
 {
-	struct nvme_ns *ns = req->q->queuedata;
 	unsigned long delay = 0;
 	u16 crd;
 
 	/* The mask and shift result must be <= 3 */
 	crd = (nvme_req(req)->status & NVME_SC_CRD) >> 11;
-	if (ns && crd)
-		delay = ns->ctrl->crdt[crd - 1] * 100;
+	if (crd)
+		delay = nvme_req(req)->ctrl->crdt[crd - 1] * 100;
 
 	nvme_req(req)->retries++;
 	blk_mq_requeue_request(req, false);
-- 
2.17.1


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

Re: [PATCH 1/3] nvme: support command retry delay for admin command

[Chaitanya Kulkarni]

On 1/8/21 06:54, Minwoo Im wrote:
> This feature can be applied not only I/O queue commands, but also admin
> queue commands.  Admin command during the initialization may fail due to
> some un-ready state of a controller, so it would be better to have this
> feature for admin commands also.
What is some un-ready state ?

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

Re: [PATCH 1/3] nvme: support command retry delay for admin command

[Minwoo Im]

Hello,

On 21-01-09 20:34:11, Chaitanya Kulkarni wrote:
> On 1/8/21 06:54, Minwoo Im wrote:
> > This feature can be applied not only I/O queue commands, but also admin
> > queue commands.  Admin command during the initialization may fail due to
> > some un-ready state of a controller, so it would be better to have this
> > feature for admin commands also.
> What is some un-ready state ?

State that controller is unable to process successfully command as
"Command Interrupt" status code represents.

Thanks,

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

Re: [PATCH 1/3] nvme: support command retry delay for admin command

[Christoph Hellwig]

On Sun, Jan 10, 2021 at 10:33:16AM +0900, Minwoo Im wrote:
> Hello,
> 
> On 21-01-09 20:34:11, Chaitanya Kulkarni wrote:
> > On 1/8/21 06:54, Minwoo Im wrote:
> > > This feature can be applied not only I/O queue commands, but also admin
> > > queue commands.  Admin command during the initialization may fail due to
> > > some un-ready state of a controller, so it would be better to have this
> > > feature for admin commands also.
> > What is some un-ready state ?
> 
> State that controller is unable to process successfully command as
> "Command Interrupt" status code represents.

Given that controller ready has a specific meaning in NVMe I think a
different wording here would be useful.

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

Re: [PATCH 1/3] nvme: support command retry delay for admin command

[Christoph Hellwig]

Except for the little wording issue this looks good to me.  I'll pick it
up for nvme-5.11 and will fix up the commit log.

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

Re: [PATCH 1/3] nvme: support command retry delay for admin command

[Minwoo Im]

Hello,

On 21-01-12 09:06:53, Christoph Hellwig wrote:
> Except for the little wording issue this looks good to me.  I'll pick it
> up for nvme-5.11 and will fix up the commit log.

Thanks for the fix!

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

Re: [PATCH 1/3] nvme: support command retry delay for admin command

[Christoph Hellwig]

On Tue, Jan 12, 2021 at 09:06:53AM +0100, Christoph Hellwig wrote:
> Except for the little wording issue this looks good to me.  I'll pick it
> up for nvme-5.11 and will fix up the commit log.

Given that we never retry admin command right now I've moved this off 
nvme-5.11 into the pending queue.

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

Re: [PATCH 1/3] nvme: support command retry delay for admin command

[Minwoo Im]

On 21-01-14 08:22:54, Christoph Hellwig wrote:
> On Tue, Jan 12, 2021 at 09:06:53AM +0100, Christoph Hellwig wrote:
> > Except for the little wording issue this looks good to me.  I'll pick it
> > up for nvme-5.11 and will fix up the commit log.
> 
> Given that we never retry admin command right now I've moved this off 
> nvme-5.11 into the pending queue.

Thanks for sharing!

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

[PATCH 2/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].

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 d0c99a9400ea..6e428fdc25a8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2501,7 +2501,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

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

[Christoph Hellwig]

On Fri, Jan 08, 2021 at 11:46:58PM +0900, 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].

Why?

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

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

[Minwoo Im]

Hello,

On 21-01-12 09:08:40, Christoph Hellwig wrote:
> On Fri, Jan 08, 2021 at 11:46:58PM +0900, 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].
> 
> Why?

I thought we should check whether there's at least one CRDT fields are
set or not regardless to CRDT number (0, 1, and 2).  If CRDT[0] is not
set, but CRDT[1] is set, then we can go ahead with CRDT[1] not [0].

Thanks!

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

[PATCH 3/3] nvme: add parameter command_retry to enable retry

[Minwoo Im]

nvme_init_request() has set REQ_FAILFAST_DRIVER to make requests
non-retryable.  This command flag value is checked in
nvme_decide_disposition() to decide whether to RETRY or other
operations.  In that point, blk_noretry_request() macro will be used to
check if command flags have one of REQ_FAILFAST_*.  If so, it just
decides to complete the request without retrying.

This patch added a module parameter named command_retry to turn on the
command retry feature in this driver.  If turning it on,
REQ3_FAILFAST_DRIVER will not be set to requests so that retry can be
reached out to nvme_retry_req() based on the module parameter.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 6e428fdc25a8..e1836ca9956f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -44,6 +44,10 @@ static unsigned char shutdown_timeout = 5;
 module_param(shutdown_timeout, byte, 0644);
 MODULE_PARM_DESC(shutdown_timeout, "timeout in seconds for controller shutdown");
 
+static bool command_retry;
+module_param(command_retry, bool, 0644);
+MODULE_PARM_DESC(command_retry, "retry commands up to nvme_max_retries");
+
 static u8 nvme_max_retries = 5;
 module_param_named(max_retries, nvme_max_retries, byte, 0644);
 MODULE_PARM_DESC(max_retries, "max number of retries a command may have");
@@ -560,7 +564,8 @@ static inline void nvme_init_request(struct request *req,
 	else /* no queuedata implies admin queue */
 		req->timeout = NVME_ADMIN_TIMEOUT;
 
-	req->cmd_flags |= REQ_FAILFAST_DRIVER;
+	if (!command_retry)
+		req->cmd_flags |= REQ_FAILFAST_DRIVER;
 	nvme_clear_nvme_request(req);
 	nvme_req(req)->cmd = cmd;
 }
-- 
2.17.1


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

Re: [PATCH 3/3] nvme: add parameter command_retry to enable retry

[Chao Leng]

On 2021/1/8 22:46, Minwoo Im wrote:
> nvme_init_request() has set REQ_FAILFAST_DRIVER to make requests
> non-retryable.  This command flag value is checked in
> nvme_decide_disposition() to decide whether to RETRY or other
> operations.  In that point, blk_noretry_request() macro will be used to
> check if command flags have one of REQ_FAILFAST_*.  If so, it just
> decides to complete the request without retrying.
> 
> This patch added a module parameter named command_retry to turn on the
> command retry feature in this driver.  If turning it on,
> REQ3_FAILFAST_DRIVER will not be set to requests so that retry can be
> reached out to nvme_retry_req() based on the module parameter.
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>   drivers/nvme/host/core.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 6e428fdc25a8..e1836ca9956f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -44,6 +44,10 @@ static unsigned char shutdown_timeout = 5;
>   module_param(shutdown_timeout, byte, 0644);
>   MODULE_PARM_DESC(shutdown_timeout, "timeout in seconds for controller shutdown");
>   
> +static bool command_retry;
> +module_param(command_retry, bool, 0644);
> +MODULE_PARM_DESC(command_retry, "retry commands up to nvme_max_retries");
> +
>   static u8 nvme_max_retries = 5;
>   module_param_named(max_retries, nvme_max_retries, byte, 0644);
>   MODULE_PARM_DESC(max_retries, "max number of retries a command may have");
> @@ -560,7 +564,8 @@ static inline void nvme_init_request(struct request *req,
>   	else /* no queuedata implies admin queue */
>   		req->timeout = NVME_ADMIN_TIMEOUT;
>   
> -	req->cmd_flags |= REQ_FAILFAST_DRIVER;
> +	if (!command_retry)
> +		req->cmd_flags |= REQ_FAILFAST_DRIVER;
In abnormal scenarios, such as request time out, connection process may takes long time or the admin command waits for long time.
Retry only for non-host errors may be a better choice. Maybe we can make some optimizations in nvme_decide_disposition.
>   	nvme_clear_nvme_request(req);
>   	nvme_req(req)->cmd = cmd;
>   }
> 

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

Re: [PATCH 3/3] nvme: add parameter command_retry to enable retry

[Minwoo Im]

Hello,

On 21-01-11 11:47:56, Chao Leng wrote:
> 
> 
> On 2021/1/8 22:46, Minwoo Im wrote:
> > nvme_init_request() has set REQ_FAILFAST_DRIVER to make requests
> > non-retryable.  This command flag value is checked in
> > nvme_decide_disposition() to decide whether to RETRY or other
> > operations.  In that point, blk_noretry_request() macro will be used to
> > check if command flags have one of REQ_FAILFAST_*.  If so, it just
> > decides to complete the request without retrying.
> > 
> > This patch added a module parameter named command_retry to turn on the
> > command retry feature in this driver.  If turning it on,
> > REQ3_FAILFAST_DRIVER will not be set to requests so that retry can be
> > reached out to nvme_retry_req() based on the module parameter.
> > 
> > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> > ---
> >   drivers/nvme/host/core.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 6e428fdc25a8..e1836ca9956f 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -44,6 +44,10 @@ static unsigned char shutdown_timeout = 5;
> >   module_param(shutdown_timeout, byte, 0644);
> >   MODULE_PARM_DESC(shutdown_timeout, "timeout in seconds for controller shutdown");
> > +static bool command_retry;
> > +module_param(command_retry, bool, 0644);
> > +MODULE_PARM_DESC(command_retry, "retry commands up to nvme_max_retries");
> > +
> >   static u8 nvme_max_retries = 5;
> >   module_param_named(max_retries, nvme_max_retries, byte, 0644);
> >   MODULE_PARM_DESC(max_retries, "max number of retries a command may have");
> > @@ -560,7 +564,8 @@ static inline void nvme_init_request(struct request *req,
> >   	else /* no queuedata implies admin queue */
> >   		req->timeout = NVME_ADMIN_TIMEOUT;
> > -	req->cmd_flags |= REQ_FAILFAST_DRIVER;
> > +	if (!command_retry)
> > +		req->cmd_flags |= REQ_FAILFAST_DRIVER;
> In abnormal scenarios, such as request time out, connection process may takes long time or the admin command waits for long time.
> Retry only for non-host errors may be a better choice. Maybe we can make some optimizations in nvme_decide_disposition.

Thanks for your review!

Oh, I agreed that it might wait for so long time in connecting process.
Restricting some of commands that should be retried would be better as
you mentiond.

Do you mean that maybe we can check this module parameter in
nvme_decide_disposition()?  Like, even if blk_noretry_request(req) says
that it's non-retriable, if this module parameter is enabled, then we
can retry rather than failfast?

Thanks,

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

Re: [PATCH 3/3] nvme: add parameter command_retry to enable retry

[Chao Leng]

On 2021/1/11 20:27, Minwoo Im wrote:
> Hello,
> 
> On 21-01-11 11:47:56, Chao Leng wrote:
>>
>>
>> On 2021/1/8 22:46, Minwoo Im wrote:
>>> nvme_init_request() has set REQ_FAILFAST_DRIVER to make requests
>>> non-retryable.  This command flag value is checked in
>>> nvme_decide_disposition() to decide whether to RETRY or other
>>> operations.  In that point, blk_noretry_request() macro will be used to
>>> check if command flags have one of REQ_FAILFAST_*.  If so, it just
>>> decides to complete the request without retrying.
>>>
>>> This patch added a module parameter named command_retry to turn on the
>>> command retry feature in this driver.  If turning it on,
>>> REQ3_FAILFAST_DRIVER will not be set to requests so that retry can be
>>> reached out to nvme_retry_req() based on the module parameter.
>>>
>>> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
>>> ---
>>>    drivers/nvme/host/core.c | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index 6e428fdc25a8..e1836ca9956f 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -44,6 +44,10 @@ static unsigned char shutdown_timeout = 5;
>>>    module_param(shutdown_timeout, byte, 0644);
>>>    MODULE_PARM_DESC(shutdown_timeout, "timeout in seconds for controller shutdown");
>>> +static bool command_retry;
>>> +module_param(command_retry, bool, 0644);
>>> +MODULE_PARM_DESC(command_retry, "retry commands up to nvme_max_retries");
>>> +
>>>    static u8 nvme_max_retries = 5;
>>>    module_param_named(max_retries, nvme_max_retries, byte, 0644);
>>>    MODULE_PARM_DESC(max_retries, "max number of retries a command may have");
>>> @@ -560,7 +564,8 @@ static inline void nvme_init_request(struct request *req,
>>>    	else /* no queuedata implies admin queue */
>>>    		req->timeout = NVME_ADMIN_TIMEOUT;
>>> -	req->cmd_flags |= REQ_FAILFAST_DRIVER;
>>> +	if (!command_retry)
>>> +		req->cmd_flags |= REQ_FAILFAST_DRIVER;
>> In abnormal scenarios, such as request time out, connection process may takes long time or the admin command waits for long time.
>> Retry only for non-host errors may be a better choice. Maybe we can make some optimizations in nvme_decide_disposition.
> 
> Thanks for your review!
> 
> Oh, I agreed that it might wait for so long time in connecting process.
> Restricting some of commands that should be retried would be better as
> you mentiond.
> 
> Do you mean that maybe we can check this module parameter in
> nvme_decide_disposition()?  Like, even if blk_noretry_request(req) says
> that it's non-retriable, if this module parameter is enabled, then we
> can retry rather than failfast?
No, I mean that add the local preferential retry which defined in the NVMe protocol.
---
  drivers/nvme/host/core.c | 18 +++++++++++++-----
  drivers/nvme/host/nvme.h |  1 +
  2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 99f91efe3824..a25e9b4956b9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -316,15 +316,20 @@ 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;

+	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;
+
  	if (req->cmd_flags & REQ_NVME_MPATH) {
-		if (nvme_is_path_error(nvme_req(req)->status) ||
-		    blk_queue_dying(req->q))
-			return FAILOVER;
+		return FAILOVER;
  	} else {
  		if (blk_queue_dying(req->q))
  			return COMPLETE;
@@ -2513,6 +2518,7 @@ 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])
  		return 0;
@@ -2524,6 +2530,8 @@ 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 bfcedfa4b057..fd914b0dec88 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -276,6 +276,7 @@ struct nvme_ctrl {
  #ifdef CONFIG_BLK_DEV_ZONED
  	u32 max_zone_append;
  #endif
+	bool acre;
  	u16 crdt[3];
  	u16 oncs;
  	u16 oacs;
-- 
> 
> Thanks,
> .
> 

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

Re: [PATCH 3/3] nvme: add parameter command_retry to enable retry

[Minwoo Im]

Hello,

On 21-01-12 17:34:48, Chao Leng wrote:
> 
> 
> On 2021/1/11 20:27, Minwoo Im wrote:
> > Hello,
> > 
> > On 21-01-11 11:47:56, Chao Leng wrote:
> > > 
> > > 
> > > On 2021/1/8 22:46, Minwoo Im wrote:
> > > > nvme_init_request() has set REQ_FAILFAST_DRIVER to make requests
> > > > non-retryable.  This command flag value is checked in
> > > > nvme_decide_disposition() to decide whether to RETRY or other
> > > > operations.  In that point, blk_noretry_request() macro will be used to
> > > > check if command flags have one of REQ_FAILFAST_*.  If so, it just
> > > > decides to complete the request without retrying.
> > > > 
> > > > This patch added a module parameter named command_retry to turn on the
> > > > command retry feature in this driver.  If turning it on,
> > > > REQ3_FAILFAST_DRIVER will not be set to requests so that retry can be
> > > > reached out to nvme_retry_req() based on the module parameter.
> > > > 
> > > > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> > > > ---
> > > >    drivers/nvme/host/core.c | 7 ++++++-
> > > >    1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > > index 6e428fdc25a8..e1836ca9956f 100644
> > > > --- a/drivers/nvme/host/core.c
> > > > +++ b/drivers/nvme/host/core.c
> > > > @@ -44,6 +44,10 @@ static unsigned char shutdown_timeout = 5;
> > > >    module_param(shutdown_timeout, byte, 0644);
> > > >    MODULE_PARM_DESC(shutdown_timeout, "timeout in seconds for controller shutdown");
> > > > +static bool command_retry;
> > > > +module_param(command_retry, bool, 0644);
> > > > +MODULE_PARM_DESC(command_retry, "retry commands up to nvme_max_retries");
> > > > +
> > > >    static u8 nvme_max_retries = 5;
> > > >    module_param_named(max_retries, nvme_max_retries, byte, 0644);
> > > >    MODULE_PARM_DESC(max_retries, "max number of retries a command may have");
> > > > @@ -560,7 +564,8 @@ static inline void nvme_init_request(struct request *req,
> > > >    	else /* no queuedata implies admin queue */
> > > >    		req->timeout = NVME_ADMIN_TIMEOUT;
> > > > -	req->cmd_flags |= REQ_FAILFAST_DRIVER;
> > > > +	if (!command_retry)
> > > > +		req->cmd_flags |= REQ_FAILFAST_DRIVER;
> > > In abnormal scenarios, such as request time out, connection process may takes long time or the admin command waits for long time.
> > > Retry only for non-host errors may be a better choice. Maybe we can make some optimizations in nvme_decide_disposition.
> > 
> > Thanks for your review!
> > 
> > Oh, I agreed that it might wait for so long time in connecting process.
> > Restricting some of commands that should be retried would be better as
> > you mentiond.
> > 
> > Do you mean that maybe we can check this module parameter in
> > nvme_decide_disposition()?  Like, even if blk_noretry_request(req) says
> > that it's non-retriable, if this module parameter is enabled, then we
> > can retry rather than failfast?
> No, I mean that add the local preferential retry which defined in the NVMe protocol.

Thanks for your suggestion, Chao.  I think this is much better idea than
module parameter.  Fixing the nvme_decide_disposition() would be better!
I think it's going to be also an answer for Christoph's question on this
patch.  Please let me prepare next pach series.

Thanks,

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

Re: [PATCH 3/3] nvme: add parameter command_retry to enable retry

[Christoph Hellwig]

On Fri, Jan 08, 2021 at 11:46:59PM +0900, Minwoo Im wrote:
> This patch added a module parameter named command_retry to turn on the
> command retry feature in this driver.  If turning it on,
> REQ3_FAILFAST_DRIVER will not be set to requests so that retry can be
> reached out to nvme_retry_req() based on the module parameter.

Why would we want to do that, and if so why only conditional on a module
parameter?

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

Re: [PATCH 3/3] nvme: add parameter command_retry to enable retry

[Minwoo Im]

Hello,

On 21-01-12 09:09:26, Christoph Hellwig wrote:
> On Fri, Jan 08, 2021 at 11:46:59PM +0900, Minwoo Im wrote:
> > This patch added a module parameter named command_retry to turn on the
> > command retry feature in this driver.  If turning it on,
> > REQ3_FAILFAST_DRIVER will not be set to requests so that retry can be
> > reached out to nvme_retry_req() based on the module parameter.
> 
> Why would we want to do that, and if so why only conditional on a module
> parameter?

If reqeust->cmd_flags are set with the REQ_FAILFAST_DRIVER in
nvme_init_request(), then it will always go to COMPLETE case rather than
RETRY in nvme_decide_disposition():

	if (blk_noretry_request(req) ||
		...

And I thought that to make command delay feature enabled, module
parameter was a good start to toggle this feature.

Thanks!

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