[PATCH] scsi: core: Simplify the code for waking up the error handler

[PATCH] scsi: core: Simplify the code for waking up the error handler

[Bart Van Assche]

scsi_dec_host_busy() is called from the hot path and hence must not
obtain the host lock if no commands have failed. scsi_dec_host_busy()
tests three different variables of which at least two are set if a
command failed. Commit 3bd6f43f5cb3 ("scsi: core: Ensure that the
SCSI error handler gets woken up") introduced a call_rcu() call to
ensure that all tasks observe the host state change before the
host_failed change. Simplify the approach for guaranteeing that the host
state and host_failed/host_eh_scheduled changes are observed in order by using
smp_store_release() to update host_failed or host_eh_scheduled after
having update the host state and smp_load_acquire() before reading the
host state.

Cc: Ming Lei <ming.lei@redhat.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: John Garry <john.g.garry@oracle.com>
Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_error.c | 22 ++++------------------
 drivers/scsi/scsi_lib.c   | 31 +++++++++----------------------
 include/scsi/scsi_cmnd.h  |  2 --
 3 files changed, 13 insertions(+), 42 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 2aa2c2aee6e7..2a809145da06 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -87,7 +87,8 @@ void scsi_schedule_eh(struct Scsi_Host *shost)
 
 	if (scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
 	    scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0) {
-		shost->host_eh_scheduled++;
+		smp_store_release(&shost->host_eh_scheduled,
+				  shost->host_eh_scheduled + 1);
 		scsi_eh_wakeup(shost);
 	}
 
@@ -278,18 +279,6 @@ static void scsi_eh_reset(struct scsi_cmnd *scmd)
 	}
 }
 
-static void scsi_eh_inc_host_failed(struct rcu_head *head)
-{
-	struct scsi_cmnd *scmd = container_of(head, typeof(*scmd), rcu);
-	struct Scsi_Host *shost = scmd->device->host;
-	unsigned long flags;
-
-	spin_lock_irqsave(shost->host_lock, flags);
-	shost->host_failed++;
-	scsi_eh_wakeup(shost);
-	spin_unlock_irqrestore(shost->host_lock, flags);
-}
-
 /**
  * scsi_eh_scmd_add - add scsi cmd to error handling.
  * @scmd:	scmd to run eh on.
@@ -312,12 +301,9 @@ void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
 
 	scsi_eh_reset(scmd);
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
+	smp_store_release(&shost->host_failed, shost->host_failed + 1);
+	scsi_eh_wakeup(shost);
 	spin_unlock_irqrestore(shost->host_lock, flags);
-	/*
-	 * Ensure that all tasks observe the host state change before the
-	 * host_failed change.
-	 */
-	call_rcu_hurry(&scmd->rcu, scsi_eh_inc_host_failed);
 }
 
 /**
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b7c569a42aa4..e59eb0cbfc83 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -263,28 +263,24 @@ int scsi_execute_cmd(struct scsi_device *sdev, const unsigned char *cmd,
 }
 EXPORT_SYMBOL(scsi_execute_cmd);
 
-/*
- * Wake up the error handler if necessary. Avoid as follows that the error
- * handler is not woken up if host in-flight requests number ==
- * shost->host_failed: use call_rcu() in scsi_eh_scmd_add() in combination
- * with an RCU read lock in this function to ensure that this function in
- * its entirety either finishes before scsi_eh_scmd_add() increases the
- * host_failed counter or that it notices the shost state change made by
- * scsi_eh_scmd_add().
- */
+/* Wake up the error handler if necessary. */
 static void scsi_dec_host_busy(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 {
 	unsigned long flags;
 
-	rcu_read_lock();
 	__clear_bit(SCMD_STATE_INFLIGHT, &cmd->state);
-	if (unlikely(scsi_host_in_recovery(shost))) {
+	/*
+	 * Test host_failed and host_eh_scheduled before the host state to
+	 * ensure that the host state update is observed if the host_failed
+	 * and/or host_eh_scheduled updates are observed.
+	 */
+	if (unlikely((smp_load_acquire(&shost->host_failed) ||
+		      smp_load_acquire(&shost->host_eh_scheduled)))) {
 		spin_lock_irqsave(shost->host_lock, flags);
-		if (shost->host_failed || shost->host_eh_scheduled)
+		if (scsi_host_in_recovery(shost))
 			scsi_eh_wakeup(shost);
 		spin_unlock_irqrestore(shost->host_lock, flags);
 	}
-	rcu_read_unlock();
 }
 
 void scsi_device_unbusy(struct scsi_device *sdev, struct scsi_cmnd *cmd)
@@ -547,14 +543,6 @@ static bool scsi_end_request(struct request *req, blk_status_t error,
 		cmd->flags &= ~SCMD_INITIALIZED;
 	}
 
-	/*
-	 * Calling rcu_barrier() is not necessary here because the
-	 * SCSI error handler guarantees that the function called by
-	 * call_rcu() has been called before scsi_end_request() is
-	 * called.
-	 */
-	destroy_rcu_head(&cmd->rcu);
-
 	/*
 	 * In the MQ case the command gets freed by __blk_mq_end_request,
 	 * so we have to do all cleanup that depends on it earlier.
@@ -1124,7 +1112,6 @@ static void scsi_initialize_rq(struct request *rq)
 	memset(cmd->cmnd, 0, sizeof(cmd->cmnd));
 	cmd->cmd_len = MAX_COMMAND_SIZE;
 	cmd->sense_len = 0;
-	init_rcu_head(&cmd->rcu);
 	cmd->jiffies_at_alloc = jiffies;
 	cmd->retries = 0;
 }
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index c2cb5f69635c..7965f5114144 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -71,8 +71,6 @@ struct scsi_cmnd {
 	struct list_head eh_entry; /* entry for the host eh_abort_list/eh_cmd_q */
 	struct delayed_work abort_work;
 
-	struct rcu_head rcu;
-
 	int eh_eflags;		/* Used by error handlr */
 
 	int budget_token;

Re: [PATCH] scsi: core: Simplify the code for waking up the error handler

[Benjamin Block]

On Tue, Mar 07, 2023 at 01:51:51PM -0800, Bart Van Assche wrote:
> scsi_dec_host_busy() is called from the hot path and hence must not
> obtain the host lock if no commands have failed. scsi_dec_host_busy()
> tests three different variables of which at least two are set if a
> command failed. Commit 3bd6f43f5cb3 ("scsi: core: Ensure that the
> SCSI error handler gets woken up") introduced a call_rcu() call to
> ensure that all tasks observe the host state change before the
> host_failed change. Simplify the approach for guaranteeing that the host
> state and host_failed/host_eh_scheduled changes are observed in order by using
> smp_store_release() to update host_failed or host_eh_scheduled after
> having update the host state and smp_load_acquire() before reading the
> host state.

It's probably just me, but "simplify" is a bit of a misnomer when you
replace RCU by plain memory barriers. And I'm kind of wondering what we
improve here? It seems to me that at least as far as the hot path is
concerned, nothing really changes? The situation for
`scsi_eh_scmd_add()` seems to improve, but that is already way off the
hot path.

> 
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: John Garry <john.g.garry@oracle.com>
> Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/scsi_error.c | 22 ++++------------------
>  drivers/scsi/scsi_lib.c   | 31 +++++++++----------------------
>  include/scsi/scsi_cmnd.h  |  2 --
>  3 files changed, 13 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 2aa2c2aee6e7..2a809145da06 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -87,7 +87,8 @@ void scsi_schedule_eh(struct Scsi_Host *shost)
>  
>  	if (scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
>  	    scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0) {
> -		shost->host_eh_scheduled++;
> +		smp_store_release(&shost->host_eh_scheduled,
> +				  shost->host_eh_scheduled + 1);

Probably should be documented.

>  		scsi_eh_wakeup(shost);
>  	}
>  
> @@ -312,12 +301,9 @@ void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
>  
>  	scsi_eh_reset(scmd);
>  	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
> +	smp_store_release(&shost->host_failed, shost->host_failed + 1);

Same.

> +	scsi_eh_wakeup(shost);
>  	spin_unlock_irqrestore(shost->host_lock, flags);
> -	/*
> -	 * Ensure that all tasks observe the host state change before the
> -	 * host_failed change.
> -	 */
> -	call_rcu_hurry(&scmd->rcu, scsi_eh_inc_host_failed);
>  }
>  
>  /**

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /         Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

Re: [PATCH] scsi: core: Simplify the code for waking up the error handler

[Bart Van Assche]

On 3/9/23 04:13, Benjamin Block wrote:
> On Tue, Mar 07, 2023 at 01:51:51PM -0800, Bart Van Assche wrote:
>> scsi_dec_host_busy() is called from the hot path and hence must not
>> obtain the host lock if no commands have failed. scsi_dec_host_busy()
>> tests three different variables of which at least two are set if a
>> command failed. Commit 3bd6f43f5cb3 ("scsi: core: Ensure that the
>> SCSI error handler gets woken up") introduced a call_rcu() call to
>> ensure that all tasks observe the host state change before the
>> host_failed change. Simplify the approach for guaranteeing that the host
>> state and host_failed/host_eh_scheduled changes are observed in order by using
>> smp_store_release() to update host_failed or host_eh_scheduled after
>> having update the host state and smp_load_acquire() before reading the
>> host state.
> 
> It's probably just me, but "simplify" is a bit of a misnomer when you
> replace RCU by plain memory barriers. And I'm kind of wondering what we
> improve here? It seems to me that at least as far as the hot path is
> concerned, nothing really changes? The situation for
> `scsi_eh_scmd_add()` seems to improve, but that is already way off the
> hot path.

Hi Benjamin,

The advantages of the approach introduced by this patch are as follows:
* The size of struct scsi_cmnd is reduced. This may improve performance
   by reducing the number of cache misses.
* One call_rcu() call is eliminated. This reduces the error handler
   wake-up latency.

>>   	if (scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
>>   	    scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0) {
>> -		shost->host_eh_scheduled++;
>> +		smp_store_release(&shost->host_eh_scheduled,
>> +				  shost->host_eh_scheduled + 1);
> 
> Probably should be documented.

I will add a comment above each new store release / load acquire operation.

Thanks,

Bart.

Re: [PATCH] scsi: core: Simplify the code for waking up the error handler

[Benjamin Block]

On Thu, Mar 09, 2023 at 09:20:45AM -0800, Bart Van Assche wrote:
> On 3/9/23 04:13, Benjamin Block wrote:
> > On Tue, Mar 07, 2023 at 01:51:51PM -0800, Bart Van Assche wrote:
> >> scsi_dec_host_busy() is called from the hot path and hence must not
> >> obtain the host lock if no commands have failed. scsi_dec_host_busy()
> >> tests three different variables of which at least two are set if a
> >> command failed. Commit 3bd6f43f5cb3 ("scsi: core: Ensure that the
> >> SCSI error handler gets woken up") introduced a call_rcu() call to
> >> ensure that all tasks observe the host state change before the
> >> host_failed change. Simplify the approach for guaranteeing that the host
> >> state and host_failed/host_eh_scheduled changes are observed in order by using
> >> smp_store_release() to update host_failed or host_eh_scheduled after
> >> having update the host state and smp_load_acquire() before reading the
> >> host state.
> > 
> > It's probably just me, but "simplify" is a bit of a misnomer when you
> > replace RCU by plain memory barriers. And I'm kind of wondering what we
> > improve here? It seems to me that at least as far as the hot path is
> > concerned, nothing really changes? The situation for
> > `scsi_eh_scmd_add()` seems to improve, but that is already way off the
> > hot path.
> 
> Hi Benjamin,
> 
> The advantages of the approach introduced by this patch are as follows:
> * The size of struct scsi_cmnd is reduced. This may improve performance
>    by reducing the number of cache misses.
> * One call_rcu() call is eliminated. This reduces the error handler
>    wake-up latency.

Is that really a problem today? From personal experience we hardly ever
have any real SCSI timeouts in our environments, and once we are at the
point where we add commands to EH, we are already past the (default) 30s
timeout for disks IIRC, so is the RCU latency significant at that point?

I'm just wondering, because at least IMHO plain memory barrier are more
complex than RCU - at least in understanding them.

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /         Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294