On 10/12/23 15:54, Benjamin Block wrote:
Hey Hannes,
I've got a few questions re the rational for this change.
On Mon, Oct 02, 2023 at 05:49:13PM +0200, Hannes Reinecke wrote:
zfcp_scsi_eh_host_reset_handler() would call fc_block_rport() to
wait for all rports to become unblocked after host reset.
But after host reset it might happen that the port is gone, hence
fc_block_rport() might fail due to a missing port.
But that's a perfectly legal operation; on FC remote ports might
come and go.
In the same vein FC HBAs are able to deal with ports being temporarily
blocked, so really there is not point in waiting for all ports
to become unblocked during host reset.
But in scsi_transport_fc.c we have this documented:
* fc_block_scsi_eh - Block SCSI eh thread for blocked fc_rport
* @cmnd: SCSI command that scsi_eh is trying to recover
*
* This routine can be called from a FC LLD scsi_eh callback. It
* blocks the scsi_eh thread until the fc_rport leaves the
* FC_PORTSTATE_BLOCKED, or the fast_io_fail_tmo fires. This is
* necessary to avoid the scsi_eh failing recovery actions for blocked
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
* rports which would lead to offlined SCSI devices.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
So I don't understand what the real expectation by the SCSI EH call back for
host reset is then.
Is it that all objects (host/target ports/luns) are operational again once we
return to the EH thread, or is it ok that some parts are still being
recovered (as with our host reset handler, rports might still be blocked after
`zfcp_erp_wait()` finishes, because of how this is organized internally).
If it's the later, I'd think this change is fine. But then I'd wonder why this
function exists in the first place? Is it because in other EH steps it's more
important that rports are ready after the step (e.g. because a TUR is send
after, and if that fails, things get escalate unnecessarily)?
Thing is, fc_block_scsi_eh() is assumed to be called from eh callbacks
_before_ any TMFs are to be sent.
Typically you would call them in eh_device_reset() or eh_target_reset()
to ensure that you can sent TMFs in the first place; no point in attempting
to send TMFs is the port is blocked.
Your particular case is arguably a mis-use of fc_block_scsi_eh() as
it is called _after_ host reset is initiated, essentially serving as
a completion point to ensure that all rports are back online.
However, for the FC transport implementation rport lifetimes are
decoupled from SCSI Host lifetimes; rports may (and do!) come and
go during the lifetime of a SCSI host. Consequently there is no
difference between a host with all rports blocked (eg during RSCN
processing) and a host just coming on-line after SCSI EH where rports
are still in the process of getting ready.
Hence the use of fc_block_scsi_eh() after host reset is not required,
and we can make our life easier by just dropping the call.
Oh.. speaking of that, we do send a TUR after host reset as well
(`scsi_eh_test_devices()`). So doesn't this break then if one or more rports
are sill blocked after host reset returns?
At least `zfcp_scsi_queuecommand()` will bail very early if the rport is
not ready (we call `fc_remote_port_chkready()` as more or less first thing),
and so `scsi_send_eh_cmnd()` that is used for the TUR will fail; then it might
be retried one time, but this is a tight loop without any delay, so I'd guess
this has a good chance to fail as well.
And then we'd offline the whole host as further escalation, which would
*REALLY* suck (with no automatic recovery no less).
My impression from look at the code that follows `scsi_try_host_reset()` in
`scsi_error.c` really is, it rather expects things to be ready to be used
after, right there and then (admittedly, this is probably already today
problematic, as things might go back to not working concurrently because of
some fabric event.. but anyway, we can life with that off-chance it seems).
Or do I miss something?
Ah, right. True, when the rports are not ready (ie still being blocked)
sending a TEST UNIT READY will fail, with probably unintended consequences.
But: if host reset would return FAST_IO_FAIL everything would be dandy
as then we would just check if the devices are online (by virtue of
scsi_eh_flush_done_q() in scsi_unjam_host()), which they really should
as no-one should have set them offline by then.
So I guess that's the correct way to go.
Will be modifying the patch accordingly.
Thanks for the feedback!
Cheers,
Hannes
