Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Thu, 27 Oct 2022 09:18:25 -0700 | Subject | Re: [PATCH v5] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace | From | Lee Duncan <> |
| |
On 10/27/22 19:00, Wenchao Hao wrote: > I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION > for multiple times which should be fixed. > > This patch introduce target_state in iscsi_cls_session to make > sure session would send only one ISCSI_KEVENT_UNBIND_SESSION. > > But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi: > Report unbind session event when the target has been removed"). The issue > is iscsid died for any reason after it send unbind session to kernel, once > iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event. > > Now kernel think iscsi_cls_session has already sent an > ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which > would cause userspace unable to logout. Actually the session is in > invalid state(it's target_id is INVALID), iscsid should not sync this > session in it's restart. > > So we need to check session's target state during iscsid restart, > if session is in unbound state, do not sync this session and perform > session teardown. It's reasonable because once a session is unbound, we > can not recover it any more(mainly because it's target id is INVALID) > > V5: > - Add ISCSI_SESSION_TARGET_ALLOCATED to indicate the session's > target has been allocated but not scanned yet. We should > sync this session and scan this session when iscsid restarted. > > V4: > - Move debug print out of spinlock critical section > > V3: > - Make target bind state to a state kind rather than a bool. > > V2: > - Using target_unbound rather than state to indicate session has been > unbound > > Signed-off-by: Wenchao Hao <haowenchao@huawei.com> > --- > drivers/scsi/scsi_transport_iscsi.c | 51 +++++++++++++++++++++++++++++ > include/scsi/scsi_transport_iscsi.h | 8 +++++ > 2 files changed, 59 insertions(+) > > diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c > index cd3db9684e52..2e0d1cd6d4ea 100644 > --- a/drivers/scsi/scsi_transport_iscsi.c > +++ b/drivers/scsi/scsi_transport_iscsi.c > @@ -1676,6 +1676,30 @@ static const char *iscsi_session_state_name(int state) > return name; > } > > +static struct { > + int value; > + char *name; > +} iscsi_session_target_state_names[] = { > + { ISCSI_SESSION_TARGET_UNBOUND, "UNBOUND" }, > + { ISCSI_SESSION_TARGET_ALLOCATED, "ALLOCATED" }, > + { ISCSI_SESSION_TARGET_BOUND, "BOUND" }, > + { ISCSI_SESSION_TARGET_UNBINDING, "UNBINDING" }, > +}; > + > +static const char *iscsi_session_target_state_name(int state) > +{ > + int i; > + char *name = NULL; > + > + for (i = 0; i < ARRAY_SIZE(iscsi_session_target_state_names); i++) { > + if (iscsi_session_target_state_names[i].value == state) { > + name = iscsi_session_target_state_names[i].name; > + break; > + } > + } > + return name; > +}
It seems like it might be more efficient to use the target state as the array index, so you don't have to loop to find the name, e.g. something like:
> static char* iscsi_session_target_state_names[] = { > .ISCSI_SESSION_TARGET_UNBOUND = "UNBOUND", > .ISCSI_SESSION_TARGET_ALLOCATED = "ALLOCATED", > .ISCSI_SESSION_TARGET_BOUND = "BOUND", > .ISCSI_SESSION_TARGET_UNBINDING = "UNBINDING", > };
I know there are only 4 states, and it's only used for sysfs, so not sure it matters much.
> + > int iscsi_session_chkready(struct iscsi_cls_session *session) > { > int err; > @@ -1779,6 +1803,7 @@ static int iscsi_user_scan_session(struct device *dev, void *data) > goto user_scan_exit; > } > id = session->target_id; > + session->target_state = ISCSI_SESSION_TARGET_BOUND; > spin_unlock_irqrestore(&session->lock, flags); > > if (id != ISCSI_MAX_TARGET) { > @@ -1899,6 +1924,7 @@ static void __iscsi_unblock_session(struct work_struct *work) > cancel_delayed_work_sync(&session->recovery_work); > spin_lock_irqsave(&session->lock, flags); > session->state = ISCSI_SESSION_LOGGED_IN; > + session->target_state = ISCSI_SESSION_TARGET_ALLOCATED; > spin_unlock_irqrestore(&session->lock, flags); > /* start IO */ > scsi_target_unblock(&session->dev, SDEV_RUNNING); > @@ -1961,6 +1987,15 @@ static void __iscsi_unbind_session(struct work_struct *work) > unsigned long flags; > unsigned int target_id; > > + spin_lock_irqsave(&session->lock, flags); > + if (session->target_state != ISCSI_SESSION_TARGET_BOUND) { > + spin_unlock_irqrestore(&session->lock, flags); > + ISCSI_DBG_TRANS_SESSION(session, "Abort unbind sesison\n");
It'd be nice if this said more, since debugging "Abort unbind sessions" would require finding the sources. How about "Abort unbind session: not bound", for example?
> + return; > + } > + session->target_state = ISCSI_SESSION_TARGET_UNBINDING; > + 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 */ > @@ -1984,6 +2019,9 @@ static void __iscsi_unbind_session(struct work_struct *work) > > unbind_session_exit: > iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION); > + spin_lock_irqsave(&session->lock, flags); > + session->target_state = ISCSI_SESSION_TARGET_UNBOUND; > + spin_unlock_irqrestore(&session->lock, flags); > ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n"); > } > > @@ -4368,6 +4406,16 @@ iscsi_session_attr(def_taskmgmt_tmo, ISCSI_PARAM_DEF_TASKMGMT_TMO, 0); > iscsi_session_attr(discovery_parent_idx, ISCSI_PARAM_DISCOVERY_PARENT_IDX, 0); > iscsi_session_attr(discovery_parent_type, ISCSI_PARAM_DISCOVERY_PARENT_TYPE, 0); > > +static ssize_t > +show_priv_session_target_state(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct iscsi_cls_session *session = iscsi_dev_to_session(dev->parent); > + return sysfs_emit(buf, "%s\n", > + iscsi_session_target_state_name(session->target_state)); > +} > +static ISCSI_CLASS_ATTR(priv_sess, target_state, S_IRUGO, > + show_priv_session_target_state, NULL); > static ssize_t > show_priv_session_state(struct device *dev, struct device_attribute *attr, > char *buf) > @@ -4470,6 +4518,7 @@ static struct attribute *iscsi_session_attrs[] = { > &dev_attr_sess_boot_target.attr, > &dev_attr_priv_sess_recovery_tmo.attr, > &dev_attr_priv_sess_state.attr, > + &dev_attr_priv_sess_target_state.attr, > &dev_attr_priv_sess_creator.attr, > &dev_attr_sess_chap_out_idx.attr, > &dev_attr_sess_chap_in_idx.attr, > @@ -4583,6 +4632,8 @@ static umode_t iscsi_session_attr_is_visible(struct kobject *kobj, > return S_IRUGO | S_IWUSR; > else if (attr == &dev_attr_priv_sess_state.attr) > return S_IRUGO; > + else if (attr == &dev_attr_priv_sess_target_state.attr) > + return S_IRUGO; > else if (attr == &dev_attr_priv_sess_creator.attr) > return S_IRUGO; > else if (attr == &dev_attr_priv_sess_target_id.attr) > diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h > index cab52b0f11d0..db5db8dea28a 100644 > --- a/include/scsi/scsi_transport_iscsi.h > +++ b/include/scsi/scsi_transport_iscsi.h > @@ -236,6 +236,13 @@ enum { > ISCSI_SESSION_FREE, > }; > > +enum { > + ISCSI_SESSION_TARGET_UNBOUND, > + ISCSI_SESSION_TARGET_ALLOCATED, > + ISCSI_SESSION_TARGET_BOUND, > + ISCSI_SESSION_TARGET_UNBINDING, > +}; > + > #define ISCSI_MAX_TARGET -1 > > struct iscsi_cls_session { > @@ -264,6 +271,7 @@ struct iscsi_cls_session { > */ > pid_t creator; > int state; > + int target_state; /* session target bind state */ > int sid; /* session id */ > void *dd_data; /* LLD private data */ > struct device dev; /* sysfs transport/container device */
Thank you for sticking with this. It is very much appreciated. -- Lee Duncan
| ![\](/images/icornerr.gif) |