| Message ID | 20231017100729.123506-2-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: EH rework prep patches, part 2 | expand |
Hey Hannes, On Tue, Oct 17, 2023 at 12:07:14PM +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. > So this patch removes the call to fc_block_rport() after host > reset. But with that rports may still be in blocked state after > host reset, so we need to return FAST_IO_FAIL from host reset > to avoid SCSI EH to fail commands prematurely if the rports > are still blocked. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > Cc: Steffen Maier <maier@linux.ibm.com > Cc: Benjamin Block <bblock@linux.ibm.com> > --- > drivers/s390/scsi/zfcp_scsi.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c > index b2a8cd792266..b1df853e6f66 100644 > --- a/drivers/s390/scsi/zfcp_scsi.c > +++ b/drivers/s390/scsi/zfcp_scsi.c > @@ -375,7 +375,7 @@ 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; > + int ret = FAST_IO_FAIL; One thing I noticed, `scsi_ioctl_reset()` doesn't handle FAST_IO_FAIL at all; it just considers that a failure, and returns EIO to userspace. I wonder if this is appropriate after what we've discussed. In our case FAST_IO_FAIL doesn't mean the host reset failed. I don't think we need to adapt this patch for that - couldn't really - but maybe there should be an extra change for that. Not exactly sure what even uses that IOCTL interface, and how it'd react to an EIO. > > if (!(adapter->connection_features & FSF_FEATURE_NPIV_MODE)) { > zfcp_erp_port_forced_reopen_all(adapter, 0, "schrh_p"); > @@ -383,10 +383,6 @@ 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; > - > zfcp_dbf_scsi_eh("schrh_r", adapter, ~0, ret); > return ret; > } I think this works for us. Thanks. Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
On 10/19/23 19:44, Benjamin Block wrote: > Hey Hannes, > > On Tue, Oct 17, 2023 at 12:07:14PM +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. >> So this patch removes the call to fc_block_rport() after host >> reset. But with that rports may still be in blocked state after >> host reset, so we need to return FAST_IO_FAIL from host reset >> to avoid SCSI EH to fail commands prematurely if the rports >> are still blocked. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> Cc: Steffen Maier <maier@linux.ibm.com >> Cc: Benjamin Block <bblock@linux.ibm.com> >> --- >> drivers/s390/scsi/zfcp_scsi.c | 6 +----- >> 1 file changed, 1 insertion(+), 5 deletions(-) >> >> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c >> index b2a8cd792266..b1df853e6f66 100644 >> --- a/drivers/s390/scsi/zfcp_scsi.c >> +++ b/drivers/s390/scsi/zfcp_scsi.c >> @@ -375,7 +375,7 @@ 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; >> + int ret = FAST_IO_FAIL; > > One thing I noticed, `scsi_ioctl_reset()` doesn't handle FAST_IO_FAIL at all; > it just considers that a failure, and returns EIO to userspace. I wonder if > this is appropriate after what we've discussed. In our case FAST_IO_FAIL > doesn't mean the host reset failed. > Hmm. Indeed, that is not quite correct. EIO indicates an IO error, whereas FAST_IO_FAIL signal more something like 'operation in progress, couldn't complete in time'. So I guess EAGAIN is more appropriate here. > I don't think we need to adapt this patch for that - couldn't really - but > maybe there should be an extra change for that. Not exactly sure what even > uses that IOCTL interface, and how it'd react to an EIO. > Yes, we should be fixing that one. After all, FAST_IO_FAIL is a perfectly legit return value for the EH functions, and we should be handling it properly. Will be adding a patch for it. Thanks for the heads-up. Cheers, Hannes
diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c index b2a8cd792266..b1df853e6f66 100644 --- a/drivers/s390/scsi/zfcp_scsi.c +++ b/drivers/s390/scsi/zfcp_scsi.c @@ -375,7 +375,7 @@ 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; + int ret = FAST_IO_FAIL; if (!(adapter->connection_features & FSF_FEATURE_NPIV_MODE)) { zfcp_erp_port_forced_reopen_all(adapter, 0, "schrh_p"); @@ -383,10 +383,6 @@ 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; - zfcp_dbf_scsi_eh("schrh_r", adapter, ~0, ret); return ret; }
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. So this patch removes the call to fc_block_rport() after host reset. But with that rports may still be in blocked state after host reset, so we need to return FAST_IO_FAIL from host reset to avoid SCSI EH to fail commands prematurely if the rports are still blocked. Signed-off-by: Hannes Reinecke <hare@suse.de> Cc: Steffen Maier <maier@linux.ibm.com Cc: Benjamin Block <bblock@linux.ibm.com> --- drivers/s390/scsi/zfcp_scsi.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)