On 8/17/21 1:53 PM, Benjamin Block wrote:
> On Tue, Aug 17, 2021 at 11:14:13AM +0200, Hannes Reinecke wrote:
>> @@ -383,9 +385,24 @@ static int zfcp_scsi_eh_host_reset_handler(struct scsi_cmnd *scpnt)
>> }
>> zfcp_erp_adapter_reopen(adapter, 0, "schrh_1");
>> zfcp_erp_wait(adapter);
>> - fc_ret = fc_block_scsi_eh(scpnt);
>> - if (fc_ret)
>> - ret = fc_ret;
>> +retry_rport_blocked:
>> + spin_lock_irqsave(host->host_lock, flags);
>> + list_for_each_entry(port, &adapter->port_list, list) {
>
> You need to take the `adapter->port_list_lock` to iterate over the `port_list`.
>
> i.e.: read_lock_irqsave(&adapter->port_list_lock, flags);
>
>> + struct fc_rport *rport = port->rport;
>> +
>> + if (rport->port_state == FC_PORTSTATE_BLOCKED) {
>> + if (rport->flags & FC_RPORT_FAST_FAIL_TIMEDOUT)
>> + ret = FAST_IO_FAIL;
>> + else
>> + ret = NEEDS_RETRY;
>> + break;
>> + }
>> + }
>> + spin_unlock_irqrestore(host->host_lock, flags);
>> + if (ret == NEEDS_RETRY) {
>> + msleep(1000);
>> + goto retry_rport_blocked;
>> + }
>
> I really can't say I like this open coded FC code in the driver at all.
>
> Is there a reason we can't use `fc_block_rport()` for all the rports of
> the adapter?
>
> We already do use it for other EH callbacks in the same file, and you
> already look up the rports in the adapters rport-list; so using that on
> the rports in the loop, instead of open-coding it doesn't seem bad? Or
> is there a locking problem?
>
> We might waste a few cycles with that, but frankly, this is all in EH
> and after adapter reset.. all performance concerns went our of the
> window with that already.
>
Question would be why we need to call fc_block_rport() at all in host reset.
To my understanding a host reset is expected to do a full resync of the
SAN topology, so the expectation is that after zfcp_erp_wait() the port
list is stable (ie the HBA has finished processing all RSCNs related to
the SAN resync).
So can't we just drop the fc_block_rport() call here?
All the other FC drivers do fine without that ...
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
