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@xxxxxxxxxx>
> Cc: Mike Christie <michael.christie@xxxxxxxxxx>
> Cc: John Garry <john.g.garry@xxxxxxxxxx>
> Cc: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxx>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> 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
