[PATCH v2 0/1] scsi: core: Restart the queue when unblocking a device set running

[PATCH v2 0/1] scsi: core: Restart the queue when unblocking a device set running

[Brian Bunker]

A SCSI device can be left in SDEV_RUNNING with its request queue stopped
for good. The task that put it there hangs holding the device lock, and
anything else that takes the lock hangs behind it.

This was seen with iSCSI when a session drops again just after
recovering. The error handler offlines a device while its transport has
it blocked, which leaves the queue stopped. iscsid then writes "running"
to the device's state attribute and store_state_field() moves it to
SDEV_RUNNING without restarting the queue. The rescan it starts sends an
INQUIRY that can never be dispatched. When the transport later unblocks
the device, scsi_internal_device_unblock_nowait() rejects it with
-EINVAL because it is already running, so nothing restarts the queue.

Changes in v2:
 - v1 restarted the queue in store_state_field(). Sashiko pointed out
   that this unquiesces a device its transport still has blocked, which
   transports such as SRP and drivers such as aacraid rely on while they
   reconnect or reset the adapter. Drop that.
 - Instead, let scsi_internal_device_unblock_nowait() finish the unblock
   of a device that is SDEV_RUNNING with its queue still stopped. The
   transport now decides when the queue restarts, and the device moves
   to the state the transport asked for. This also stops
   scsi_host_unblock() from bailing out at such a device.
 - The cost is that the writer of "running" stays stuck holding the
   device lock until the transport unblocks, instead of for good.
 v1: https://lore.kernel.org/r/20261030164229.10037-1-brian@purestorage.com

Evidence from a vmcore of a hung initiator (a RHEL 9.6 5.14 kernel; the
code involved is the same in mainline). The device is sd 0:0:0:1, and
iscsid is the task that wrote "running" to it:

  PID: 12087  TASK: ffff9635c9c80000  COMMAND: "iscsid"
   #5 blk_execute_rq
   #6 scsi_execute_cmd
   #7 scsi_vpd_inquiry
  #10 scsi_attach_vpd
  #11 scsi_rescan_device_rh
  #12 store_state_field
  #13 kernfs_fop_write_iter

  sdev_state                   = SDEV_RUNNING
  queue_stopped                = 1
  request_queue->quiesce_depth = 1
  sdev_gendev.mutex.owner      = 0xffff9635c9c80001 (iscsid, plus a flag bit)

The INQUIRY iscsid is waiting on belongs to that device's queue and was
never dispatched:

  state = MQ_RQ_IDLE, start_time_ns = 0, deadline = 0

The other LUNs of the same session were unblocked by the transport and
had their queues restarted:

  lun 1: SDEV_RUNNING            queue_stopped = 1
  lun 2: SDEV_TRANSPORT_OFFLINE  queue_stopped = 0
  ...
  lun 9: SDEV_TRANSPORT_OFFLINE  queue_stopped = 0

(scsi_rescan_device_rh is this distro kernel's name for
scsi_rescan_device().)

The kernel log of the same crash shows the sequence for the stuck device
(sd 0:0:0:1) and its siblings:

  [50958.843822]  connection9:0: detected conn error (1021)
  [50968.259428]  connection9:0: detected conn error (1020)
  [50968.259456] sd 0:0:0:1: Device offlined - not ready after error recovery
  [50978.298947]  session9: session recovery timed out after 10 secs
  [50978.298977] sd 0:0:0:2: rejecting I/O to offline device

The other LUNs are unblocked by the recovery timeout at 50978. The
offlined device is not: by then it had already been written back to
running.

Questions for review:

 - The new case only runs for a device that is SDEV_RUNNING with
   queue_stopped set. As far as I can tell that only happens when
   something sets the device running while it is blocked, as the sysfs
   write does here. Is there any caller that relies on -EINVAL for that
   combination?
 - scsi_internal_device_unblock_nowait() can be called without
   state_mutex by some drivers. Could a concurrent unblock see the
   transient SDEV_RUNNING-with-stopped-queue state between the state
   assignment and scsi_start_queue() and apply a different new_state?

I do not have a deterministic reproducer. The analysis comes from the
vmcore, and initiators running the v1 change have not hit the hang
again, but with a race this narrow that is not proof. Review of the
state transitions is welcome.

Brian Bunker (1):
  scsi: core: Restart the queue when unblocking a device set running

 drivers/scsi/scsi_lib.c | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.56.0

[PATCH v2 1/1] scsi: core: Restart the queue when unblocking a device set running

[Brian Bunker]

A device that its transport blocks has its queue stopped. If the SCSI
error handler then gives up on the device, it moves it from SDEV_BLOCK
to SDEV_OFFLINE and leaves the queue stopped. When userspace later
writes "running" to the device's state attribute, store_state_field()
moves it to SDEV_RUNNING, still with the queue stopped, and rescans it.
The rescan takes the device lock and waits for I/O that can never be
dispatched.

When the transport then unblocks the device,
scsi_internal_device_unblock_nowait() rejects a device in SDEV_RUNNING
with -EINVAL before it reaches scsi_start_queue(), so nothing ever
restarts the queue. The task that wrote "running" hangs forever holding
the device lock, and anything else that takes it hangs behind it. The
-EINVAL also makes scsi_host_unblock() stop at this device and leave the
rest of the host blocked, and keeps the device in SDEV_RUNNING when the
transport asked for SDEV_TRANSPORT_OFFLINE.

This shows up with iSCSI when a session drops again just after
recovering. iscsid onlines the session's devices after a relogin by
writing "running" to every device it finds offline:

  1. The session is blocked, which blocks the device and stops its
     queue.
  2. The error handler cannot recover the device and moves it from
     SDEV_BLOCK to SDEV_OFFLINE. The queue stays stopped.
  3. iscsid writes "running": SDEV_OFFLINE -> SDEV_RUNNING with the
     queue still stopped, then the rescan sends an INQUIRY that sits
     unsent while holding the device lock.
  4. The session recovery timeout calls scsi_target_unblock(). The
     other devices go from SDEV_BLOCK to SDEV_TRANSPORT_OFFLINE and get
     their queues restarted, but this one is skipped because it is
     already running.

A vmcore from such a hang shows the device in SDEV_RUNNING with
queue_stopped set and the queue's quiesce_depth at 1, the rescan's
INQUIRY never started (MQ_RQ_IDLE, no start time or deadline), and
iscsid owning the device lock while other tasks wait for it.

Handle this in scsi_internal_device_unblock_nowait(), which already
restarts the queue of an SDEV_OFFLINE device: accept an SDEV_RUNNING
device whose queue is still stopped, move it to the requested state and
restart the queue. A running device whose queue is not stopped was never
blocked and is still rejected.

Do not restart the queue from store_state_field() instead. The device is
still blocked by its transport at that point, and some transports and
drivers rely on the stopped queue to keep commands away while they
reconnect or reset the adapter.

Fixes: f0f82e2476f6 ("scsi: core: Fix capacity set to zero after offlinining device")
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi_lib.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f695a6605815..b89407902430 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3065,6 +3065,15 @@ int scsi_internal_device_unblock_nowait(struct scsi_device *sdev,
 		else
 			sdev->sdev_state = SDEV_CREATED;
 		break;
+	case SDEV_RUNNING:
+		/*
+		 * The device was set running through sysfs while it was
+		 * blocked, so its queue is still stopped. Finish the unblock.
+		 */
+		if (!sdev->queue_stopped)
+			return -EINVAL;
+		sdev->sdev_state = new_state;
+		break;
 	case SDEV_CANCEL:
 	case SDEV_OFFLINE:
 		break;
-- 
2.56.0