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 <[email protected]>
---
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;
+}
+
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");
+ 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 */
--
2.35.3
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 <[email protected]>
> ---
> 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
On 10/27/22 9:00 PM, Wenchao Hao wrote:
> @@ -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;
I think there are 2 issues with setting this to ISCSI_SESSION_TARGET_ALLOCATED
here.
1. We allocated the target_id in iscsi_add_session so it's weird to set
the state late above.
2. We call the above function when we relogin, so it overwrites
ISCSI_SESSION_TARGET_BOUND.
I think we can just do:
@@ -1785,7 +1785,12 @@ static int iscsi_user_scan_session(struct device *dev, void *data)
if ((scan_data->channel == SCAN_WILD_CARD ||
scan_data->channel == 0) &&
(scan_data->id == SCAN_WILD_CARD ||
- scan_data->id == id))
+ scan_data->id == id)) {
+
+ spin_lock_irqsave(&session->lock, flags);
+ session->target_state = ISCSI_SESSION_TARGET_SCANNED;
+ spin_unlock_irqsave(&session->lock, flags);
+
scsi_scan_target(&session->dev, 0, id,
scan_data->lun, scan_data->rescan);
}
@@ -1972,6 +1977,7 @@ static void __iscsi_unbind_session(struct work_struct *work)
goto unbind_session_exit;
}
+ session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
target_id = session->target_id;
session->target_id = ISCSI_MAX_TARGET;
spin_unlock_irqrestore(&session->lock, flags);
@@ -1983,6 +1989,10 @@ static void __iscsi_unbind_session(struct work_struct *work)
ida_free(&iscsi_sess_ida, target_id);
unbind_session_exit:
+ spin_lock_irqsave(&session->lock, flags);
+ session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
+ spin_unlock_irqrestore(&session->lock, flags);
+
iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
}
@@ -2061,6 +2071,7 @@ int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
session->ida_used = true;
} else
session->target_id = target_id;
+ session->target_state = ISCSI_SESSION_TARGET_ID_ALLOCATED;
dev_set_name(&session->dev, "session%u", session->sid);
err = device_add(&session->dev);
then in userspace we can do:
1. if have conn in sysfs
if session->target_state == ISCSI_SESSION_TARGET_ID_ALLOCATED
if conn->state == ISCSI_CONN_UP
/*
* we did the initial login and did a ISCSI_UEVENT_START_CONN
* but crashed/restarted
* before we were able to scan.
*/
sync session and scan session
else
/*
* the initial login didn't happen so
* just cleanup the kernel. Note: we could also
* finish setting the conn up but it probably doesn't
* come up enough to do this.
*/
else if session->target_state == ISCSI_SESSION_TARGET_SCANNED
do normal sync
else if session->target_state == ISCSI_SESSION_TARGET_REMOVING
wait for state to change to ISCSI_SESSION_TARGET_UNBOUND
or setup session/conn in userspace so we can wait for
ISCSI_KEVENT_UNBIND_SESSION then cleanup session/conn.
else if session->target_state == ISCSI_SESSION_TARGET_UNBOUND
cleanup session/conn based on their state and what's in sysfs
if no conn in sysfs
cleanup session
If we are in agreement, then you can take my code above and merge it into
your patch and resubmit in one patch.
On 2022/10/28 0:18, Lee Duncan wrote:
> On 10/27/22 19:00, Wenchao Hao wrote:
>> 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.
>
It's a better implement, I would update it.
>> @@ -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?
>
Of course, I would updated.
>> @@ -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.
I should apologize for taking this issue off for a long time because of my slow response.
What's more, should I add an acked-by or reviewed-by in my next patch?
On 2022/10/28 5:02, Mike Christie wrote:
> On 10/27/22 9:00 PM, Wenchao Hao wrote:
>> @@ -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;
>
> I think there are 2 issues with setting this to ISCSI_SESSION_TARGET_ALLOCATED
> here.
>
> 1. We allocated the target_id in iscsi_add_session so it's weird to set
> the state late above.
> 2. We call the above function when we relogin, so it overwrites
> ISCSI_SESSION_TARGET_BOUND.
>
> I think we can just do:
>
> @@ -1785,7 +1785,12 @@ static int iscsi_user_scan_session(struct device *dev, void *data)
> if ((scan_data->channel == SCAN_WILD_CARD ||
> scan_data->channel == 0) &&
> (scan_data->id == SCAN_WILD_CARD ||
> - scan_data->id == id))
> + scan_data->id == id)) {
> +
> + spin_lock_irqsave(&session->lock, flags);
> + session->target_state = ISCSI_SESSION_TARGET_SCANNED;
> + spin_unlock_irqsave(&session->lock, flags);
> +
> scsi_scan_target(&session->dev, 0, id,
> scan_data->lun, scan_data->rescan);
> }
> @@ -1972,6 +1977,7 @@ static void __iscsi_unbind_session(struct work_struct *work)
> goto unbind_session_exit;
> }
>
> + session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
> target_id = session->target_id;
> session->target_id = ISCSI_MAX_TARGET;
> spin_unlock_irqrestore(&session->lock, flags);
> @@ -1983,6 +1989,10 @@ static void __iscsi_unbind_session(struct work_struct *work)
> ida_free(&iscsi_sess_ida, target_id);
>
> unbind_session_exit:
> + spin_lock_irqsave(&session->lock, flags);
> + session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
> + spin_unlock_irqrestore(&session->lock, flags);
> +
> iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
> ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
> }
> @@ -2061,6 +2071,7 @@ int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
> session->ida_used = true;
> } else
> session->target_id = target_id;
> + session->target_state = ISCSI_SESSION_TARGET_ID_ALLOCATED;
>
> dev_set_name(&session->dev, "session%u", session->sid);
> err = device_add(&session->dev);
>
>
> then in userspace we can do:
>
> 1. if have conn in sysfs
> if session->target_state == ISCSI_SESSION_TARGET_ID_ALLOCATED
> if conn->state == ISCSI_CONN_UP
> /*
> * we did the initial login and did a ISCSI_UEVENT_START_CONN
> * but crashed/restarted
> * before we were able to scan.
> */
> sync session and scan session
> else
> /*
> * the initial login didn't happen so
> * just cleanup the kernel. Note: we could also
> * finish setting the conn up but it probably doesn't
> * come up enough to do this.
> */
>
> else if session->target_state == ISCSI_SESSION_TARGET_SCANNED
> do normal sync
> else if session->target_state == ISCSI_SESSION_TARGET_REMOVING
> wait for state to change to ISCSI_SESSION_TARGET_UNBOUND
> or setup session/conn in userspace so we can wait for
> ISCSI_KEVENT_UNBIND_SESSION then cleanup session/conn.
> else if session->target_state == ISCSI_SESSION_TARGET_UNBOUND
> cleanup session/conn based on their state and what's in sysfs
>
> if no conn in sysfs
> cleanup session
>
>
> If we are in agreement, then you can take my code above and merge it into
> your patch and resubmit in one patch.
> .
Thanks a lot for your suggestions, I just updated the PR in last night.
I would update this patch according to your suggestions.
On 10/28/22 02:45, Wenchao Hao wrote:
>
> On 2022/10/28 0:18, Lee Duncan wrote:
>> On 10/27/22 19:00, Wenchao Hao wrote:
>>> 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.
>>
>
> It's a better implement, I would update it.
>
>>> @@ -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?
>>
>
> Of course, I would updated.
>
>>> @@ -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.
>
> I should apologize for taking this issue off for a long time because of my slow response.
>
> What's more, should I add an acked-by or reviewed-by in my next patch?
Please add my Reviewed-by tag after you make these changes.
And no problem taking a while. I appreciate your effort!
--
Lee Duncan
On 2022/10/28 23:05, Lee Duncan wrote:
> Please add my Reviewed-by tag after you make these changes.
>
> And no problem taking a while. I appreciate your effort!
More changes are added in my next patch, so I did not add your Reviewed-by tag.
Would you help review my new patch, thanks a lot?