diff mbox series

scsi: core: Simplify the code for waking up the error handler

Message ID 20230307215151.3705164-1-bvanassche@acm.org
State New
Headers show
Series scsi: core: Simplify the code for waking up the error handler | expand

Commit Message

Bart Van Assche March 7, 2023, 9:51 p.m. UTC 
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(-)

Comments

Bart Van Assche March 9, 2023, 5:20 p.m. UTC | #1 
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.
diff mbox series

Patch

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;