scsi timeout handling updates

A driver may have internal state to cleanup if we're pretending a request didn't complete. Return 'false' if the command wasn't actually completed due to the timeout error injection, and true otherwise.

Signed-off-by Keith Busch <[email protected]>

The scsi timeout error handling had been directly updating the block layer's request state to prevent a error handling and a natural completion from completing the same request twice. Fix this layering violation by having scsi control the fate of its commands with scsi owned flags rather than use blk-mq's.

Signed-off-by Keith Busch <[email protected]>

There are no more users relying on blk-mq request states to prevent double completions, so replace the relatively expensive cmpxchg operation with WRITE_ONCE.

Signed-off-by Keith Busch <[email protected]>

The iterative update to the previous version taking into account review comments.

Background:

The main objective is to remove the generic block layer's lock prefix currently required to transition a request to its completed state by shifting that expense to lower level drivers that actually need it, and removing the software layering violation that was required to use that mechnaism.

Quoted 5 lines · Keith Busch

The iterative update to the previous version taking into account review comments.

Background:

Quoted 5 lines

The main objective is to remove the generic block layer's lock prefix currently required to transition a request to its completed state by shifting that expense to lower level drivers that actually need it, and removing the software layering violation that was required to use that mechnaism.

Thanks Keith, added for 4.21.

Quoted 12 lines · Jens Axboe

Quoted 10 lines · Keith Busch

The iterative update to the previous version taking into account review comments.

Background:

The main objective is to remove the generic block layer's lock prefix currently required to transition a request to its completed state by shifting that expense to lower level drivers that actually need it, and removing the software layering violation that was required to use that mechnaism.

Quoted 1 line

Thanks Keith, added for 4.21.

Hi Jens & Keith,

Seems this patchset causes the following kernel panic, since not see this issue with commit 4ab32bf3305e

Quoted 13 lines · Ming Lei

Quoted 12 lines · Jens Axboe

Quoted 10 lines · Keith Busch

The iterative update to the previous version taking into account review comments.

Background:

The main objective is to remove the generic block layer's lock prefix currently required to transition a request to its completed state by shifting that expense to lower level drivers that actually need it, and removing the software layering violation that was required to use that mechnaism.

Quoted 6 lines

Quoted 1 line

Thanks Keith, added for 4.21.

Hi Jens & Keith,

Seems this patchset causes the following kernel panic, since not see this issue with commit 4ab32bf3305e

Is this the nvme target on top of null_blk?

Quoted 1 line

Is this the nvme target on top of null_blk?

Yes.

Quoted 3 lines · Ming Lei

Quoted 1 line

Is this the nvme target on top of null_blk?

Yes.

And it goes away if you revert just the last patch?

Quoted 5 lines · Christoph Hellwig

Quoted 3 lines · Ming Lei

Quoted 1 line

Is this the nvme target on top of null_blk?

Yes.

Quoted 1 line

And it goes away if you revert just the last patch?

It looks like a problem existed before that last patch. Reverting it helps only if the request happened to have not been reallocated. If it had been reallocated, the NULL_IRQ_TIMER would have completed the wrong request out-of-order. If this were a real device, that'd probably result in data corruption.

Quoted 8 lines · Keith Busch

Quoted 6 lines · Christoph Hellwig

Quoted 3 lines · Ming Lei

Quoted 1 line

Is this the nvme target on top of null_blk?

Yes.

And it goes away if you revert just the last patch?

Quoted 5 lines

It looks like a problem existed before that last patch. Reverting it helps only if the request happened to have not been reallocated. If it had been reallocated, the NULL_IRQ_TIMER would have completed the wrong request out-of-order. If this were a real device, that'd probably result in data corruption.

null_blk just needs updating for this.

Quoted 15 lines · Jens Axboe

Quoted 13 lines · Keith Busch

Quoted 6 lines · Christoph Hellwig

Quoted 3 lines · Ming Lei

Quoted 1 line

Is this the nvme target on top of null_blk?

Yes.

And it goes away if you revert just the last patch?

It looks like a problem existed before that last patch. Reverting it helps only if the request happened to have not been reallocated. If it had been reallocated, the NULL_IRQ_TIMER would have completed the wrong request out-of-order. If this were a real device, that'd probably result in data corruption.

Quoted 1 line

null_blk just needs updating for this.

Isn't this the nvme target's problem? It shouldn't complete requests dispatched to its backing device, so I'm thinking something like the following is what should happen. Untested at the moment, but will try it out shortly.

---

drivers/nvme/target/loop.c +2 -3

index 9908082b32c4..116398b240e5 100644
@@ -428,10 +428,14 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
 static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl)
 {
 	if (ctrl->ctrl.queue_count > 1) {
-		nvme_stop_queues(&ctrl->ctrl);
-		blk_mq_tagset_busy_iter(&ctrl->tag_set,
-					nvme_cancel_request, &ctrl->ctrl);
+		/*
+		 * The back end device driver is responsible for completing all

Quoted 14 lines · Keith Busch

---

drivers/nvme/target/loop.c +2 -3

index 9908082b32c4..116398b240e5 100644
@@ -428,10 +428,14 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
 static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl)
 {
 	if (ctrl->ctrl.queue_count > 1) {
-		nvme_stop_queues(&ctrl->ctrl);
-		blk_mq_tagset_busy_iter(&ctrl->tag_set,
-					nvme_cancel_request, &ctrl->ctrl);
+		/*
+		 * The back end device driver is responsible for completing all

drivers/nvme/target/loop.c static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)

Quoted 10 lines

drivers/nvme/target/loop.c +5 -1

+		 * entered requests
+		 */
+		nvme_start_freeze(&ctrl->ctrl);
+		nvme_wait_freeze(&ctrl->ctrl);
 		nvme_loop_destroy_io_queues(ctrl);
+		nvme_unfreeze(&ctrl->ctrl);
 	}
 
 	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
---

The above tests fine with io and nvme resets on a target nvme loop backed by null_blk, but I also couldn't recreate the original report without the patch.

Ming,

Could you tell me a little more how you set it up? I'm just configuring null_blk with queue_mode=2 irqmode=2. Anything else to recreate?

---

drivers/nvme/target/loop.c +2 -3

index 9908082b32c4..116398b240e5 100644
@@ -428,10 +428,14 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
 static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl)
 {
 	if (ctrl->ctrl.queue_count > 1) {
-		nvme_stop_queues(&ctrl->ctrl);
-		blk_mq_tagset_busy_iter(&ctrl->tag_set,
-					nvme_cancel_request, &ctrl->ctrl);
+		/*
+		 * The back end device driver is responsible for completing all

Waiting for a freeze isn't really the criteria we need anyway: we don't care if there are entered requests in REQ_MQ_IDLE. We just want to wait for dispatched ones to return, and we currently don't have a good way to sync with that condition.

block/blk-mq.c EXPORT_SYMBOL(blk_mq_complete_request); · line 652

Quoted 12 lines

block/blk-mq.c +1 -1

index a82830f39933..d0ef540711c7 100644
@@ -647,7 +647,7 @@ EXPORT_SYMBOL(blk_mq_complete_request);
 
 int blk_mq_request_started(struct request *rq)
 {
-	return blk_mq_rq_state(rq) != MQ_RQ_IDLE;
+	return blk_mq_rq_state(rq) == MQ_RQ_IN_FLIGHT;
 }
 EXPORT_SYMBOL_GPL(blk_mq_request_started);

Independ of this series this change looks like the right thing to do. But this whole area is a mine field, so separate testing would be very helpful.

I also wonder why we even bother with the above helper, a direct state comparism seems a lot more obvious to the reader.

Last but not least the blk_mq_request_started check in nbd should probably be lifted into blk_mq_tag_to_rq while we're at it..

As for the nvme issue - it seems to me like we need to decouple the nvme loop frontend request from the target backend request. In case of a timeout/reset we'll complete struct request like all other nvme transport drivers, but we leave the backend target state around, which will be freed when it completes (or leaks when the completion is lost).

Quoted 17 lines · Christoph Hellwig

block/blk-mq.c EXPORT_SYMBOL(blk_mq_complete_request); · line 652

Quoted 12 lines

block/blk-mq.c +1 -1

index a82830f39933..d0ef540711c7 100644
@@ -647,7 +647,7 @@ EXPORT_SYMBOL(blk_mq_complete_request);
 
  int blk_mq_request_started(struct request *rq)
  {
-	return blk_mq_rq_state(rq) != MQ_RQ_IDLE;
+	return blk_mq_rq_state(rq) == MQ_RQ_IN_FLIGHT;
  }
  EXPORT_SYMBOL_GPL(blk_mq_request_started);

Independ of this series this change looks like the right thing to do. But this whole area is a mine field, so separate testing would be very helpful.

Quoted 2 lines

I also wonder why we even bother with the above helper, a direct state comparism seems a lot more obvious to the reader.

I think it's just because blk_mq_rq_state() is a private interface. The enum mq_rq_state is defined under include/linux/, so it looks okay to make getting the state public too.

Quoted 8 lines

Last but not least the blk_mq_request_started check in nbd should probably be lifted into blk_mq_tag_to_rq while we're at it..

As for the nvme issue - it seems to me like we need to decouple the nvme loop frontend request from the target backend request. In case of a timeout/reset we'll complete struct request like all other nvme transport drivers, but we leave the backend target state around, which will be freed when it completes (or leaks when the completion is lost).

I don't think nvme's loop target should do anything to help a command complete. It shouldn't even implement a timeout for the same reason no stacking block driver implements these. If a request is stuck, the lowest level is the only driver that should have the responsibility to make it unstuck.

Quoted 3 lines · Ming Lei

Quoted 1 line

Is this the nvme target on top of null_blk?

Yes.

And it goes away if you revert just the last patch?

Quoted 6 lines · Ming Lei

Quoted 3 lines

Quoted 1 line

Is this the nvme target on top of null_blk?

Yes.

And it goes away if you revert just the last patch?

Today I run this test again and can't reproduce it any more.

So maybe not a new issue, and it is just triggered in yesterday's for-4.21/block, :-)

Ming,

Quoted 10 lines · Christoph Hellwig

Quoted 6 lines · Ming Lei

Quoted 3 lines

Quoted 1 line

Is this the nvme target on top of null_blk?

Yes.

And it goes away if you revert just the last patch?

Today I run this test again and can't reproduce it any more.

Quoted 2 lines

So maybe not a new issue, and it is just triggered in yesterday's for-4.21/block, :-)

Can you reproduce with yesterday's tree then?

I'm obviously a bit concerned about merging something without knowing the cause of the problems you were seeing.