nvme: allow timed-out ios to retry

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(-)

drivers/nvme/host/core.c +0 -2

index acc816b67582..90d09067a82a 100644
@@ -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;

Quoted 6 lines · 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.

Quoted 8 lines

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?

Quoted 20 lines · Keith Busch

Quoted 10 lines · 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.

Quoted 8 lines

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.

Quoted 8 lines

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.

Quoted 4 lines · James Smart

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.

Quoted 8 lines · Keith Busch

Quoted 4 lines · James Smart

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'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.

Quoted 4 lines

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.