| Message ID | 20220502213820.3187-4-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: EH rework prep patches, part 1 | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
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/ > + 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. > + > + 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; > + } > > zfcp_dbf_scsi_eh("schrh_r", adapter, ~0, ret); > return ret;
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
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) { + 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; + } zfcp_dbf_scsi_eh("schrh_r", adapter, ~0, ret); return ret;
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(-)