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

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

[Wei Fang]

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>
---
Changes v1->v2:
- update Documentation/scsi/scsi_eh.txt about ->host_failed
Changes v2->v3:
- don't use atomic type, just zero ->host_failed after the strategy handle

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

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

[Wei Fang]

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>
---
Changes v1->v2:
- update Documentation/scsi/scsi_eh.txt about ->host_failed
Changes v2->v3:
- don't use atomic type, just zero ->host_failed after the strategy handle

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

[PATCH v3 2/2] Documentation/scsi: update scsi_eh.txt about ->host_failed

[Wei Fang]

Update the new rules of ->host_failed.

Signed-off-by: Wei Fang <fangwei1@huawei.com>
---
 Documentation/scsi/scsi_eh.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index 8638f61..9702c78 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -263,7 +263,6 @@ scmd->allowed.
 
  3. scmd recovered
     ACTION: scsi_eh_finish_cmd() is invoked to EH-finish scmd
-	- shost->host_failed--
 	- clear scmd->eh_eflags
 	- scsi_setup_cmd_retry()
 	- move from local eh_work_q to local eh_done_q
@@ -276,6 +275,7 @@ scmd->allowed.
 	- if retry is necessary, scmd is requeued using
           scsi_queue_insert()
 	- otherwise, scsi_finish_command() is invoked for scmd
+	- zero shost->host_failed
     LOCKING: queue or finish function performs appropriate locking
 
 
-- 
1.7.1

[PATCH v3 2/2] Documentation/scsi: update scsi_eh.txt about ->host_failed

[Wei Fang]

Update the new rules of ->host_failed.

Signed-off-by: Wei Fang <fangwei1@huawei.com>
---
 Documentation/scsi/scsi_eh.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index 8638f61..9702c78 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -263,7 +263,6 @@ scmd->allowed.
 
  3. scmd recovered
     ACTION: scsi_eh_finish_cmd() is invoked to EH-finish scmd
-	- shost->host_failed--
 	- clear scmd->eh_eflags
 	- scsi_setup_cmd_retry()
 	- move from local eh_work_q to local eh_done_q
@@ -276,6 +275,7 @@ scmd->allowed.
 	- if retry is necessary, scmd is requeued using
           scsi_queue_insert()
 	- otherwise, scsi_finish_command() is invoked for scmd
+	- zero shost->host_failed
     LOCKING: queue or finish function performs appropriate locking
 
 
-- 
1.7.1

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

[James Bottomley]

On Thu, 2016-06-02 at 16:42 +0800, 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>
> ---
> Changes v1->v2:
> - update Documentation/scsi/scsi_eh.txt about ->host_failed
> Changes v2->v3:
> - don't use atomic type, just zero ->host_failed after the strategy
> handle

Reviewed-by: James Bottomley <jejb@linux.vnet.ibm.com>

Re: [PATCH v3 2/2] Documentation/scsi: update scsi_eh.txt about ->host_failed

[James Bottomley]

On Thu, 2016-06-02 at 16:42 +0800, Wei Fang wrote:
> Update the new rules of ->host_failed.
> 
> Signed-off-by: Wei Fang <fangwei1@huawei.com>

Changes look fine, but I was wondering if we could also add a note
about the new concurrency rules:

> ---
>  Documentation/scsi/scsi_eh.txt |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/scsi/scsi_eh.txt
> b/Documentation/scsi/scsi_eh.txt
> index 8638f61..9702c78 100644
> --- a/Documentation/scsi/scsi_eh.txt
> +++ b/Documentation/scsi/scsi_eh.txt
> @@ -263,7 +263,6 @@ scmd->allowed.
>  
>   3. scmd recovered
>      ACTION: scsi_eh_finish_cmd() is invoked to EH-finish scmd
> -	- shost->host_failed--
>  	- clear scmd->eh_eflags
>  	- scsi_setup_cmd_retry()
>  	- move from local eh_work_q to local eh_done_q

+	CONCURRENCY: at most one thread per separate eh_work_q to
+                    keep queue manipulation lockless

And the same addition in

   4. EH Completes

Probably

    ACTION: scsi_eh_flush_done_q() retries scmds or notifies upper
	    layer of failure. May be called concurrently but must have
            a no more than one thread per eh_work_q to manipulate the
            queue locklessly

> @@ -276,6 +275,7 @@ scmd->allowed.
>  	- if retry is necessary, scmd is requeued using
>            scsi_queue_insert()
>  	- otherwise, scsi_finish_command() is invoked for scmd
> +	- zero shost->host_failed
>      LOCKING: queue or finish function performs appropriate locking
>  
>  

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

[Greg KH]

On Thu, Jun 02, 2016 at 04:42:37PM +0800, 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>


<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

Re: [PATCH v3 2/2] Documentation/scsi: update scsi_eh.txt about ->host_failed

[Greg KH]

On Thu, Jun 02, 2016 at 04:42:38PM +0800, Wei Fang wrote:
> Update the new rules of ->host_failed.
> 
> Signed-off-by: Wei Fang <fangwei1@huawei.com>
> ---
>  Documentation/scsi/scsi_eh.txt |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)


<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

Re: [PATCH v3 2/2] Documentation/scsi: update scsi_eh.txt about ->host_failed

[Martin K. Petersen]

>>>>> "Wei" == Wei Fang <fangwei1@huawei.com> writes:

Wei,

Wei> Update the new rules of ->host_failed.

Are you going to incorporate the amendment from James?

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 2/2] Documentation/scsi: update scsi_eh.txt about ->host_failed

[Martin K. Petersen]

>>>>> "Wei" == Wei Fang <fangwei1@huawei.com> writes:

Wei,

Wei> Update the new rules of ->host_failed.

Are you going to incorporate the amendment from James?

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 2/2] Documentation/scsi: update scsi_eh.txt about ->host_failed

[Wei Fang]

On 2016/6/7 11:22, Martin K. Petersen wrote:
>>>>>> "Wei" == Wei Fang <fangwei1@huawei.com> writes:
> 
> Wei,
> 
> Wei> Update the new rules of ->host_failed.
> 
> Are you going to incorporate the amendment from James?
> 

Yes, I'll send the patch soon.

Thanks,
Wei