On 2022/11/23 2:15, Mike Christie wrote:
> On 11/22/22 11:29 AM, Wenchao Hao wrote:
>> On Wed, Nov 23, 2022 at 1:04 AM Mike Christie
>> <michael.chris...@oracle.com> wrote:
>>>
>>> On 11/21/22 8:17 AM, Wenchao Hao wrote:
>>>> And the function looks like following after change:
>>>>
>>>> static void __iscsi_unbind_session(struct work_struct *work)
>>>> {
>>>> struct iscsi_cls_session *session =
>>>> container_of(work, struct iscsi_cls_session,
>>>> unbind_work);
>>>> struct Scsi_Host *shost = iscsi_session_to_shost(session);
>>>> struct iscsi_cls_host *ihost = shost->shost_data;
>>>> unsigned long flags;
>>>> unsigned int target_id;
>>>>
>>>> ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>>>>
>>>> /* Prevent new scans and make sure scanning is not in progress */
>>>> mutex_lock(&ihost->mutex);
>>>> spin_lock_irqsave(&session->lock, flags);
>>>> if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
>>>
>>> What was the reason for not checking for ALLOCATED and freeing the ida
>>> in that case?
>>>
>>
>> target_state would be in "ALLOCATED" state if iscsid died after add
>> session successfully.
>> When iscsid restarted, if the session's target_state is "ALLOCATED",
>> it should scan
>> the session and the target_state would switch to "SCANNED".
>>
>> So I think we would not call in __iscsi_unbind_session() with
>> session's target_state
>> is ALLOCATED.
>
> Makes sense for the normal case.
>
> The only issue is when __iscsi_unbind_session is called via
> iscsi_remove_session for the cases where userspace didn't do
> the UNBIND event. Some tools don't do unbind or open-iscsi
> sometimes doesn't if the session is down. We will leak the ida,
> so you need some code to handle that.
>
> .
Sorry, I did not take this condition in consideration. I would change
the __iscsi_unbind_session as following:
1. do not check if target_id is ISCSI_MAX_TARGET
2. define remove_target and default set to true, if target_state is ALLOCATED,
then set
it to false and continue the unbind flow; else if target_state not SCANNED,
just return.
3. set target_state to ISCSI_SESSION_TARGET_UNBOUND after is sent to avoid
potential race condition.
diff --git a/drivers/scsi/scsi_transport_iscsi.c
b/drivers/scsi/scsi_transport_iscsi.c
index cd3db9684e52..9264c75ad9ea 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1960,31 +1960,40 @@ 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(&ihost->mutex);
spin_lock_irqsave(&session->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(&session->lock, flags);
mutex_unlock(&ihost->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(&session->lock, flags);
mutex_unlock(&ihost->mutex);
- scsi_remove_target(&session->dev);
+ if (remove_target)
+ scsi_remove_target(&session->dev);
if (session->ida_used)
ida_free(&iscsi_sess_ida, target_id);
-unbind_session_exit:
iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
+
+ spin_lock_irqsave(&session->lock, flags);
+ session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
+ spin_unlock_irqrestore(&session->lock, flags);
}
And the function would be:
static void __iscsi_unbind_session(struct work_struct *work)
{
struct iscsi_cls_session *session =
container_of(work, struct iscsi_cls_session,
unbind_work);
struct Scsi_Host *shost = iscsi_session_to_shost(session);
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(&ihost->mutex);
spin_lock_irqsave(&session->lock, flags);
if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
remove_target = false;
} else if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
spin_unlock_irqrestore(&session->lock, flags);
mutex_unlock(&ihost->mutex);
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(&session->lock, flags);
mutex_unlock(&ihost->mutex);
if (remove_target)
scsi_remove_target(&session->dev);
if (session->ida_used)
ida_free(&iscsi_sess_ida, target_id);
iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
spin_lock_irqsave(&session->lock, flags);
session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
spin_unlock_irqrestore(&session->lock, flags);
}
--
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/3a2ba8f0-3a86-36cf-cafd-969298594830%40huawei.com.
