| Message ID | 20220414014947.4168447-3-haowenchao@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix multiple iscsi session unbind event sent to userspace | expand |
On 4/13/22 8:49 PM, Wenchao Hao wrote: > __iscsi_unbind_session() set session state to ISCSI_SESSION_UNBOUND, which > would overwrite the ISCSI_SESSION_FREE state. > > Signed-off-by: Wenchao Hao <haowenchao@huawei.com> > --- > drivers/scsi/scsi_transport_iscsi.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c > index 97a9fee02efa..d8dd9279cea8 100644 > --- a/drivers/scsi/scsi_transport_iscsi.c > +++ b/drivers/scsi/scsi_transport_iscsi.c > @@ -2173,6 +2173,22 @@ void iscsi_remove_session(struct iscsi_cls_session *session) > if (!cancel_work_sync(&session->block_work)) > cancel_delayed_work_sync(&session->recovery_work); > cancel_work_sync(&session->unblock_work); > + > + scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE); > + /* > + * qla4xxx can perform it's own scans when it runs in kernel only > + * mode. Make sure to flush those scans. > + */ > + flush_work(&session->scan_work); > + > + /* > + * flush running unbind operations > + * if unbind work did not queued, call __iscsi_unbind_session > + * directly to perform target remove We probably don't need the flush_work test because we are going to normally call __iscsi_unbind_session. If the unbind work had already run, which is the normal case, then flush_work returns false and we end up calling __iscsi_unbind_session like before. That function then checks if the target is really unbound. So the extra check doesn't normally buy us anything with your patches because in patch 1 you fixed it so __iscsi_unbind_session doesn't send the extra event. > + */ > + if (!flush_work(&session->unbind_work)) > + __iscsi_unbind_session(&session->unbind_work); > + > /* > * If we are blocked let commands flow again. The lld or iscsi > * layer should set up the queuecommand to fail commands. > @@ -2183,16 +2199,6 @@ void iscsi_remove_session(struct iscsi_cls_session *session) > session->state = ISCSI_SESSION_FREE; > spin_unlock_irqrestore(&session->lock, flags); > > - scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE); > - /* > - * qla4xxx can perform it's own scans when it runs in kernel only > - * mode. Make sure to flush those scans. > - */ > - flush_work(&session->scan_work); > - /* flush running unbind operations */ > - flush_work(&session->unbind_work); > - __iscsi_unbind_session(&session->unbind_work); > - > /* hw iscsi may not have removed all connections from session */ > err = device_for_each_child(&session->dev, NULL, > iscsi_iter_destroy_conn_fn);
On 2022/4/14 23:30, Mike Christie wrote: > On 4/13/22 8:49 PM, Wenchao Hao wrote: >> __iscsi_unbind_session() set session state to ISCSI_SESSION_UNBOUND, which >> would overwrite the ISCSI_SESSION_FREE state. >> >> Signed-off-by: Wenchao Hao <haowenchao@huawei.com> >> --- >> drivers/scsi/scsi_transport_iscsi.c | 26 ++++++++++++++++---------- >> 1 file changed, 16 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c >> index 97a9fee02efa..d8dd9279cea8 100644 >> --- a/drivers/scsi/scsi_transport_iscsi.c >> +++ b/drivers/scsi/scsi_transport_iscsi.c >> @@ -2173,6 +2173,22 @@ void iscsi_remove_session(struct iscsi_cls_session *session) >> if (!cancel_work_sync(&session->block_work)) >> cancel_delayed_work_sync(&session->recovery_work); >> cancel_work_sync(&session->unblock_work); >> + >> + scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE); >> + /* >> + * qla4xxx can perform it's own scans when it runs in kernel only >> + * mode. Make sure to flush those scans. >> + */ >> + flush_work(&session->scan_work); >> + >> + /* >> + * flush running unbind operations >> + * if unbind work did not queued, call __iscsi_unbind_session >> + * directly to perform target remove > > We probably don't need the flush_work test because we are going to > normally call __iscsi_unbind_session. > I think we still need calling flush_work here. The introduce of flush_work is to make sure sysfs objects are removed in an correct order. There is a very low probability that __iscsi_unbind_session() triggered by queue_work() has not been finished, and iscsi_remove_session() is called. So we need flush_work() to make sure __iscsi_unbind_session() has done if it has been activated by queue_work(). > If the unbind work had already run, which is the normal case, then > flush_work returns false and we end up calling __iscsi_unbind_session > like before. That function then checks if the target is really unbound. > So the extra check doesn't normally buy us anything with your patches > because in patch 1 you fixed it so __iscsi_unbind_session doesn't send > the extra event. > > >> + */ >> + if (!flush_work(&session->unbind_work)) >> + __iscsi_unbind_session(&session->unbind_work); >> + >> /* >> * If we are blocked let commands flow again. The lld or iscsi >> * layer should set up the queuecommand to fail commands. >> @@ -2183,16 +2199,6 @@ void iscsi_remove_session(struct iscsi_cls_session *session) >> session->state = ISCSI_SESSION_FREE; >> spin_unlock_irqrestore(&session->lock, flags); >> >> - scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE); >> - /* >> - * qla4xxx can perform it's own scans when it runs in kernel only >> - * mode. Make sure to flush those scans. >> - */ >> - flush_work(&session->scan_work); >> - /* flush running unbind operations */ >> - flush_work(&session->unbind_work); >> - __iscsi_unbind_session(&session->unbind_work); >> - >> /* hw iscsi may not have removed all connections from session */ >> err = device_for_each_child(&session->dev, NULL, >> iscsi_iter_destroy_conn_fn); > > .
On 4/15/22 4:40 AM, Wenchao Hao wrote: > On 2022/4/14 23:30, Mike Christie wrote: >> On 4/13/22 8:49 PM, Wenchao Hao wrote: >>> __iscsi_unbind_session() set session state to ISCSI_SESSION_UNBOUND, which >>> would overwrite the ISCSI_SESSION_FREE state. >>> >>> Signed-off-by: Wenchao Hao <haowenchao@huawei.com> >>> --- >>> drivers/scsi/scsi_transport_iscsi.c | 26 ++++++++++++++++---------- >>> 1 file changed, 16 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c >>> index 97a9fee02efa..d8dd9279cea8 100644 >>> --- a/drivers/scsi/scsi_transport_iscsi.c >>> +++ b/drivers/scsi/scsi_transport_iscsi.c >>> @@ -2173,6 +2173,22 @@ void iscsi_remove_session(struct iscsi_cls_session *session) >>> if (!cancel_work_sync(&session->block_work)) >>> cancel_delayed_work_sync(&session->recovery_work); >>> cancel_work_sync(&session->unblock_work); >>> + >>> + scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE); >>> + /* >>> + * qla4xxx can perform it's own scans when it runs in kernel only >>> + * mode. Make sure to flush those scans. >>> + */ >>> + flush_work(&session->scan_work); >>> + >>> + /* >>> + * flush running unbind operations >>> + * if unbind work did not queued, call __iscsi_unbind_session >>> + * directly to perform target remove >> >> We probably don't need the flush_work test because we are going to >> normally call __iscsi_unbind_session. >> > > I think we still need calling flush_work here. The introduce of flush_work Above I was saying we don't need to *test* if it returned true/false. Just always flush and then call __iscsi_unbind_session like we did originally below. The check you added in __iscsi_unbind_session in the first patch will detect if it's been called or not. > is to make sure sysfs objects are removed in an correct order. There is a > very low probability that __iscsi_unbind_session() triggered by queue_work() > has not been finished, and iscsi_remove_session() is called. So we need > flush_work() to make sure __iscsi_unbind_session() has done if it has been > activated by queue_work(). > >> If the unbind work had already run, which is the normal case, then >> flush_work returns false and we end up calling __iscsi_unbind_session >> like before. That function then checks if the target is really unbound. >> So the extra check doesn't normally buy us anything with your patches >> because in patch 1 you fixed it so __iscsi_unbind_session doesn't send >> the extra event. >> >> >>> + */ >>> + if (!flush_work(&session->unbind_work)) >>> + __iscsi_unbind_session(&session->unbind_work); >>> + >>> /* >>> * If we are blocked let commands flow again. The lld or iscsi >>> * layer should set up the queuecommand to fail commands. >>> @@ -2183,16 +2199,6 @@ void iscsi_remove_session(struct iscsi_cls_session *session) >>> session->state = ISCSI_SESSION_FREE; >>> spin_unlock_irqrestore(&session->lock, flags); >>> >>> - scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE); >>> - /* >>> - * qla4xxx can perform it's own scans when it runs in kernel only >>> - * mode. Make sure to flush those scans. >>> - */ >>> - flush_work(&session->scan_work); >>> - /* flush running unbind operations */ >>> - flush_work(&session->unbind_work); >>> - __iscsi_unbind_session(&session->unbind_work); >>> - >>> /* hw iscsi may not have removed all connections from session */ >>> err = device_for_each_child(&session->dev, NULL, >>> iscsi_iter_destroy_conn_fn); >> >> . >
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c index 97a9fee02efa..d8dd9279cea8 100644 --- a/drivers/scsi/scsi_transport_iscsi.c +++ b/drivers/scsi/scsi_transport_iscsi.c @@ -2173,6 +2173,22 @@ void iscsi_remove_session(struct iscsi_cls_session *session) if (!cancel_work_sync(&session->block_work)) cancel_delayed_work_sync(&session->recovery_work); cancel_work_sync(&session->unblock_work); + + scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE); + /* + * qla4xxx can perform it's own scans when it runs in kernel only + * mode. Make sure to flush those scans. + */ + flush_work(&session->scan_work); + + /* + * flush running unbind operations + * if unbind work did not queued, call __iscsi_unbind_session + * directly to perform target remove + */ + if (!flush_work(&session->unbind_work)) + __iscsi_unbind_session(&session->unbind_work); + /* * If we are blocked let commands flow again. The lld or iscsi * layer should set up the queuecommand to fail commands. @@ -2183,16 +2199,6 @@ void iscsi_remove_session(struct iscsi_cls_session *session) session->state = ISCSI_SESSION_FREE; spin_unlock_irqrestore(&session->lock, flags); - scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE); - /* - * qla4xxx can perform it's own scans when it runs in kernel only - * mode. Make sure to flush those scans. - */ - flush_work(&session->scan_work); - /* flush running unbind operations */ - flush_work(&session->unbind_work); - __iscsi_unbind_session(&session->unbind_work); - /* hw iscsi may not have removed all connections from session */ err = device_for_each_child(&session->dev, NULL, iscsi_iter_destroy_conn_fn);
__iscsi_unbind_session() set session state to ISCSI_SESSION_UNBOUND, which would overwrite the ISCSI_SESSION_FREE state. Signed-off-by: Wenchao Hao <haowenchao@huawei.com> --- drivers/scsi/scsi_transport_iscsi.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)