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]>

---
  drivers/scsi/scsi_error.c | 22 +++++++++++-----------
  drivers/scsi/scsi_lib.c   |  6 +++++-
  include/scsi/scsi_cmnd.h  |  5 ++++-
  3 files changed, 20 insertions(+), 13 deletions(-)

drivers/scsi/scsi_error.c +11 -11

index dd338a8cd275..e92e088f636f 100644
@@ -297,19 +297,19 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 
  	if (rtn == BLK_EH_DONE) {
  		/*
-		 * For blk-mq, we must set the request state to complete now
-		 * before sending the request to the scsi error handler. This
-		 * will prevent a use-after-free in the event the LLD manages
-		 * to complete the request before the error handler finishes
-		 * processing this timed out request.
+		 * Set the command to complete first in order to prevent a real
+		 * completion from releasing the command while error handling
+		 * is using it. If the command was already completed, then the
+		 * lower level driver beat the timeout handler, and it is safe
+		 * to return without escalating error recovery.
  		 *
-		 * If the request was already completed, then the LLD beat the
-		 * time out handler from transferring the request to the scsi
-		 * error handler. In that case we can return immediately as no
-		 * further action is required.
+		 * If timeout handling lost the race to a real completion, the
+		 * block layer may ignore that due to a fake timeout injection,
+		 * so return RESET_TIMER to allow error handling another shot
+		 * at this command.
  		 */
-		if (!blk_mq_mark_complete(req))
-			return rtn;
+		if (test_and_set_bit(__SCMD_COMPLETE, &scmd->flags))
+			return BLK_EH_RESET_TIMER;
  		if (scsi_abort_command(scmd) != SUCCESS) {
  			set_host_byte(scmd, DID_TIME_OUT);
  			scsi_eh_scmd_add(scmd);

drivers/scsi/scsi_lib.c +4 -1

index 5d83a162d03b..c1d5e4e36125 100644
@@ -1635,8 +1635,11 @@ static blk_status_t scsi_mq_prep_fn(struct request *req)
 
  static void scsi_mq_done(struct scsi_cmnd *cmd)
  {
+	if (unlikely(test_and_set_bit(__SCMD_COMPLETE, &cmd->flags)))
+		return;
  	trace_scsi_dispatch_cmd_done(cmd);
-	blk_mq_complete_request(cmd->request);
+	if (unlikely(!blk_mq_complete_request(cmd->request)))
+		clear_bit(__SCMD_COMPLETE, &cmd->flags);
  }
 
  static void scsi_mq_put_budget(struct blk_mq_hw_ctx *hctx)

Quoted 45 lines · Keith Busch

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]>>>---

 drivers/scsi/scsi_error.c |22 +-+-+-+-+-+-+-+-+-+-+------------
 drivers/scsi/scsi_lib.c   | 6 +-+-+-+-+--
 include/scsi/scsi_cmnd.h  | 5 +-+-+-+--
 3 files changed, 20 insertions(+-), 13 deletions(-)

drivers/scsi/scsi_error.c +1 -1

index dd338a8cd275..e92e088f636f 100644
--- a/drivers/scsi/scsi_error.c
+-+-+- b/drivers/scsi/scsi_error.c

@@-297,19 +-297,19 @@enum blk_eh_timer_return scsi_times_out(struct request *req)

drivers/scsi/scsi_error.c +10 -9

 	if (rtn ==BLK_EH_DONE) {>>  		/*>>-		 *For blk-mq, we must set the request state to complete now
-		 *before sending the request to the scsi error handler. This
-		 *will prevent a use-after-free in the event the LLD manages
-		 *to complete the request before the error handler finishes
-		 *processing this timed out request.
+-		 *Set the command to complete first in order to prevent a real
+-		 *completion from releasing the command while error handling
+-		 *is using it. If the command was already completed, then the
+-		 *lower level driver beat the timeout handler, and it is safe
+-		 *to return without escalating error recovery.
 		 *>>-		 *If the request was already completed, then the LLD beat the
-		 *time out handler from transferring the request to the scsi
-		 *error handler. In that case we can return immediately as no
-		 *further action is required.
+-		 *If timeout handling lost the race to a real completion, the
+-		 *block layer may ignore that due to a fake timeout injection,
+-		 *so return RESET_TIMER to allow error handling another shot
+-		 *at this command.
 		 */
-		if (!blk_mq_mark_complete(req))
-			return rtn;>>+-		if (test_and_set_bit(__SCMD_COMPLETE, &scmd->flags))
+-			return BLK_EH_RESET_TIMER;>>  		if (scsi_abort_command(scmd) !=SUCCESS) {>>  			set_host_byte(scmd, DID_TIME_OUT);>>  			scsi_eh_scmd_add(scmd);>>diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c

index 5d83a162d03b..c1d5e4e36125 100644
--- a/drivers/scsi/scsi_lib.c
+-+-+- b/drivers/scsi/scsi_lib.c
@@-1635,8 +-1635,11 @@static blk_status_t scsi_mq_prep_fn(struct request *req)

Quoted 7 lines

static void scsi_mq_done(struct scsi_cmnd *cmd)
{>>+- if (unlikely(test_and_set_bit(__SCMD_COMPLETE, &cmd->flags)))

+-		return;>>  	trace_scsi_dispatch_cmd_done(cmd);>>-	blk_mq_complete_request(cmd->request);>>+-	if (unlikely(!blk_mq_complete_request(cmd->request)))
+-		clear_bit(__SCMD_COMPLETE, &cmd->flags);>>  }>>  

static void scsi_mq_put_budget(struct blk_mq_hw_ctx *hctx)
@@-1701,6 +-1704,7 @@static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
goto out_dec_host_busy;>> req->rq_flags |=RQF_DONTPREP;>> }else {>>+- cmd->flags &=~SCMD_COMPLETE;>> blk_mq_start_request(req);>> }>>

Why do you use a normal assignment here (and not a clear_bit() as in the above hunk)?

And shouldn't you add a barrier here to broadcast the state change to other cores?

index 5d83a162d03b..c1d5e4e36125 100644

drivers/scsi/scsi_lib.c +1 -0

 static void scsi_mq_put_budget(struct blk_mq_hw_ctx *hctx)

@@ -1701,6 +1704,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 			goto out_dec_host_busy;
 		req->rq_flags |= RQF_DONTPREP;
 	} else {
+		cmd->flags &= ~SCMD_COMPLETE;
 		blk_mq_start_request(req);
 	}
 

include/scsi/scsi_cmnd.h +0 -0

index d6fd2aba0380..ded7c7194a28 100644

Quoted 5 lines

index 5d83a162d03b..c1d5e4e36125 100644

drivers/scsi/scsi_lib.c +0 -0

@@ -1635,8 +1635,11 @@ static blk_status_t scsi_mq_prep_fn(struct request *req)
 

drivers/scsi/scsi_lib.c static blk_status_t scsi_mq_prep_fn(struct request *req) · line 1643

Quoted 9 lines

drivers/scsi/scsi_lib.c +4 -1

 static void scsi_mq_done(struct scsi_cmnd *cmd)
 {
+	if (unlikely(test_and_set_bit(__SCMD_COMPLETE, &cmd->flags)))
+		return;
 	trace_scsi_dispatch_cmd_done(cmd);
-	blk_mq_complete_request(cmd->request);
+	if (unlikely(!blk_mq_complete_request(cmd->request)))
+		clear_bit(__SCMD_COMPLETE, &cmd->flags);
 }

This looks a little odd to me. If we didn't complete the command someone else did. Why would we clear the bit in this case?

Quoted 5 lines

index 5d83a162d03b..c1d5e4e36125 100644

drivers/scsi/scsi_lib.c +0 -0

@@ -1635,8 +1635,11 @@ static blk_status_t scsi_mq_prep_fn(struct request *req)
 

drivers/scsi/scsi_lib.c static blk_status_t scsi_mq_prep_fn(struct request *req) · line 1643

Quoted 9 lines

drivers/scsi/scsi_lib.c +4 -1

 static void scsi_mq_done(struct scsi_cmnd *cmd)
 {
+	if (unlikely(test_and_set_bit(__SCMD_COMPLETE, &cmd->flags)))
+		return;
 	trace_scsi_dispatch_cmd_done(cmd);
-	blk_mq_complete_request(cmd->request);
+	if (unlikely(!blk_mq_complete_request(cmd->request)))
+		clear_bit(__SCMD_COMPLETE, &cmd->flags);
 }

This looks a little odd to me. If we didn't complete the command someone else did. Why would we clear the bit in this case?

Quoted 18 lines

drivers/scsi/scsi_lib.c +1 -0

 static void scsi_mq_put_budget(struct blk_mq_hw_ctx *hctx)

@@ -1701,6 +1704,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 			goto out_dec_host_busy;
 		req->rq_flags |= RQF_DONTPREP;
 	} else {
+		cmd->flags &= ~SCMD_COMPLETE;
 		blk_mq_start_request(req);
 	}
 

include/scsi/scsi_cmnd.h +1 -0

index d6fd2aba0380..ded7c7194a28 100644
@@ -58,6 +58,9 @@ struct scsi_pointer {
 #define SCMD_TAGGED		(1 << 0)
 #define SCMD_UNCHECKED_ISA_DMA	(1 << 1)
 #define SCMD_INITIALIZED	(1 << 2)
+

include/scsi/scsi_cmnd.h struct scsi_pointer { · line 63

Quoted 2 lines

include/scsi/scsi_cmnd.h +2 -0

+#define __SCMD_COMPLETE		3
+#define SCMD_COMPLETE		(1 << __SCMD_COMPLETE)

This mixing of atomic and non-atomic bitops looks rather dangerous to me. Can you add a new atomic_flags just for the completed flag, and always use the bitops on it for now? I think we can eventually kill most of the existing flags except for SCMD_TAGGED over the next merge window or two and then move that over as well.

Otherwise the concept looks fine to me.

Quoted 5 lines · Christoph Hellwig

Quoted 5 lines

index 5d83a162d03b..c1d5e4e36125 100644

drivers/scsi/scsi_lib.c +0 -0

@@ -1635,8 +1635,11 @@ static blk_status_t scsi_mq_prep_fn(struct request *req)
 

Quoted 12 lines

drivers/scsi/scsi_lib.c static blk_status_t scsi_mq_prep_fn(struct request *req) · line 1643

Quoted 9 lines

drivers/scsi/scsi_lib.c +4 -1

 static void scsi_mq_done(struct scsi_cmnd *cmd)
 {
+	if (unlikely(test_and_set_bit(__SCMD_COMPLETE, &cmd->flags)))
+		return;
 	trace_scsi_dispatch_cmd_done(cmd);
-	blk_mq_complete_request(cmd->request);
+	if (unlikely(!blk_mq_complete_request(cmd->request)))
+		clear_bit(__SCMD_COMPLETE, &cmd->flags);
 }

This looks a little odd to me. If we didn't complete the command someone else did. Why would we clear the bit in this case?

It's only to go along with the fake timeout. If we don't clear the bit, then then scsi timeout handler will believe it has nothing to do because scsi did its required part. The block layer just pretends the LLD didn't do its part, so scsi has to play along too.

Quoted 8 lines

drivers/scsi/scsi_lib.c static blk_status_t scsi_mq_prep_fn(struct request *req) · line 1645

Quoted 2 lines

drivers/scsi/scsi_lib.c +2 -0

+#define __SCMD_COMPLETE		3
+#define SCMD_COMPLETE		(1 << __SCMD_COMPLETE)

This mixing of atomic and non-atomic bitops looks rather dangerous to me. Can you add a new atomic_flags just for the completed flag, and always use the bitops on it for now? I think we can eventually kill most of the existing flags except for SCMD_TAGGED over the next merge window or two and then move that over as well.

The only concurrent access is completion + timeout, otherwise access is single-threaded. I'm using the atomic operations only where it is needed.

We implicitly clear the SCMD_COMPLETED flag along with SCMD_TAGGED in scsi_init_command() too, and I didn't want to add new overhead with new atomics.

Quoted 19 lines · Keith Busch

Quoted 5 lines · Christoph Hellwig

Quoted 5 lines

index 5d83a162d03b..c1d5e4e36125 100644

drivers/scsi/scsi_lib.c +0 -0

@@ -1635,8 +1635,11 @@ static blk_status_t scsi_mq_prep_fn(struct request *req)
 

Quoted 12 lines

drivers/scsi/scsi_lib.c static blk_status_t scsi_mq_prep_fn(struct request *req) · line 1643

Quoted 9 lines

drivers/scsi/scsi_lib.c +4 -1

 static void scsi_mq_done(struct scsi_cmnd *cmd)
 {
+	if (unlikely(test_and_set_bit(__SCMD_COMPLETE, &cmd->flags)))
+		return;
 	trace_scsi_dispatch_cmd_done(cmd);
-	blk_mq_complete_request(cmd->request);
+	if (unlikely(!blk_mq_complete_request(cmd->request)))
+		clear_bit(__SCMD_COMPLETE, &cmd->flags);
 }

This looks a little odd to me. If we didn't complete the command someone else did. Why would we clear the bit in this case?

Quoted 4 lines

It's only to go along with the fake timeout. If we don't clear the bit, then then scsi timeout handler will believe it has nothing to do because scsi did its required part. The block layer just pretends the LLD didn't do its part, so scsi has to play along too.

This just looks way to magic to me. In other word - it needs a big fat comment explaining the situation.

Quoted 13 lines

Quoted 8 lines

drivers/scsi/scsi_lib.c static blk_status_t scsi_mq_prep_fn(struct request *req) · line 1645

Quoted 2 lines

drivers/scsi/scsi_lib.c +2 -0

+#define __SCMD_COMPLETE		3
+#define SCMD_COMPLETE		(1 << __SCMD_COMPLETE)

This mixing of atomic and non-atomic bitops looks rather dangerous to me. Can you add a new atomic_flags just for the completed flag, and always use the bitops on it for now? I think we can eventually kill most of the existing flags except for SCMD_TAGGED over the next merge window or two and then move that over as well.

The only concurrent access is completion + timeout, otherwise access is single-threaded. I'm using the atomic operations only where it is needed.

Quoted 3 lines

We implicitly clear the SCMD_COMPLETED flag along with SCMD_TAGGED in scsi_init_command() too, and I didn't want to add new overhead with new atomics.

In general mixing access types on a single field (nevermind bit) is going to cause us problems further down the road sooner or later.

I'd be much happier with a separate field.

Quoted 45 lines · Christoph Hellwig

Quoted 19 lines · Keith Busch

Quoted 5 lines · Christoph Hellwig

Quoted 5 lines

index 5d83a162d03b..c1d5e4e36125 100644

drivers/scsi/scsi_lib.c +0 -0

@@ -1635,8 +1635,11 @@ static blk_status_t scsi_mq_prep_fn(struct request *req)
 

Quoted 12 lines

drivers/scsi/scsi_lib.c static blk_status_t scsi_mq_prep_fn(struct request *req) · line 1643

Quoted 9 lines

drivers/scsi/scsi_lib.c +4 -1

 static void scsi_mq_done(struct scsi_cmnd *cmd)
 {
+	if (unlikely(test_and_set_bit(__SCMD_COMPLETE, &cmd->flags)))
+		return;
 	trace_scsi_dispatch_cmd_done(cmd);
-	blk_mq_complete_request(cmd->request);
+	if (unlikely(!blk_mq_complete_request(cmd->request)))
+		clear_bit(__SCMD_COMPLETE, &cmd->flags);
 }

This looks a little odd to me. If we didn't complete the command someone else did. Why would we clear the bit in this case?

Quoted 4 lines

It's only to go along with the fake timeout. If we don't clear the bit, then then scsi timeout handler will believe it has nothing to do because scsi did its required part. The block layer just pretends the LLD didn't do its part, so scsi has to play along too.

This just looks way to magic to me. In other word - it needs a big fat comment explaining the situation.

Quoted 16 lines

Quoted 8 lines

drivers/scsi/scsi_lib.c static blk_status_t scsi_mq_prep_fn(struct request *req) · line 1645

Quoted 2 lines

drivers/scsi/scsi_lib.c +2 -0

+#define __SCMD_COMPLETE		3
+#define SCMD_COMPLETE		(1 << __SCMD_COMPLETE)

This mixing of atomic and non-atomic bitops looks rather dangerous to me. Can you add a new atomic_flags just for the completed flag, and always use the bitops on it for now? I think we can eventually kill most of the existing flags except for SCMD_TAGGED over the next merge window or two and then move that over as well.

The only concurrent access is completion + timeout, otherwise access is single-threaded. I'm using the atomic operations only where it is needed.

We implicitly clear the SCMD_COMPLETED flag along with SCMD_TAGGED in scsi_init_command() too, and I didn't want to add new overhead with new atomics.

Quoted 4 lines

In general mixing access types on a single field (nevermind bit) is going to cause us problems further down the road sooner or later.

I'd be much happier with a separate field.

Keith, will you please respin with the separate field? Would be nice to get this merged for 4.21.

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 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 need it, and removing the software layering violation that was required to use that mechnaism.