On Mon, 2016-05-30 at 15:27 +0800, Wei Fang wrote:

Hi James, Christoph,

On 2016/5/29 23:41, James Bottomley wrote:

quoted

On Sat, 2016-05-28 at 23:54 -0700, Christoph Hellwig wrote:

quoted

On Sat, May 28, 2016 at 11:51:11AM +0800, Wei Fang wrote:

quoted

async_sas_ata_eh(), which will call scsi_eh_finish_cmd() in 
some case, would be performed simultaneously in
sas_ata_strategy_handler(). In this case, ->host_failed may 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. Then SCSI error handler thread won't become 
running, SCSI errors after that won't be handled forever.

Use atomic type for ->host_failed to fix this race.

Looks fine,

Actually, it doesn't look fine at all.  The same mechanism that's
supposed to protect the host_failed decrement is also supposed to
protect the list_move_tail().  If there's a problem with the former
then we're also in danger of corrupting the list.

Scmd is moved to local eh_done_q list here, and I checked that the
list won't be touched concurrently.

quoted

Can we go back to the theory of what the problem is, since it's not
spelled out very clearly in the change log.  Our usual reason for 
not requiring locking in eh routines is that the eh is single 
threaded on the eh thread per host, so any host manipulations can't 
have concurrency problems.  In this case, the sas_ata routines are
trying to be clever and use asynchronous workqueues for the port 
error handler and you theorise that these can execute concurrently 
on two CPUs, thus causing the problem?

Yes, it's the case. The works of the port error handler are added to
system_unbound_wq, and will be performed concurrently on different 
CPUs. We have already met that problem on our machine.

OK, add that to the changelog and also that this fixes

commit 50824d6c5657ce340e3911171865a8d99fdd8eba
Author: Dan Williams [off-list ref]
Date:   Sun Dec 4 01:06:24 2011 -0800

    [SCSI] libsas: async ata-eh

Because that's where the concurrency rules weren't verified when this
async threading was added.

One final thing is that we don't need this replaced by atomics.  The
only atomic check we need is the up count, which is already serialised
by the host lock.  Nothing actually ever bothers with the down count,
so it can just be eliminated and host_failed set to zero after the
strategy handle is complete (but before scsi_restart_operations) in the
eh_thread.

Once this change is made, scsi_eh_finish_cmd() and
scsi_eh_flush_done_q() are safe provided the done_q list is not
modifiable by any other thread.

As Christoph said, the documentation needs updating to reflect these
new concurrency rules.

James