Re: [PATCH 1/1] scsi: core: Restart a stopped queue when sysfs sets a device running

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: core: store_state_field() unquiesces device while transport is blocked
--

commit 4cd1c48b68b2c4d68b2b301b92ebce5b3fae1723
Author: Brian Bunker <brian@purestorage.com>

scsi: core: Restart a stopped queue when sysfs sets a device running

This commit calls scsi_start_queue() in store_state_field() when transitioning
a SCSI device to SDEV_RUNNING via sysfs so that a device offlined while its
transport had blocked the queue does not hang during the subsequent rescan.

> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index fe54792ed69e4..f5416a1548bd3 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -811,8 +811,16 @@ store_state_field(struct device *dev, struct device_attribute *attr,
>  		ret = 0;
>  	} else {
>  		ret = scsi_device_set_state(sdev, state);
> -		if (ret == 0 && state == SDEV_RUNNING)
> +		if (ret == 0 && state == SDEV_RUNNING) {
> +			/*
> +			 * The device may have been offlined while blocked by
> +			 * its transport, so its queue can still be stopped.
> +			 * The transport's unblock skips a running device and
> +			 * would never restart it, so restart it here.
> +			 */
> +			scsi_start_queue(sdev);
>  			rescan_dev = true;

[Severity: High]
Does transitioning the device to SDEV_RUNNING and calling scsi_start_queue()
while sdev->queue_stopped is set unquiesce the device while its transport or
host still actively has it blocked?

sdev->queue_stopped is set in scsi_stop_queue() when a device is blocked via
scsi_device_block() or scsi_internal_device_block_nowait(). When a transport
or host later unblocks an SDEV_OFFLINE device,
scsi_internal_device_unblock_nowait() in drivers/scsi/scsi_lib.c already
restarts the queue:

	switch (sdev->sdev_state) {
	case SDEV_BLOCK:
	case SDEV_TRANSPORT_OFFLINE:
		sdev->sdev_state = new_state;
		break;
	case SDEV_CREATED_BLOCK:
		if (new_state == SDEV_TRANSPORT_OFFLINE ||
		    new_state == SDEV_OFFLINE)
			sdev->sdev_state = new_state;
		else
			sdev->sdev_state = SDEV_CREATED;
		break;
	case SDEV_CANCEL:
	case SDEV_OFFLINE:
		break;
	default:
		return -EINVAL;
	}
	scsi_start_queue(sdev);

So if an SDEV_OFFLINE device still has sdev->queue_stopped set when
store_state_field() runs, the transport or host has not unblocked it yet.
Unquiescing the queue and moving the device to SDEV_RUNNING in that state
appears to cause several regressions:

1. Some transports and drivers rely on scsi_block_targets() or
scsi_host_block() keeping request queues quiesced for mutual exclusion rather
than rejecting commands in queuecommand. For example, in
include/scsi/scsi_transport_srp.h, srp_chkready() returns 0 even when
rport->state is SRP_RPORT_BLOCKED:

	switch (rport->state) {
	case SRP_RPORT_RUNNING:
	case SRP_RPORT_BLOCKED:
	default:
		return 0;
	case SRP_RPORT_FAIL_FAST:
		return DID_TRANSPORT_FAILFAST << 16;
	case SRP_RPORT_LOST:
		return DID_NO_CONNECT << 16;
	}

While srp_reconnect_rport() in drivers/scsi/scsi_transport_srp.c blocks the
targets, srp_rport_reconnect() in drivers/infiniband/ulp/srp/ib_srp.c
recreates RDMA channels and reinitializes ch->free_tx without locking under
the guarantee that queues are stopped:

	scsi_block_targets(shost, &shost->shost_gendev);
	res = rport->state != SRP_RPORT_LOST ? i->f->reconnect(rport) : -ENODEV;

Similarly, aacraid (aac_suspend(), aac_shutdown(), aac_pci_error_detected(),
and aac_reset_adapter() in drivers/scsi/aacraid/linit.c) relies on
scsi_host_block() to keep queues stopped while tearing down adapter resources.

2. If the host was blocked via scsi_host_block(), a subsequent
scsi_host_unblock() in drivers/scsi/scsi_lib.c stops iterating on the first
device where scsi_internal_device_unblock() returns an error:

	shost_for_each_device(sdev, shost) {
		ret = scsi_internal_device_unblock(sdev, new_state);
		if (ret) {
			scsi_device_put(sdev);
			break;
		}
	}

Because scsi_internal_device_unblock_nowait() returns -EINVAL when
sdev->sdev_state is SDEV_RUNNING, unblocking the host will abort on this
device and leave any remaining devices on the host stuck in SDEV_BLOCK with
stopped queues.

3. When transport recovery times out (such as session_recovery_timedout() in
drivers/scsi/scsi_transport_iscsi.c calling scsi_target_unblock(&session->dev,
SDEV_TRANSPORT_OFFLINE)), scsi_internal_device_unblock_nowait() rejects
SDEV_RUNNING with -EINVAL, leaving the device in SDEV_RUNNING on a timed-out
session instead of transitioning it to SDEV_TRANSPORT_OFFLINE.

4. On iSCSI and FC, while the session or rport is blocked,
iscsi_session_chkready() and fc_remote_port_chkready() return
DID_IMM_RETRY << 16. Once error recovery has finished
(!scsi_host_in_recovery(cmd->device->host)), __scsi_queue_insert() in
drivers/scsi/scsi_lib.c requeues the command and kicks requeue_work
immediately:

	blk_mq_requeue_request(scsi_cmd_to_rq(cmd),
			       !scsi_host_in_recovery(cmd->device->host));

With the queue unquiesced by store_state_field(), won't the rescan I/O (and
any other queued requests) spin in a tight zero-delay requeue loop until the
transport unblocks or the command runtime expires while holding the device
lock in scsi_rescan_device()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030164229.10037-1-brian@purestorage.com?part=1