[PATCH] nvme_fc: correct hang in nvme_ns_remove()

[PATCH] nvme_fc: correct hang in nvme_ns_remove()

[James Smart]

When connectivity is lost to a device, the association is terminated
and the blk-mq queues are quiesced/stopped. When connectivity is
re-established, they are resumed.

If connectivity is lost for a sufficient amount of time that the
controller is then deleted, the delete path starts tearing down queues,
and eventually calling nvme_ns_remove(). It appears that pending
commands may cause blk_cleanup_queue() to never complete and the
teardown stalls.

Correct by starting the ns queues after transitioning to a DELETING
state, allowing pending commands to be flushed with io failures. Thus
the delete path is clear when reached.

Signed-off-by: James Smart <james.smart at broadcom.com>
---
 drivers/nvme/host/fc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index fe98e994498c..99bf51c7e513 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2938,6 +2938,9 @@ nvme_fc_delete_ctrl(struct nvme_ctrl *nctrl)
 	 * waiting for io to terminate
 	 */
 	nvme_fc_delete_association(ctrl);
+
+	/* resume the io queues so that things will fast fail */
+	nvme_start_queues(nctrl);
 }
 
 static void
-- 
2.13.1

[PATCH] nvme_fc: correct hang in nvme_ns_remove()

[James Smart]

If you compare behavior of FC with rdma, rdma starts the queues at the 
tail end of losing connectivity to the device - meaning any pending io 
and any future io issued while connectivity has yet to
be re-established (e.g. in RECONNECTING state) will fail with an io
error. This is good, if there is a multipathing config, as it's a
near-immediate fast fail scenario. But... if there is no multipath,
it means applications and filesystems are now seeing io errors while
connectivity is pending and that can be disastrous.  FC currently
leaves the queues quiesced while connectivity is pending so io errors 
are not seen. But this means FC won't fastfail the ios to the
multipath'er.

For now I want to fix this keeping the existing FC behavior. From there, 
I'd like the transports to block like FC does so no errors. However, a 
new timer would be introduced for a "fast failure timeout" - which 
starts at loss of connectivity and when expires, starts the queues and 
fails any pending and future io.

Thoughts ?

-- james


On 1/11/2018 3:21 PM, James Smart wrote:
> When connectivity is lost to a device, the association is terminated
> and the blk-mq queues are quiesced/stopped. When connectivity is
> re-established, they are resumed.
> 
> If connectivity is lost for a sufficient amount of time that the
> controller is then deleted, the delete path starts tearing down queues,
> and eventually calling nvme_ns_remove(). It appears that pending
> commands may cause blk_cleanup_queue() to never complete and the
> teardown stalls.
> 
> Correct by starting the ns queues after transitioning to a DELETING
> state, allowing pending commands to be flushed with io failures. Thus
> the delete path is clear when reached.

[PATCH] nvme_fc: correct hang in nvme_ns_remove()

[Keith Busch]

On Thu, Jan 11, 2018@03:34:58PM -0800, James Smart wrote:
> If you compare behavior of FC with rdma, rdma starts the queues at the tail
> end of losing connectivity to the device - meaning any pending io and any
> future io issued while connectivity has yet to
> be re-established (e.g. in RECONNECTING state) will fail with an io
> error. This is good, if there is a multipathing config, as it's a
> near-immediate fast fail scenario. But... if there is no multipath,
> it means applications and filesystems are now seeing io errors while
> connectivity is pending and that can be disastrous.  FC currently
> leaves the queues quiesced while connectivity is pending so io errors are
> not seen. But this means FC won't fastfail the ios to the
> multipath'er.
> 
> For now I want to fix this keeping the existing FC behavior. From there, I'd
> like the transports to block like FC does so no errors. However, a new timer
> would be introduced for a "fast failure timeout" - which starts at loss of
> connectivity and when expires, starts the queues and fails any pending and
> future io.
> 
> Thoughts ?

Yes, I think that sounds ok.

Longer term, I think it's a bit tacky that we rely on queue_rq to check
for early termination states. Since we can quiece blk-mq, it'd be better
if we introduce another tag iterator to end unstarted requests directly
when we need to give up on the request, rather than rely on queue_rq. I
was going to post a patch that does just that, but I still haven't gotten
a chance to test it... :(

[PATCH] nvme_fc: correct hang in nvme_ns_remove()

[Sagi Grimberg]

>> If you compare behavior of FC with rdma, rdma starts the queues at the tail
>> end of losing connectivity to the device - meaning any pending io and any
>> future io issued while connectivity has yet to
>> be re-established (e.g. in RECONNECTING state) will fail with an io
>> error. This is good, if there is a multipathing config, as it's a
>> near-immediate fast fail scenario. But... if there is no multipath,
>> it means applications and filesystems are now seeing io errors while
>> connectivity is pending and that can be disastrous.  FC currently
>> leaves the queues quiesced while connectivity is pending so io errors are
>> not seen. But this means FC won't fastfail the ios to the
>> multipath'er.
>>
>> For now I want to fix this keeping the existing FC behavior. From there, I'd
>> like the transports to block like FC does so no errors. However, a new timer
>> would be introduced for a "fast failure timeout" - which starts at loss of
>> connectivity and when expires, starts the queues and fails any pending and
>> future io.
>>
>> Thoughts ?
> 
> Yes, I think that sounds ok.
> 
> Longer term, I think it's a bit tacky that we rely on queue_rq to check
> for early termination states. Since we can quiece blk-mq, it'd be better
> if we introduce another tag iterator to end unstarted requests directly
> when we need to give up on the request, rather than rely on queue_rq. I
> was going to post a patch that does just that, but I still haven't gotten
> a chance to test it... :(

I agree with the fast_fail_tmo, we also need it for the multipath code
which currently blocks forever until either we reconnect successfully
or delete the controller.

James, are you looking into this? Note that this should be done in
nvme-core.

[PATCH] nvme_fc: correct hang in nvme_ns_remove()

[Sagi Grimberg]

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index fe98e994498c..99bf51c7e513 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2938,6 +2938,9 @@ nvme_fc_delete_ctrl(struct nvme_ctrl *nctrl)
>   	 * waiting for io to terminate
>   	 */
>   	nvme_fc_delete_association(ctrl);
> +
> +	/* resume the io queues so that things will fast fail */
> +	nvme_start_queues(nctrl);

Is fc also unquiescing the admin queue here?