| Message ID | 20220414014947.4168447-1-haowenchao@huawei.com |
|---|---|
| Headers | show |
| Series | Fix multiple iscsi session unbind event sent to userspace | expand |
On 4/13/22 8:49 PM, Wenchao Hao wrote: > Fix the issue of kernel send multiple ISCSI_KEVENT_UNBIND_SESSION event. > If session is in UNBOUND state, do not perform unbind operations anymore, > else unbind session and set session to UNBOUND state. > I don't think we want this to be a state because you can have a session with no target or it could be partially deleted and it could be in the logged in or failed state. If scsi-ml is sending SYNC_CACHEs as part of the target/device removal operation, and we lose the session then we could go through recovery and the state will go from failed to logged in, and your new unbound state will have been overwritten. I think it might be better to have a new sysfs file, target_state, for this where you could have values like scanning, bound, unbinding, and unbound, or just a sysfs file, target_bound, that is bool. > Reference:https://github.com/open-iscsi/open-iscsi/issues/338 > You should add a description of the problem in the commit, because that link might be gone one day. > Signed-off-by: Wenchao Hao <haowenchao@huawei.com> > --- > drivers/scsi/scsi_transport_iscsi.c | 19 +++++++++++++++++-- > include/scsi/scsi_transport_iscsi.h | 1 + > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c > index 27951ea05dd4..97a9fee02efa 100644 > --- a/drivers/scsi/scsi_transport_iscsi.c > +++ b/drivers/scsi/scsi_transport_iscsi.c > @@ -1656,6 +1656,7 @@ static struct { > { ISCSI_SESSION_LOGGED_IN, "LOGGED_IN" }, > { ISCSI_SESSION_FAILED, "FAILED" }, > { ISCSI_SESSION_FREE, "FREE" }, > + { ISCSI_SESSION_UNBOUND, "UNBOUND" }, > }; > > static const char *iscsi_session_state_name(int state) > @@ -1686,6 +1687,9 @@ int iscsi_session_chkready(struct iscsi_cls_session *session) > case ISCSI_SESSION_FREE: > err = DID_TRANSPORT_FAILFAST << 16; > break; > + case ISCSI_SESSION_UNBOUND: > + err = DID_NO_CONNECT << 16; > + break; > default: > err = DID_NO_CONNECT << 16; > break; > @@ -1838,7 +1842,8 @@ int iscsi_block_scsi_eh(struct scsi_cmnd *cmd) > > spin_lock_irqsave(&session->lock, flags); > while (session->state != ISCSI_SESSION_LOGGED_IN) { > - if (session->state == ISCSI_SESSION_FREE) { > + if ((session->state == ISCSI_SESSION_FREE) || > + (session->state == ISCSI_SESSION_UNBOUND)) { > ret = FAST_IO_FAIL; > break; > } > @@ -1869,6 +1874,7 @@ static void session_recovery_timedout(struct work_struct *work) > break; > case ISCSI_SESSION_LOGGED_IN: > case ISCSI_SESSION_FREE: > + case ISCSI_SESSION_UNBOUND: > /* we raced with the unblock's flush */ > spin_unlock_irqrestore(&session->lock, flags); > return; > @@ -1957,6 +1963,14 @@ static void __iscsi_unbind_session(struct work_struct *work) > unsigned long flags; > unsigned int target_id; > > + spin_lock_irqsave(&session->lock, flags); > + if (session->state == ISCSI_SESSION_UNBOUND) { > + spin_unlock_irqrestore(&session->lock, flags); > + return; > + } > + session->state = ISCSI_SESSION_UNBOUND; > + spin_unlock_irqrestore(&session->lock, flags); > + > ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n"); > > /* Prevent new scans and make sure scanning is not in progress */ > @@ -4329,7 +4343,8 @@ store_priv_session_##field(struct device *dev, \ > struct iscsi_cls_session *session = \ > iscsi_dev_to_session(dev->parent); \ > if ((session->state == ISCSI_SESSION_FREE) || \ > - (session->state == ISCSI_SESSION_FAILED)) \ > + (session->state == ISCSI_SESSION_FAILED) || \ > + (session->state == ISCSI_SESSION_UNBOUND)) \ > return -EBUSY; \ > if (strncmp(buf, "off", 3) == 0) { \ > session->field = -1; \ > diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h > index 38e4a67f5922..80149643cbcd 100644 > --- a/include/scsi/scsi_transport_iscsi.h > +++ b/include/scsi/scsi_transport_iscsi.h > @@ -232,6 +232,7 @@ enum { > ISCSI_SESSION_LOGGED_IN, > ISCSI_SESSION_FAILED, > ISCSI_SESSION_FREE, > + ISCSI_SESSION_UNBOUND, > }; > > #define ISCSI_MAX_TARGET -1
On 2022/4/14 23:22, Mike Christie wrote: > On 4/13/22 8:49 PM, Wenchao Hao wrote: >> Fix the issue of kernel send multiple ISCSI_KEVENT_UNBIND_SESSION event. >> If session is in UNBOUND state, do not perform unbind operations anymore, >> else unbind session and set session to UNBOUND state. >> > > I don't think we want this to be a state because you can have a session > with no target or it could be partially deleted and it could be in the > logged in or failed state. If scsi-ml is sending SYNC_CACHEs as part of > the target/device removal operation, and we lose the session then we could > go through recovery and the state will go from failed to logged in, and > your new unbound state will have been overwritten. > > I think it might be better to have a new sysfs file, target_state, for > this where you could have values like scanning, bound, unbinding, and > unbound, or just a sysfs file, target_bound, that is bool. > Thanks for your review, I would modify and send another patch. >> Reference:https://github.com/open-iscsi/open-iscsi/issues/338 >> > > You should add a description of the problem in the commit, because that > link might be gone one day. > > >> Signed-off-by: Wenchao Hao <haowenchao@huawei.com> >> --- >> drivers/scsi/scsi_transport_iscsi.c | 19 +++++++++++++++++-- >> include/scsi/scsi_transport_iscsi.h | 1 + >> 2 files changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c >> index 27951ea05dd4..97a9fee02efa 100644 >> --- a/drivers/scsi/scsi_transport_iscsi.c >> +++ b/drivers/scsi/scsi_transport_iscsi.c >> @@ -1656,6 +1656,7 @@ static struct { >> { ISCSI_SESSION_LOGGED_IN, "LOGGED_IN" }, >> { ISCSI_SESSION_FAILED, "FAILED" }, >> { ISCSI_SESSION_FREE, "FREE" }, >> + { ISCSI_SESSION_UNBOUND, "UNBOUND" }, >> }; >> >> static const char *iscsi_session_state_name(int state) >> @@ -1686,6 +1687,9 @@ int iscsi_session_chkready(struct iscsi_cls_session *session) >> case ISCSI_SESSION_FREE: >> err = DID_TRANSPORT_FAILFAST << 16; >> break; >> + case ISCSI_SESSION_UNBOUND: >> + err = DID_NO_CONNECT << 16; >> + break; >> default: >> err = DID_NO_CONNECT << 16; >> break; >> @@ -1838,7 +1842,8 @@ int iscsi_block_scsi_eh(struct scsi_cmnd *cmd) >> >> spin_lock_irqsave(&session->lock, flags); >> while (session->state != ISCSI_SESSION_LOGGED_IN) { >> - if (session->state == ISCSI_SESSION_FREE) { >> + if ((session->state == ISCSI_SESSION_FREE) || >> + (session->state == ISCSI_SESSION_UNBOUND)) { >> ret = FAST_IO_FAIL; >> break; >> } >> @@ -1869,6 +1874,7 @@ static void session_recovery_timedout(struct work_struct *work) >> break; >> case ISCSI_SESSION_LOGGED_IN: >> case ISCSI_SESSION_FREE: >> + case ISCSI_SESSION_UNBOUND: >> /* we raced with the unblock's flush */ >> spin_unlock_irqrestore(&session->lock, flags); >> return; >> @@ -1957,6 +1963,14 @@ static void __iscsi_unbind_session(struct work_struct *work) >> unsigned long flags; >> unsigned int target_id; >> >> + spin_lock_irqsave(&session->lock, flags); >> + if (session->state == ISCSI_SESSION_UNBOUND) { >> + spin_unlock_irqrestore(&session->lock, flags); >> + return; >> + } >> + session->state = ISCSI_SESSION_UNBOUND; >> + spin_unlock_irqrestore(&session->lock, flags); >> + >> ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n"); >> >> /* Prevent new scans and make sure scanning is not in progress */ >> @@ -4329,7 +4343,8 @@ store_priv_session_##field(struct device *dev, \ >> struct iscsi_cls_session *session = \ >> iscsi_dev_to_session(dev->parent); \ >> if ((session->state == ISCSI_SESSION_FREE) || \ >> - (session->state == ISCSI_SESSION_FAILED)) \ >> + (session->state == ISCSI_SESSION_FAILED) || \ >> + (session->state == ISCSI_SESSION_UNBOUND)) \ >> return -EBUSY; \ >> if (strncmp(buf, "off", 3) == 0) { \ >> session->field = -1; \ >> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h >> index 38e4a67f5922..80149643cbcd 100644 >> --- a/include/scsi/scsi_transport_iscsi.h >> +++ b/include/scsi/scsi_transport_iscsi.h >> @@ -232,6 +232,7 @@ enum { >> ISCSI_SESSION_LOGGED_IN, >> ISCSI_SESSION_FAILED, >> ISCSI_SESSION_FREE, >> + ISCSI_SESSION_UNBOUND, >> }; >> >> #define ISCSI_MAX_TARGET -1 > > .
kernel would send ISCSI_KEVENT_UNBIND_SESSION twice to userspace, for open-iscsi, this would trigger iscsi_stop twice. We should fix this issue. Here introduced a new session state ISCSI_SESSION_UNBOUND to address it. Once session state is ISCSI_KEVENT_UNBIND_SESSION, it means __iscsi_unbind_session() has been called for this session and do not need to execute any more. Reference:https://github.com/open-iscsi/open-iscsi/issues/338 Wenchao Hao (2): scsi: iscsi: introduce session UNBOUND state to avoid multiple unbind event iscsi: set session to FREE state after unbind session in remove session drivers/scsi/scsi_transport_iscsi.c | 45 +++++++++++++++++++++-------- include/scsi/scsi_transport_iscsi.h | 1 + 2 files changed, 34 insertions(+), 12 deletions(-)