| Message ID | 20210817091456.73342-9-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | SCSI EH argument reshuffle part II | expand |
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.
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
On 8/17/21 2:54 PM, Hannes Reinecke wrote: > 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? Waiting for all rports to unblock in host_reset has been on my todo list since we prepared the eh callbacks to get rid of scsi_cmnd with v4.18 commits: 674595d8519f ("scsi: zfcp: decouple our scsi_eh callbacks from scsi_cmnd") 42afc6527d43 ("scsi: zfcp: decouple TMFs from scsi_cmnd by using fc_block_rport") 26f5fa9d47c1 ("scsi: zfcp: decouple SCSI setup of TMF from scsi_cmnd") 39abb11aca00 ("scsi: zfcp: decouple FSF request setup of TMF from scsi_cmnd") e0116c91c7d8 ("scsi: zfcp: split FCP_CMND IU setup between SCSI I/O and TMF again") 266883f2f7d5 ("scsi: zfcp: decouple TMF response handler from scsi_cmnd") 822121186375 ("scsi: zfcp: decouple SCSI traces for scsi_eh / TMF from scsi_cmnd") But the synchronization is non-trivial as Benjamin's question shows. There are also considerations about lock order, etc. I'm busy with other things, so don't hold your breath until I can review and test the code; I don't want any regression in that recovery code. >> 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). There is more to do in zfcp than in other FC HBA drivers, e.g. LUN open recoveries and how they related to rport unblock: v4.10 6f2ce1c6af37 ("scsi: zfcp: fix rport unblock race with LUN recovery"). The rport unblock is async to our internal recovery. zfcp_erp_wait() only waits for the latter by design. > So can't we just drop the fc_block_rport() call here? I don't think so. > All the other FC drivers do fine without that ... It would have been nice to have a common interface for all scsi_eh scopes. I.e. fc_block_host(struct Scsi_Host*) like we already have for fc_block_scsi_eh(struct scsi_cmnd*) and fc_block_rport(struct fc_rport*) [the latter having been introduced at the time of above eh callback preparations]. But if zfcp is the only one needing it for host_reset, having the code only in zfcp seems fine to me.
On 8/17/21 4:03 PM, Steffen Maier wrote: > On 8/17/21 2:54 PM, Hannes Reinecke wrote: >> 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? > > Waiting for all rports to unblock in host_reset has been on my todo list > since we prepared the eh callbacks to get rid of scsi_cmnd with v4.18 > commits: > 674595d8519f ("scsi: zfcp: decouple our scsi_eh callbacks from scsi_cmnd") > 42afc6527d43 ("scsi: zfcp: decouple TMFs from scsi_cmnd by using > fc_block_rport") > 26f5fa9d47c1 ("scsi: zfcp: decouple SCSI setup of TMF from scsi_cmnd") > 39abb11aca00 ("scsi: zfcp: decouple FSF request setup of TMF from > scsi_cmnd") > e0116c91c7d8 ("scsi: zfcp: split FCP_CMND IU setup between SCSI I/O and > TMF again") > 266883f2f7d5 ("scsi: zfcp: decouple TMF response handler from scsi_cmnd") > 822121186375 ("scsi: zfcp: decouple SCSI traces for scsi_eh / TMF from > scsi_cmnd") > > But the synchronization is non-trivial as Benjamin's question shows. > There are also considerations about lock order, etc. > > I'm busy with other things, so don't hold your breath until I can review > and test the code; I don't want any regression in that recovery code. > >>> 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). > > There is more to do in zfcp than in other FC HBA drivers, e.g. LUN open > recoveries and how they related to rport unblock: > v4.10 6f2ce1c6af37 ("scsi: zfcp: fix rport unblock race with LUN > recovery"). > The rport unblock is async to our internal recovery. zfcp_erp_wait() > only waits for the latter by design. > >> So can't we just drop the fc_block_rport() call here? > > I don't think so. > >> All the other FC drivers do fine without that ... > > It would have been nice to have a common interface for all scsi_eh > scopes. I.e. fc_block_host(struct Scsi_Host*) like we already have for > fc_block_scsi_eh(struct scsi_cmnd*) and fc_block_rport(struct fc_rport*) > [the latter having been introduced at the time of above eh callback > preparations]. > But if zfcp is the only one needing it for host_reset, having the code > only in zfcp seems fine to me. > > Right. Just wanted to clarify that. If we need to use fc_block_rport() in host reset so be it; just wanted to clarify if this _really_ is the case (and not just some copy'n'paste stuff). I'll be reworking the patch to call fc_block_rport(). Cheers, Hannes
On 8/17/21 4:10 PM, Hannes Reinecke wrote: > On 8/17/21 4:03 PM, Steffen Maier wrote: >> On 8/17/21 2:54 PM, Hannes Reinecke wrote: >>> 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? >> >> Waiting for all rports to unblock in host_reset has been on my todo list >> since we prepared the eh callbacks to get rid of scsi_cmnd with v4.18 >> commits: >> 674595d8519f ("scsi: zfcp: decouple our scsi_eh callbacks from scsi_cmnd") >> 42afc6527d43 ("scsi: zfcp: decouple TMFs from scsi_cmnd by using >> fc_block_rport") >> 26f5fa9d47c1 ("scsi: zfcp: decouple SCSI setup of TMF from scsi_cmnd") >> 39abb11aca00 ("scsi: zfcp: decouple FSF request setup of TMF from >> scsi_cmnd") >> e0116c91c7d8 ("scsi: zfcp: split FCP_CMND IU setup between SCSI I/O and >> TMF again") >> 266883f2f7d5 ("scsi: zfcp: decouple TMF response handler from scsi_cmnd") >> 822121186375 ("scsi: zfcp: decouple SCSI traces for scsi_eh / TMF from >> scsi_cmnd") >> >> But the synchronization is non-trivial as Benjamin's question shows. >> There are also considerations about lock order, etc. >> >> I'm busy with other things, so don't hold your breath until I can review >> and test the code; I don't want any regression in that recovery code. >> >>>> 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). >> >> There is more to do in zfcp than in other FC HBA drivers, e.g. LUN open >> recoveries and how they related to rport unblock: >> v4.10 6f2ce1c6af37 ("scsi: zfcp: fix rport unblock race with LUN >> recovery"). >> The rport unblock is async to our internal recovery. zfcp_erp_wait() >> only waits for the latter by design. >> >>> So can't we just drop the fc_block_rport() call here? >> >> I don't think so. >> >>> All the other FC drivers do fine without that ... >> >> It would have been nice to have a common interface for all scsi_eh >> scopes. I.e. fc_block_host(struct Scsi_Host*) like we already have for >> fc_block_scsi_eh(struct scsi_cmnd*) and fc_block_rport(struct fc_rport*) >> [the latter having been introduced at the time of above eh callback >> preparations]. >> But if zfcp is the only one needing it for host_reset, having the code >> only in zfcp seems fine to me. >> >> > Right. Just wanted to clarify that. > If we need to use fc_block_rport() in host reset so be it; just wanted > to clarify if this _really_ is the case (and not just some copy'n'paste > stuff). > I'll be reworking the patch to call fc_block_rport(). On second thought, I might have been wrong. The argument I used with the old commit was that we must not unblock the rport too early with regards to zfcp-internal recovery. This is fixed within zfcp recovery (erp) code. So after zfcp_erp_wait() in host_reset, this is still ensured; and eventually the rport unblock will occur. I guess I was rather worried about returning from the host_reset callback with the async rport(s) unblock still pending. After all, (some) other reset_handler sync with rport unblock. However I cannot remember all details right now. Before you invest more time into this, maybe just drop this patch from the series for now and we solve it later on? I mean it's not necessary for the reset_handler function signature change.
On 8/18/21 1:00 PM, Steffen Maier wrote: > On 8/17/21 4:10 PM, Hannes Reinecke wrote: >> On 8/17/21 4:03 PM, Steffen Maier wrote: [ .. ] >>> It would have been nice to have a common interface for all scsi_eh >>> scopes. I.e. fc_block_host(struct Scsi_Host*) like we already have for >>> fc_block_scsi_eh(struct scsi_cmnd*) and fc_block_rport(struct fc_rport*) >>> [the latter having been introduced at the time of above eh callback >>> preparations]. >>> But if zfcp is the only one needing it for host_reset, having the code >>> only in zfcp seems fine to me. >>> >>> >> Right. Just wanted to clarify that. >> If we need to use fc_block_rport() in host reset so be it; just wanted >> to clarify if this _really_ is the case (and not just some copy'n'paste >> stuff). >> I'll be reworking the patch to call fc_block_rport(). > > On second thought, I might have been wrong. > > The argument I used with the old commit was that we must not unblock the > rport too early with regards to zfcp-internal recovery. This is fixed > within zfcp recovery (erp) code. So after zfcp_erp_wait() in host_reset, > this is still ensured; and eventually the rport unblock will occur. > > I guess I was rather worried about returning from the host_reset > callback with the async rport(s) unblock still pending. After all, > (some) other reset_handler sync with rport unblock. However I cannot > remember all details right now. > > Before you invest more time into this, maybe just drop this patch from > the series for now and we solve it later on? I mean it's not necessary > for the reset_handler function signature change. > Well, actually it is. With the signature change host_reset is being called with a Scsi_Host argument, so we cannot identify 'the' rport. But I've modified the patch to cycle through all rports and call fc_block_rport() on each of them. That should be good enough for now. Cheers, Hannes
diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c index 9da9b2b2a580..9393f1587e8a 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(-)