Re: [PATCH 03/24] zfcp: open-code fc_block_scsi_eh() for host reset

Hannes Reinecke
2022-05-03T18:42:30Z

On 5/3/22 10:21, Steffen Maier wrote:
> Hi Hannes,
>
> On 5/2/22 23:37, Hannes Reinecke wrote:
>> When issuing a host reset we should be waiting for all
>> ports to become unblocked; just waiting for one might
>> be resulting in host reset to return too early.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> Cc: Steffen Maier <maier@linux.ibm.com>
>> Cc: Benjamin Block <bblock@linux.ibm.com>
>> ---
>>   drivers/s390/scsi/zfcp_scsi.c | 29 +++++++++++++++++++++++------
>>   1 file changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/s390/scsi/zfcp_scsi.c
>> b/drivers/s390/scsi/zfcp_scsi.c
>> index 526ac240d9fe..fb2b73fca381 100644
>> --- a/drivers/s390/scsi/zfcp_scsi.c
>> +++ b/drivers/s390/scsi/zfcp_scsi.c
>> @@ -373,9 +373,11 @@ static int
>> zfcp_scsi_eh_target_reset_handler(struct scsi_cmnd *scpnt)
>>   static int zfcp_scsi_eh_host_reset_handler(struct scsi_cmnd *scpnt)
>>   {
>> -    struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scpnt->device);
>> -    struct zfcp_adapter *adapter = zfcp_sdev->port->adapter;
>> -    int ret = SUCCESS, fc_ret;
>> +    struct Scsi_Host *host = scpnt->device->host;
>> +    struct zfcp_adapter *adapter = (struct zfcp_adapter
>> *)host->hostdata[0];
>> +    int ret = SUCCESS;
>> +    unsigned long flags;
>> +    struct zfcp_port *port;
>>       if (!(adapter->connection_features & FSF_FEATURE_NPIV_MODE)) {
>>           zfcp_erp_port_forced_reopen_all(adapter, 0, "schrh_p");
>> @@ -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) {
>
> Reading adapter->port_list needs to be protected by
>     read_lock_irq(&adapter->port_list_lock);
>
> Cf. Benjamin's last review
> https://lore.kernel.org/linux-scsi/YRujHScPbb1Aokrj@t480-pf1aa2c2.linux.ibm.com/
>
>
Ah. Sorry. Will be including it in the next round.

>> +        struct fc_rport *rport = port->rport;
>
> While an rport is blocked, port->rport == NULL [zfcp_scsi_rport_block()
> and zfcp_scsi_rport_register()], so below could be a NULL pointer deref.
> Or is there evidence we would never have a blocked rport while iterating
> port_list here?
>
> See also my previous review comments
> https://lore.kernel.org/linux-scsi/a7950ea7-380c-bb01-7f31-5c555217ad2d@linux.vnet.ibm.com/
>
> It also alludes to lock ordering.
>
Right. Will be folding in the changes.

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer