Re: [PATCH 1/2] scsi: fix race between simultaneous decrements of ->host_failed

Wei Fang
2016-06-02T08:35:07Z

Forgot to add changes, please ignore it.

On 2016/6/2 15:54, Wei Fang wrote:
> sas_ata_strategy_handler() adds the works of the ata error handler
> to system_unbound_wq. This workqueue asynchronously runs work items,
> so the ata error handler will be performed concurrently on different
> CPUs. In this case, ->host_failed will be decreased simultaneously in
> scsi_eh_finish_cmd() on different CPUs, and become abnormal.
>
> It will lead to permanently inequal between ->host_failed and
> ->host_busy, and scsi error handler thread won't become running.
> IO errors after that won't be handled forever.
>
> Since all scmds must have been handled in the strategy handle, just
> remove the decrement in scsi_eh_finish_cmd() and zero ->host_busy
> after the strategy handle to fix this race.
>
> This fixes the problem introduced in
> commit 50824d6c5657 ("[SCSI] libsas: async ata-eh").
>
> Signed-off-by: Wei Fang <fangwei1@huawei.com>
> ---
> drivers/ata/libata-eh.c | 2 +-
> drivers/scsi/scsi_error.c | 4 +++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 961acc7..91a9e6a 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -606,7 +606,7 @@ void ata_scsi_error(struct Scsi_Host *host)
> ata_scsi_port_error_handler(host, ap);
>
> /* finish or retry handled scmd's and clean up */
> - WARN_ON(host->host_failed || !list_empty(&eh_work_q));
> + WARN_ON(!list_empty(&eh_work_q));
>
> DPRINTK("EXIT\n");
> }
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 984ddcb..1b9c049 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1127,7 +1127,6 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
> */
> void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
> {
> - scmd->device->host->host_failed--;
> scmd->eh_eflags = 0;
> list_move_tail(&scmd->eh_entry, done_q);
> }
> @@ -2226,6 +2225,9 @@ int scsi_error_handler(void *data)
> else
> scsi_unjam_host(shost);
>
> + /* All scmds have been handled */
> + shost->host_failed = 0;
> +
> /*
> * Note - if the above fails completely, the action is to take
> * individual devices offline and flush the queue of any
>