Re: [PATCH v8 0/5] scsi: Refresh INQUIRY data and reprobe on rescan

[Brian Bunker <brian@purestorage.com>]

On 10/19/26 3:41 PM, Bart Van Assche wrote:
> Some feedback from an experimental LLM can be found below. Please take a
> look.

Hi Bart,

Thanks for the detailed review. Before I post v9, I want to answer the
design question so that v9 isn't heading in a direction you and Martin
don't want.

> The open question: a target can now remove a disk that is bound and in
> use. [...] My suggestion is to only reprobe when no driver is bound
> (PQ 1 -> 0), but the author should say which direction the ALUA case
> actually needs.

Both directions. Let me describe the setup that motivates this, because
it isn't a short interruption.

Pure ActiveCluster is two separate arrays that present the same volume.
The volume is synchronously replicated between them, and each array
exposes it through its own front-end ports, which are in their own
target port groups. A host sees paths to both arrays as one multipath
device.

When the replication link between the arrays breaks, one array keeps
serving the volume and the other must stop, because it can no longer
mirror writes or persistent reservation changes to its peer. Its
front-end ports stay up, but its paths must not serve I/O. The array
reports those target port groups as unavailable. Unavailable is the
state we can use here, because its required command set is one we can
fully support without the link: we don't have to accept reservations
we cannot mirror. This lasts as long as the link is down, which can be
much longer than a microcode download.

> - Cite the SPC text that requires a PQ change across the ALUA
> unavailable state.

You're right that the citation in 4/5 is wrong; it should be SPC-4
5.15.2.4.5, not 5.15.2.4.4. That section says that while in the
unavailable state the device server shall support:

  a) INQUIRY (the peripheral qualifier (see 6.6.2) shall be set to
     001b);

So the PQ change isn't something we chose: once the array uses
unavailable, the standard requires PQ = 001b. We set it to 1 on
entering unavailable and back to 0 when we leave it.

Why I think Linux should honor it in both directions: today the
outcome depends on when the host looked, not on what the target
reports.

  - A host that scans while the link is down sees PQ = 1 on those
    paths. scsi_bus_match() doesn't bind sd, so they are not disks.
    When the link comes back and PQ returns to 0, nothing binds sd, and
    those paths stay unusable until someone deletes and rescans the
    device. That is the bug this series started from.

  - A host that was already running keeps sd bound on the same paths
    for as long as the link is down.

Reprobing on a PQ change in either direction makes the binding follow
the PQ the target reports, which is the rule the scan path already
applies. With dm-multipath, the unavailable paths leave the map while
the link is down and come back as new paths afterwards, while the
other array's paths keep serving I/O. Without multipath, a disk whose
only path is unavailable cannot do I/O anyway, and it reappears when
PQ returns to 0.

If you or Martin would still rather not unbind a bound disk by
default, please say so now and I'll rework v9 around that.

> Offlining can't be undone: [...] I'd drop the offline step.

Agreed. Hannes, you asked in the v4 review for more than a warning when
device_reprobe() fails, which is where the SDEV_OFFLINE step came from.
But Bart is right: scsi_rescan_device() only acts on SDEV_RUNNING
devices, so an offlined device never comes back through a later
rescan, and failing to bind a ULD is not an I/O error. For v9 I plan to
just log a real reprobe failure and not treat -EPROBE_DEFER as one.
Please shout if you see that differently.

> 5/5 widens rescan behaviour a lot.

5/5 is needed for FC, but you're right that it does far too much. The
two directions reach the host differently:

  - Entering unavailable, the array reports INQUIRY DATA HAS CHANGED
    (along with the ALUA state change). sd is bound and I/O is flowing,
    so the host sees the UNIT ATTENTION on its next command, and
    scsi_evt_thread() -> scsi_rescan_device() handles it. That is 4/5.

  - Leaving unavailable, a UNIT ATTENTION alone doesn't work. With
    PQ = 1 nothing is bound to the LUN and no commands reach it, so
    nothing ever sees the UNIT ATTENTION. So the array also makes the
    host rescan, the same way it does when the first volume is mapped
    to a host.

    On iSCSI it sends an asynchronous SCSI event with REPORTED LUNS
    DATA HAS CHANGED. iscsid writes to the sysfs rescan attribute of
    every existing device on the session, which is scsi_rescan_device()
    and therefore 4/5, and then scans the host for new LUNs.

    On FC there is no equivalent, so it logs out the host's initiators.
    After the re-login, fc_scsi_scan_rport() calls
    scsi_scan_target(..., SCSI_SCAN_RESCAN), and that scan finds the
    existing scsi_device. Without 5/5 it does nothing with it, PQ = 0
    is never seen, and sd never binds.

So the scan path only has to handle LUNs that can't receive a UNIT
ATTENTION, which are the ones with no driver bound. For v9 I plan to
limit 5/5 to existing devices that are registered and have no driver
bound. A bound LUN learns about INQUIRY changes through UNIT ATTENTIONs
on its own I/O, so the scan path leaves it alone. On a normal host that
skips nearly every LUN, which removes the per-scan cost and the
scan_mutex hold time you described. It also avoids the double LUN 0
rescan, except in the rare case where LUN 0 itself has no driver bound.

For the rest, v9 will:

  - check device_is_registered() before reprobing, so a rescan before
    scsi_sysfs_add_sdev() can't reach __device_attach() with dev->p
    still NULL;
  - keep the LLD's INQUIRY length limit separate from the last response
    length, so scsi_probe_lun() behaves the same on a refresh as on the
    first probe;
  - when a refresh returns PQ 3, PQ 1 with PDT 0x1f, or a different
    vendor/model, warn and keep the existing data rather than accept
    a response that the scan path would treat differently;
  - merge 2/5 and 3/5, declare scsi_update_inquiry_data() in
    scsi_priv.h, and keep the comment explaining why a PQ = 1 device is
    still created;
  - drop cdl_supported from the INQUIRY-derived list in 1/5 and limit
    its claims to what the mutex actually protects;
  - say in the 4/5 commit message that every scsi_rescan_device()
    caller now sends an extra INQUIRY;
  - address the smaller points (0444, a shared constant instead of 256,
    the explicit first-call argument, the stale comments).

Thanks,

Brian