nvme: don't retry request marked as NVME_REQ_CANCELLED

If request is marked as NVME_REQ_CANCELLED, we don't need to retry for requeuing it, and it should be completed immediately. Even simply from the flag name, it needn't to be requeued.

Otherwise, it is easy to cause IO hang when IO is timed out in case of PCI NVMe:

1) IO timeout is triggered, and nvme_timeout() tries to disable device(nvme_dev_disable) and reset controller(nvme_reset_ctrl)

2) inside nvme_dev_disable(), queue is frozen and quiesced, and try to cancel every request, but the timeout request can't be canceled since it is completed by __blk_mq_complete_request() in blk_mq_rq_timed_out().

3) this timeout req is requeued via nvme_complete_rq(), but can't be dispatched at all because queue is quiesced and hardware isn't ready, finally nvme_wait_freeze() waits for ever in nvme_reset_work().

Hi Ming

Thanks for your patch and detailed comment.

Quoted 14 lines · Ming Lei

If request is marked as NVME_REQ_CANCELLED, we don't need to retry for requeuing it, and it should be completed immediately. Even simply from the flag name, it needn't to be requeued.

Otherwise, it is easy to cause IO hang when IO is timed out in case of PCI NVMe:

1) IO timeout is triggered, and nvme_timeout() tries to disable device(nvme_dev_disable) and reset controller(nvme_reset_ctrl)

2) inside nvme_dev_disable(), queue is frozen and quiesced, and try to cancel every request, but the timeout request can't be canceled since it is completed by __blk_mq_complete_request() in blk_mq_rq_timed_out().

Quoted 3 lines

3) this timeout req is requeued via nvme_complete_rq(), but can't be dispatched at all because queue is quiesced and hardware isn't ready, finally nvme_wait_freeze() waits for ever in nvme_reset_work().

The queue will be unquiesced by nvme_start_queues() before start to wait freeze.

nvme_reset_work
....

		nvme_start_queues(&dev->ctrl);
		nvme_wait_freeze(&dev->ctrl);
		/* hit this only when allocate tagset fails */
		if (nvme_dev_add(dev))
			new_state = NVME_CTRL_ADMIN_ONLY;
		nvme_unfreeze(&dev->ctrl);

....
And the relationship between nvme_timeout and nvme_dev_disable is really complicated.
The nvme_timeout may run with nvme_dev_disable in parallel and also could invoke it. nvme_dev_disbale may sleep holding shutdown_lock, nvme_timeout will try to get it when invokes nvme_dev_disable....

On the other hand, can we provide a helper interface to quiesce the request_work timeout_work ?

Such as following:

block/blk-core.c +41 -0

index b888175..60469cd 100644
@@ -616,6 +616,44 @@ void blk_queue_bypass_end(struct request_queue *q)
 }
 EXPORT_SYMBOL_GPL(blk_queue_bypass_end);
 
+/*
+ * When returns, the timeout_work will be invoked any more.
+ * The caller must take the charge of the outstanding IOs.
+ */
+void blk_quiesce_queue_timeout(struct request_queue *q)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	queue_flag_set(QUEUE_FLAG_TIMEOUT_QUIESCED, q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	synchronize_rcu_bh();
+	cancel_work_sync(&q->timeout_work);
+	/*
+	 * The timeout work will not be invoked anymore.
+	 */
+	del_timer_sync(&q->timeout);
+	/*
+	 * If the caller doesn't want the timer to be re-armed,
+	 * it has to stop/quiesce the queue.
+	 */
+}
+EXPORT_SYMBOL_GPL(blk_quiesce_queue_timeout);
+
+/*
+ * Caller must ensure all the outstanding IOs has been handled.
+ */
+void blk_unquiesce_queue_timeout(struct request_queue *q)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	queue_flag_clear(QUEUE_FLAG_TIMEOUT_QUIESCED, q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+}
+
+EXPORT_SYMBOL_GPL(blk_unquiesce_queue_timeout);
 void blk_set_queue_dying(struct request_queue *q)
 {
 	spin_lock_irq(q->queue_lock);

@@ -867,6 +905,9 @@ static void blk_rq_timed_out_timer(struct timer_list *t)
 {
 	struct request_queue *q = from_timer(q, t, timeout);
 
+	if (blk_queue_timeout_quiesced(q))
+		return;
+
 	kblockd_schedule_work(&q->timeout_work);
 }
 

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

Hi Jianchao,

Quoted 19 lines · jianchao.wang

Hi Ming

Thanks for your patch and detailed comment.

Quoted 14 lines · Ming Lei

If request is marked as NVME_REQ_CANCELLED, we don't need to retry for requeuing it, and it should be completed immediately. Even simply from the flag name, it needn't to be requeued.

Otherwise, it is easy to cause IO hang when IO is timed out in case of PCI NVMe:

1) IO timeout is triggered, and nvme_timeout() tries to disable device(nvme_dev_disable) and reset controller(nvme_reset_ctrl)

2) inside nvme_dev_disable(), queue is frozen and quiesced, and try to cancel every request, but the timeout request can't be canceled since it is completed by __blk_mq_complete_request() in blk_mq_rq_timed_out().

Quoted 5 lines

Quoted 3 lines

3) this timeout req is requeued via nvme_complete_rq(), but can't be dispatched at all because queue is quiesced and hardware isn't ready, finally nvme_wait_freeze() waits for ever in nvme_reset_work().

The queue will be unquiesced by nvme_start_queues() before start to wait freeze.

Hammmm, just miss this point, so no such io hang in this case.

Quoted 9 lines

nvme_reset_work
....

		nvme_start_queues(&dev->ctrl);
		nvme_wait_freeze(&dev->ctrl);
		/* hit this only when allocate tagset fails */
		if (nvme_dev_add(dev))
			new_state = NVME_CTRL_ADMIN_ONLY;
		nvme_unfreeze(&dev->ctrl);

Quoted 5 lines

....
And the relationship between nvme_timeout and nvme_dev_disable is really complicated.
The nvme_timeout may run with nvme_dev_disable in parallel and also could invoke it. nvme_dev_disbale may sleep holding shutdown_lock, nvme_timeout will try to get it when invokes nvme_dev_disable....

Yes, the implementation is really complicated, but nvme_dev_disable() is run exclusively because of dev->shutdown_lock. Also nvme_timeout() only handles the timeout request which is invisible to nvme_dev_disable()

So in concept, looks it is fine, :-)

Quoted 1 line

On the other hand, can we provide a helper interface to quiesce the request_work timeout_work ?

Could you explain a bit why it is required?

Quoted 68 lines

Such as following:

block/blk-core.c +41 -0

index b888175..60469cd 100644
@@ -616,6 +616,44 @@ void blk_queue_bypass_end(struct request_queue *q)
 }
 EXPORT_SYMBOL_GPL(blk_queue_bypass_end);
 
+/*
+ * When returns, the timeout_work will be invoked any more.
+ * The caller must take the charge of the outstanding IOs.
+ */
+void blk_quiesce_queue_timeout(struct request_queue *q)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	queue_flag_set(QUEUE_FLAG_TIMEOUT_QUIESCED, q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	synchronize_rcu_bh();
+	cancel_work_sync(&q->timeout_work);
+	/*
+	 * The timeout work will not be invoked anymore.
+	 */
+	del_timer_sync(&q->timeout);
+	/*
+	 * If the caller doesn't want the timer to be re-armed,
+	 * it has to stop/quiesce the queue.
+	 */
+}
+EXPORT_SYMBOL_GPL(blk_quiesce_queue_timeout);
+
+/*
+ * Caller must ensure all the outstanding IOs has been handled.
+ */
+void blk_unquiesce_queue_timeout(struct request_queue *q)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	queue_flag_clear(QUEUE_FLAG_TIMEOUT_QUIESCED, q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+}
+
+EXPORT_SYMBOL_GPL(blk_unquiesce_queue_timeout);
 void blk_set_queue_dying(struct request_queue *q)
 {
 	spin_lock_irq(q->queue_lock);

@@ -867,6 +905,9 @@ static void blk_rq_timed_out_timer(struct timer_list *t)
 {
 	struct request_queue *q = from_timer(q, t, timeout);
 
+	if (blk_queue_timeout_quiesced(q))
+		return;
+
 	kblockd_schedule_work(&q->timeout_work);
 }
 

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=ZXWFEWytJgigh5HwlzkcMrlwBTLl73tTNVA1fhL2FDk&s=zXoLlfl9uF107FMTkPVyoKQd2QF1i0h9BdcAGlIw4LM&e=

Hi ming

Thanks for your kindly response.
And really sorry for delayed feedback here.

Quoted 45 lines · Ming Lei

Hi Jianchao,

Quoted 19 lines · jianchao.wang

Hi Ming

Thanks for your patch and detailed comment.

Quoted 14 lines · Ming Lei

If request is marked as NVME_REQ_CANCELLED, we don't need to retry for requeuing it, and it should be completed immediately. Even simply from the flag name, it needn't to be requeued.

Otherwise, it is easy to cause IO hang when IO is timed out in case of PCI NVMe:

1) IO timeout is triggered, and nvme_timeout() tries to disable device(nvme_dev_disable) and reset controller(nvme_reset_ctrl)

2) inside nvme_dev_disable(), queue is frozen and quiesced, and try to cancel every request, but the timeout request can't be canceled since it is completed by __blk_mq_complete_request() in blk_mq_rq_timed_out().

Quoted 5 lines

Quoted 3 lines

3) this timeout req is requeued via nvme_complete_rq(), but can't be dispatched at all because queue is quiesced and hardware isn't ready, finally nvme_wait_freeze() waits for ever in nvme_reset_work().

The queue will be unquiesced by nvme_start_queues() before start to wait freeze.

Hammmm, just miss this point, so no such io hang in this case.

Quoted 9 lines

nvme_reset_work
....

		nvme_start_queues(&dev->ctrl);
		nvme_wait_freeze(&dev->ctrl);
		/* hit this only when allocate tagset fails */
		if (nvme_dev_add(dev))
			new_state = NVME_CTRL_ADMIN_ONLY;
		nvme_unfreeze(&dev->ctrl);

Quoted 5 lines

....
And the relationship between nvme_timeout and nvme_dev_disable is really complicated.
The nvme_timeout may run with nvme_dev_disable in parallel and also could invoke it. nvme_dev_disbale may sleep holding shutdown_lock, nvme_timeout will try to get it when invokes nvme_dev_disable....

Quoted 5 lines

Yes, the implementation is really complicated, but nvme_dev_disable() is run exclusively because of dev->shutdown_lock. Also nvme_timeout() only handles the timeout request which is invisible to nvme_dev_disable()

So in concept, looks it is fine, :-)

In fact, I have incurred a scenario which is similar with description as above. nvme_dev_disable issue sq/cq delete command on amdinq, but nvme device didn't response due to some unknown reason. At the moment, the previous IO requests expired, and nvne_timeout is invoked when ctrl->state is RESETTING, then nvme_dev_disable was invoked. So got a scenario:

context A context B
nvme_dev_disable nvme_timeout
hold shutdown_lock nvme_dev_disable
wait sq/cq delete command response require shutdown_lock

Finally, the context A broke out due to timeout, things went ok. But this is a very dangerous.

There are other cases where context may sleep with shutdown_lock held in nvme_dev_disable

Quoted 3 lines

Quoted 1 line

On the other hand, can we provide a helper interface to quiesce the request_work timeout_work ?

Could you explain a bit why it is required?

For nvme_dev_disable, it need to cancel all the previously outstanding requests. And it cannot wait them to be completed, because the device may goes wrong at the moment.

But nvme_dev_disable may run with nvme_timeout in parallel or race with it.

The best way to fix this is to ensure the timeout path has been completed before cancel the previously outstanding requests. (Just ignore the case where the nvme_timeout will invoke nvme_dev_disable, it has to be fixed by other way.)

Quoted 1 line

But nvme_dev_disable may run with nvme_timeout in parallel or race with it.

But that doesn't mean it is a race, blk_mq_complete_request() can avoid race between timeout and other completions, such as cancel.
Yes, I know blk_mq_complete_request could avoid the a request is accessed by timeout

Quoted 3 lines

The best way to fix this is to ensure the timeout path has been completed before cancel the previously outstanding requests. (Just ignore the case where the nvme_timeout will invoke nvme_dev_disable, it has to be fixed by other way.)

Maybe your approach looks a bit clean and simplify the implementation, but seems it isn't necessary.

So could you explain a bit what the exact issue you are worrying about? deadlock? or others?

Hi ming

Thanks for your detailed response.
That's really appreciated.

Quoted 4 lines · Ming Lei

Quoted 1 line

But nvme_dev_disable may run with nvme_timeout in parallel or race with it.

But that doesn't mean it is a race, blk_mq_complete_request() can avoid race between timeout and other completions, such as cancel.
Yes, I know blk_mq_complete_request could avoid the a request is accessed by timeout

path and other completion path concurrently. :)
What's I worry about is the timeout path could hold the expired request, so when nvme_dev_disable return, we cannot ensure all the previous outstanding requests has been handled. That's really bad.

Quoted 3 lines

Quoted 3 lines

The best way to fix this is to ensure the timeout path has been completed before cancel the previously outstanding requests. (Just ignore the case where the nvme_timeout will invoke nvme_dev_disable, it has to be fixed by other way.)

Quoted 5 lines

Maybe your approach looks a bit clean and simplify the implementation, but seems it isn't necessary.

So could you explain a bit what the exact issue you are worrying about? deadlock? or others?

There is indeed potential issue. But it is in very narrow window. Please refer to https://lkml.org/lkml/2018/1/19/68

Yes, once controller disabling completes, any started requests will be handled and cannot expire. But before the _boundary_, there could be a nvme_timeout context runs with nvme_dev_disable in parallel. If a timeout path grabs a request, then nvme_dev_disable cannot get and cancel it.

So even though the nvme_dev_disable completes, there still could be a request in nvme_timeout context.

The worst case is :
nvme_timeout nvme_reset_work
if (ctrl->state == RESETTING ) nvme_dev_disable
nvme_dev_disable initializing procedure

the nvme_dev_disable run with reinit procedure in nvme_reset_work in parallel.

Quoted 10 lines

Quoted 8 lines

Quoted 3 lines

The best way to fix this is to ensure the timeout path has been completed before cancel the previously outstanding requests. (Just ignore the case where the nvme_timeout will invoke nvme_dev_disable, it has to be fixed by other way.)

Maybe your approach looks a bit clean and simplify the implementation, but seems it isn't necessary.

So could you explain a bit what the exact issue you are worrying about? deadlock? or others?

There is indeed potential issue. But it is in very narrow window.
Please refer to https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2018_1_19_68&d=DwICAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=vcQM1keq4TzxrKgWxIBOPADXTw7IRtGtWTBlHVIvPn8&s=83k_LmLl0TFLYwkVn4HjZbj5RgIMlLv570rnB5jNRC0&e=

OK, follows description from the above link:

Quoted 7 lines

Yes, once controller disabling completes, any started requests will be handled and cannot expire. But before the _boundary_, there could be a nvme_timeout context runs with nvme_dev_disable in parallel. If a timeout path grabs a request, then nvme_dev_disable cannot get and cancel it.

So even though the nvme_dev_disable completes, there still could be a request in nvme_timeout context.

Quoted 6 lines

The worst case is :
nvme_timeout nvme_reset_work
if (ctrl->state == RESETTING ) nvme_dev_disable
nvme_dev_disable initializing procedure

the nvme_dev_disable run with reinit procedure in nvme_reset_work in parallel.

OK, that is the issue, the nvme_dev_disable() in nvme_timeout() may disable queues again, and cause hang in nvme_reset_work().

Looks Keith's suggestion of introducing nvme_sync_queues() should be enough to kill the race, but seems nvme_sync_queues() should have been called at the entry of nvme_reset_work() unconditionally.