[PATCH] nvme: allow timed-out ios to retry

[PATCH] nvme: allow timed-out ios to retry

[James Smart]

Currently the nvme_req_needs_retry() applies several checks to see if
a retry is allowed. On of those is whether the current time has exceeded
the start time of the io plus the timeout length. This check, if an io
times out, means there is never a retry allowed for the io. Which means
applications see the io failure.

Remove this check and allow the io to timeout, like it does on other
protocols, and retries to be made.

On the FC transport, a frame can be lost for an individual io, and there
may be no other errors that escalate for the connection/association.
The io will timeout, which causes the transport to escalate into creating
a new association, but the io that timed out, due to this retry logic, has
already failed back to the application and things are hosed.

Signed-off-by: James Smart <james.smart at broadcom.com>
---
 drivers/nvme/host/core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index acc816b67582..90d09067a82a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -134,8 +134,6 @@ static inline bool nvme_req_needs_retry(struct request *req)
 		return false;
 	if (nvme_req(req)->status & NVME_SC_DNR)
 		return false;
-	if (jiffies - req->start_time >= req->timeout)
-		return false;
 	if (nvme_req(req)->retries >= nvme_max_retries)
 		return false;
 	return true;
-- 
2.13.1

[PATCH] nvme: allow timed-out ios to retry

[Keith Busch]

On Thu, Sep 07, 2017@01:18:04PM -0700, James Smart wrote:
> Currently the nvme_req_needs_retry() applies several checks to see if
> a retry is allowed. On of those is whether the current time has exceeded
> the start time of the io plus the timeout length. This check, if an io
> times out, means there is never a retry allowed for the io. Which means
> applications see the io failure.
> 
> Remove this check and allow the io to timeout, like it does on other
> protocols, and retries to be made.
> 
> On the FC transport, a frame can be lost for an individual io, and there
> may be no other errors that escalate for the connection/association.
> The io will timeout, which causes the transport to escalate into creating
> a new association, but the io that timed out, due to this retry logic, has
> already failed back to the application and things are hosed.

I'm a bit conflicted on this. While it'd be nice to give commands a chance
to succeed after a timeout handling's controller reset, some uses would
rather a command fail fast than succeed slow, and this change could keep
a request outstanding for a very long time.

What if we have a second timeout value: one for in-flight timeout before
abort/controller resset, and another for total request lifetime?

> Signed-off-by: James Smart <james.smart at broadcom.com>
> ---
>  drivers/nvme/host/core.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index acc816b67582..90d09067a82a 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -134,8 +134,6 @@ static inline bool nvme_req_needs_retry(struct request *req)
>  		return false;
>  	if (nvme_req(req)->status & NVME_SC_DNR)
>  		return false;
> -	if (jiffies - req->start_time >= req->timeout)
> -		return false;
>  	if (nvme_req(req)->retries >= nvme_max_retries)
>  		return false;
>  	return true;
> -- 

[PATCH] nvme: allow timed-out ios to retry

[James Smart]

On 9/7/2017 1:37 PM, Keith Busch wrote:
> On Thu, Sep 07, 2017@01:18:04PM -0700, James Smart wrote:
>> Currently the nvme_req_needs_retry() applies several checks to see if
>> a retry is allowed. On of those is whether the current time has exceeded
>> the start time of the io plus the timeout length. This check, if an io
>> times out, means there is never a retry allowed for the io. Which means
>> applications see the io failure.
>>
>> Remove this check and allow the io to timeout, like it does on other
>> protocols, and retries to be made.
>>
>> On the FC transport, a frame can be lost for an individual io, and there
>> may be no other errors that escalate for the connection/association.
>> The io will timeout, which causes the transport to escalate into creating
>> a new association, but the io that timed out, due to this retry logic, has
>> already failed back to the application and things are hosed.
> 
> I'm a bit conflicted on this. While it'd be nice to give commands a chance
> to succeed after a timeout handling's controller reset, some uses would
> rather a command fail fast than succeed slow, and this change could keep
> a request outstanding for a very long time.
> 
> What if we have a second timeout value: one for in-flight timeout before
> abort/controller resset, and another for total request lifetime?

I believe its mandatory to allow an in-flight timeout and at least 1 
retry, unless the io callee explicitly disables the retry.  We can't 
make an enterprise-quality solution otherwise.

I assume the existing NVME_IO_TIMEOUT value is what we continue to use 
for the in-flight timeout. "In-flight" defined as outstanding and 
waiting on the controller: i.e. placed on the SQ by the host/transport 
and no corresponding completion received from the controller.

I'm ok with a lifetime timeout. But - is it necessary? Usually the 
lifetime timeout is (io timeout * # retries allowed) and it allows for 
slop as the "timeout" recovery isn't always immediate/instantaneous. In 
other words, Timeout will fire at time X, then the transport does what 
it needs to recover the io as of the timeout, which may take an 
additional amount of time Y, then the retry determinism kicks in. So 
it's not a hard M time ticks.

Like SCSI added "fast_io_fail_tmo" to it's similar "blocked" conditions 
for an io - I expect we need a 3rd timeout for "fastfail". I/O is 
stopped/terminated when the controller is reset or reconnect started. If 
a further retry is not allowed, it will fail back to the callee. If a 
further retry is allowed, the io is queued on the blk queue, but the blk 
queue is stopped by the transport waits for controller reconnection. The 
fastfail timer would start as of the blocking of the blk queues. The 
timer would be cancelled if connectivity is restored and the blk queue 
released again allowing the io to be in-flight again. Timeout expiration 
would fail all pending io on the block queue with a connectivity status 
error and no further retries attempted.


-- james

[PATCH] nvme: allow timed-out ios to retry

[Christoph Hellwig]

On Thu, Sep 07, 2017@04:37:59PM -0400, Keith Busch wrote:
> I'm a bit conflicted on this. While it'd be nice to give commands a chance
> to succeed after a timeout handling's controller reset, some uses would
> rather a command fail fast than succeed slow, and this change could keep
> a request outstanding for a very long time.

That's what we have the fastfail flags for, which we set for all
the internal commands for example? and evaluate just a little bit
above the start_time line.

[PATCH] nvme: allow timed-out ios to retry

[James Smart]

On 9/8/2017 9:11 AM, James Smart wrote:
> On 9/7/2017 1:37 PM, Keith Busch wrote:
>> On Thu, Sep 07, 2017@01:18:04PM -0700, James Smart wrote:
>>> Currently the nvme_req_needs_retry() applies several checks to see if
>>> a retry is allowed. On of those is whether the current time has 
>>> exceeded
>>> the start time of the io plus the timeout length. This check, if an io
>>> times out, means there is never a retry allowed for the io. Which means
>>> applications see the io failure.
>>>
>>> Remove this check and allow the io to timeout, like it does on other
>>> protocols, and retries to be made.
>>>
>>> On the FC transport, a frame can be lost for an individual io, and 
>>> there
>>> may be no other errors that escalate for the connection/association.
>>> The io will timeout, which causes the transport to escalate into 
>>> creating
>>> a new association, but the io that timed out, due to this retry 
>>> logic, has
>>> already failed back to the application and things are hosed.
>>
>> I'm a bit conflicted on this. While it'd be nice to give commands a 
>> chance
>> to succeed after a timeout handling's controller reset, some uses would
>> rather a command fail fast than succeed slow, and this change could keep
>> a request outstanding for a very long time.
>>
>> What if we have a second timeout value: one for in-flight timeout before
>> abort/controller resset, and another for total request lifetime?
>
> I believe its mandatory to allow an in-flight timeout and at least 1 
> retry, unless the io callee explicitly disables the retry.? We can't 
> make an enterprise-quality solution otherwise.
>
> I assume the existing NVME_IO_TIMEOUT value is what we continue to use 
> for the in-flight timeout. "In-flight" defined as outstanding and 
> waiting on the controller: i.e. placed on the SQ by the host/transport 
> and no corresponding completion received from the controller.
>
> I'm ok with a lifetime timeout. But - is it necessary? Usually the 
> lifetime timeout is (io timeout * # retries allowed) and it allows for 
> slop as the "timeout" recovery isn't always immediate/instantaneous. 
> In other words, Timeout will fire at time X, then the transport does 
> what it needs to recover the io as of the timeout, which may take an 
> additional amount of time Y, then the retry determinism kicks in. So 
> it's not a hard M time ticks.
>
> Like SCSI added "fast_io_fail_tmo" to it's similar "blocked" 
> conditions for an io - I expect we need a 3rd timeout for "fastfail". 
> I/O is stopped/terminated when the controller is reset or reconnect 
> started. If a further retry is not allowed, it will fail back to the 
> callee. If a further retry is allowed, the io is queued on the blk 
> queue, but the blk queue is stopped by the transport waits for 
> controller reconnection. The fastfail timer would start as of the 
> blocking of the blk queues. The timer would be cancelled if 
> connectivity is restored and the blk queue released again allowing the 
> io to be in-flight again. Timeout expiration would fail all pending io 
> on the block queue with a connectivity status error and no further 
> retries attempted.
>
>
> -- james
>

So where are we with this - what should be put in place ?

The one revision I'd make from above based is - we'd only apply this 
timer on an I/O marked with a fastfail flag.

-- james

[PATCH] nvme: allow timed-out ios to retry

[Keith Busch]

On Mon, Sep 18, 2017@10:15:45AM -0700, James Smart wrote:
> 
> So where are we with this - what should be put in place ?
> 
> The one revision I'd make from above based is - we'd only apply this timer
> on an I/O marked with a fastfail flag.

That's unnecessary as we already return 'false' immediately from the
blk_noretry_request check (which checks failfast) prior to checking the
timeout and retry count. So, I think you're patch is good as-is.

[PATCH] nvme: allow timed-out ios to retry

[James Smart]

On 9/18/2017 10:24 AM, Keith Busch wrote:
> On Mon, Sep 18, 2017@10:15:45AM -0700, James Smart wrote:
>> So where are we with this - what should be put in place ?
>>
>> The one revision I'd make from above based is - we'd only apply this timer
>> on an I/O marked with a fastfail flag.
> That's unnecessary as we already return 'false' immediately from the
> blk_noretry_request check (which checks failfast) prior to checking the
> timeout and retry count. So, I think you're patch is good as-is.
ok - how about a Reviewed-by ?

[PATCH] nvme: allow timed-out ios to retry

[Keith Busch]

On Mon, Sep 18, 2017@10:42:00AM -0700, James Smart wrote:
> On 9/18/2017 10:24 AM, Keith Busch wrote:
> > On Mon, Sep 18, 2017@10:15:45AM -0700, James Smart wrote:
> > > So where are we with this - what should be put in place ?
> > > 
> > > The one revision I'd make from above based is - we'd only apply this timer
> > > on an I/O marked with a fastfail flag.
> > That's unnecessary as we already return 'false' immediately from the
> > blk_noretry_request check (which checks failfast) prior to checking the
> > timeout and retry count. So, I think you're patch is good as-is.
> ok - how about a Reviewed-by ?

I accidently sent directly to Christoph off-list.

Reviewed-by: Keith Busch <keith.busch at intel.com>

[PATCH] nvme: allow timed-out ios to retry

[Sagi Grimberg]

>> I'm a bit conflicted on this. While it'd be nice to give commands a chance
>> to succeed after a timeout handling's controller reset, some uses would
>> rather a command fail fast than succeed slow, and this change could keep
>> a request outstanding for a very long time.
> 
> That's what we have the fastfail flags for, which we set for all
> the internal commands for example? and evaluate just a little bit
> above the start_time line.

I'm 100% on board with this change!

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>