On 7/2/21 1:31 PM, Alan Stern wrote:

On Fri, Jul 02, 2021 at 06:03:20PM +0100, John Garry wrote:

quoted

Hi guys,

We're experiencing a deadlock between trying to remove a SATA device and
doing a rescan in scsi_rescan_device().

I'm just looking for a suggestion on how to solve.

The background is that the host (hisi sas v3 hw) uses SAS SCSI transport and
supports RPM. In the testcase, the host and disks are put to suspend. Then
we run fio on the disk to make them active and then immediately hard reset
the disk link, which causes the disk to be disconnected (please don't ask
why ...).

We find that there is a conflict between the rescan and the device removal
code, resulting in a deadlock:

quoted

The rescan holds the sdev_gendev.device lock in scsi_rescan_device(), while
the removal code in __scsi_remove_device() wants to grab it.

However the rescan will not release (the lock) until the blk_queue_enter()
succeeds, above. That can happen 2x ways:

- the queue is dying, which would not happen until after the device_del() in
__scsi_remove_device(), so not going to happen

- q->pm_only falls to 0. This would be when scsi_runtime_resume() ->
sdev_runtime_resume() -> blk_post_runtime_resume(err = 0) ->
blk_set_runtime_active() is called. However, I find that the err argument
for me is -5, which comes from sdev_runtime_resume() -> pm->runtime_resume
(=sd_resume()), which fails. That sd_resume() -> sd_start_stop_device()
fails as the disk is not attached. So we go into error state:

$:more /sys/devices/pci0000:b4/0000:b4:04.0/host3/port-3:0/end_device-3:0/target3:0:0/3:0:0:0/power/runtime_status
error

Removing commit e27829dc92e5 ("scsi: serialize ->rescan against ->remove")
solves this issue for me, but that is there for a reason.

Any suggestion on how to fix this deadlock?

This is indeed a tricky question.  It seems like we should allow a 
runtime resume to succeed if the only reason it failed was that the 
device has been removed.

More generally, perhaps we should always consider that a runtime 
resume succeeds.  Any remaining problems will be dealt with by the 
device's driver and subsystem once the device is marked as 
runtime-active again.

Suppose you try changing blk_post_runtime_resume() so that it always 
calls blk_set_runtime_active() regardless of the value of err.  Does 
that fix the problem?

And more importantly, will it cause any other problems...?

That would cause trouble for the UFS driver and other drivers for which
runtime resume can fail due to e.g. the link between host and device
being in a bad state.

How about checking the SCSI device state inside scsi_rescan_device() and
skipping the rescan if the SCSI device state is SDEV_CANCEL or SDEV_DEL?

Adding such a check inside __scsi_execute() would break sd_remove() and
sd_shutdown() since both use __scsi_execute() to submit a SYNCHRONIZE
CACHE command to the device.

Bart.