Re: [PATCH v7] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

2022-12-13 Thread Martin K. Petersen 


Wenchao,

> I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
> for multiple times which should be fixed.

Applied to 6.2/scsi-staging, thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/yq1wn6uem2d.fsf%40ca-mkp.ca.oracle.com.

Re: [PATCH v7] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

2022-12-08 Thread 'Wu Bo' via open-iscsi 

On 2022/11/26 9:07, 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)

V7:
- Define target state to string map and refer this map directly
- Cleanup __iscsi_unbind_session, drop check for session's
   target_id == ISCSI_MAX_TARGET since it can be handled by target_state

V6:
- Set target state to ALLOCATED in iscsi_add_session
- Rename state BOUND to SCANNED
- Make iscsi_session_target_state_name() more efficient

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 
---
  drivers/scsi/scsi_transport_iscsi.c | 47 ++---
  include/scsi/scsi_transport_iscsi.h |  9 ++
  2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index cd3db9684e52..812578c20fe5 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1676,6 +1676,13 @@ static const char *iscsi_session_state_name(int state)
return name;
  }
  
+static char *iscsi_session_target_state_name[] = {

+   [ISCSI_SESSION_TARGET_UNBOUND]   = "UNBOUND",
+   [ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
+   [ISCSI_SESSION_TARGET_SCANNED]   = "SCANNED",
+   [ISCSI_SESSION_TARGET_UNBINDING] = "UNBINDING",
+};
+
  int iscsi_session_chkready(struct iscsi_cls_session *session)
  {
int err;
@@ -1785,9 +1792,13 @@ 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)) {
scsi_scan_target(>dev, 0, id,
 scan_data->lun, scan_data->rescan);
+   spin_lock_irqsave(>lock, flags);
+   session->target_state = ISCSI_SESSION_TARGET_SCANNED;
+   spin_unlock_irqrestore(>lock, flags);
+   }
}
  
  user_scan_exit:

@@ -1960,31 +1971,41 @@ static void __iscsi_unbind_session(struct work_struct 
*work)
struct iscsi_cls_host *ihost = shost->shost_data;
unsigned long flags;
unsigned int target_id;
+   bool remove_target = true;
  
  	ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
  
  	/* Prevent new scans and make sure scanning is not in progress */

mutex_lock(>mutex);
spin_lock_irqsave(>lock, flags);
-   if (session->target_id == ISCSI_MAX_TARGET) {
+   if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
+   remove_target = false;
+   } else if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
spin_unlock_irqrestore(>lock, flags);
mutex_unlock(>mutex);
-   goto unbind_session_exit;
+   ISCSI_DBG_TRANS_SESSION(session,
+   "Skipping target unbinding: Session is 
unbound/unbinding.\n");
+   return;
}
  
+	session->target_state = ISCSI_SESSION_TARGET_UNBINDING;

target_id = session->target_id;
session->target_id = ISCSI_MAX_TARGET;
spin_unlock_irqrestore(>lock, flags);
mutex_unlock(>mutex);
  
-	scsi_remove_target(>dev);

+   if (remove_target)
+   scsi_remove_target(>dev);
  
  	if (session->ida_used)

ida_free(_sess_ida, target_id);
  
-unbind_session_exit:

iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);

Re: [PATCH v7] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

2022-12-07 Thread Mike Christie 
On 11/25/22 7:07 PM, 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)
> 
> V7:
> - Define target state to string map and refer this map directly
> - Cleanup __iscsi_unbind_session, drop check for session's
>   target_id == ISCSI_MAX_TARGET since it can be handled by target_state
> 
> V6:
> - Set target state to ALLOCATED in iscsi_add_session
> - Rename state BOUND to SCANNED
> - Make iscsi_session_target_state_name() more efficient
> 
> 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 

Reviewed-by: Mike Christie 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/050e7458-c952-1b1f-a98e-6c12c5b30cdb%40oracle.com.

Re: [PATCH v7] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

2022-12-05 Thread 'Wenchao Hao' via open-iscsi 
On 2022/11/26 9:07, 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)
> 
> V7:
> - Define target state to string map and refer this map directly
> - Cleanup __iscsi_unbind_session, drop check for session's
>   target_id == ISCSI_MAX_TARGET since it can be handled by target_state
> 
> V6:
> - Set target state to ALLOCATED in iscsi_add_session
> - Rename state BOUND to SCANNED
> - Make iscsi_session_target_state_name() more efficient
> 
> 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 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 47 ++---
>  include/scsi/scsi_transport_iscsi.h |  9 ++
>  2 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index cd3db9684e52..812578c20fe5 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1676,6 +1676,13 @@ static const char *iscsi_session_state_name(int state)
>   return name;
>  }
>  
> +static char *iscsi_session_target_state_name[] = {
> + [ISCSI_SESSION_TARGET_UNBOUND]   = "UNBOUND",
> + [ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
> + [ISCSI_SESSION_TARGET_SCANNED]   = "SCANNED",
> + [ISCSI_SESSION_TARGET_UNBINDING] = "UNBINDING",
> +};
> +
>  int iscsi_session_chkready(struct iscsi_cls_session *session)
>  {
>   int err;
> @@ -1785,9 +1792,13 @@ 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)) {
>   scsi_scan_target(>dev, 0, id,
>scan_data->lun, scan_data->rescan);
> + spin_lock_irqsave(>lock, flags);
> + session->target_state = ISCSI_SESSION_TARGET_SCANNED;
> + spin_unlock_irqrestore(>lock, flags);
> + }
>   }
>  
>  user_scan_exit:
> @@ -1960,31 +1971,41 @@ static void __iscsi_unbind_session(struct work_struct 
> *work)
>   struct iscsi_cls_host *ihost = shost->shost_data;
>   unsigned long flags;
>   unsigned int target_id;
> + bool remove_target = true;
>  
>   ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>  
>   /* Prevent new scans and make sure scanning is not in progress */
>   mutex_lock(>mutex);
>   spin_lock_irqsave(>lock, flags);
> - if (session->target_id == ISCSI_MAX_TARGET) {
> + if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
> + remove_target = false;
> + } else if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
>   spin_unlock_irqrestore(>lock, flags);
>   mutex_unlock(>mutex);
> - goto unbind_session_exit;
> + ISCSI_DBG_TRANS_SESSION(session,
> + "Skipping target unbinding: Session is 
> unbound/unbinding.\n");
> + return;
>   }
>  
> + session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
>   target_id = session->target_id;
>   session->target_id = ISCSI_MAX_TARGET;
>   spin_unlock_irqrestore(>lock, flags);
>   mutex_unlock(>mutex);
>  
> - scsi_remove_target(>dev);
> + if (remove_target)
> + scsi_remove_target(>dev);
>  
>   if