On 2022/11/9 11:47, Mike Christie wrote:
> On 11/7/22 7:44 PM, Wenchao Hao wrote:
>> I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
>> for multiple times which should be fixed.
>>
>> +static char *iscsi_session_target_state_names[] = {
>> + "UNBOUND",
>> + "ALLOCATED",
>> + "SCANNED",
>> + "UNBINDING",
>> +};
>
> I think maybe Lee meant you to do something like:
>
> static int iscsi_target_state_to_name[] = {
> [ISCSI_SESSION_TARGET_UNBOUND] = "UNBOUND",
> [ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
> .....
>
>
Define array as following and remove previous helper function:
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",
};
Reference the array directly:
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]);
}
>> + spin_lock_irqsave(&session->lock, flags);
>> + if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
>> + spin_unlock_irqrestore(&session->lock, flags);
>> + if (session->ida_used)
>> + ida_free(&iscsi_sess_ida, session->target_id);
>> + ISCSI_DBG_TRANS_SESSION(session, "Donot unbind sesison:
>> allocated\n");
>
> Could you change the error message to "Skipping target unbinding: Session not
> yet scanned.\n"
>
>> + goto unbind_session_exit;
>> + }
>
> Just add a newline/return here.
Actually we should skip unbind this session if call into
__iscsi_unbind_session() with target state
is ALLOCATED. So I removed the check, and check only one condition in
__iscsi_unbind_session(): if the
target state is SCANNED.
>
> I think you want to move both state checks to after the we take the host lock
> and
> session lock after the line above. You don't have to take the lock multiple
> times
> and we can drop the target_id == ISCSI_MAX_TARGET since it would then rely on
> the
> state checks (I left out the ISCSI_DBG_TRANS_SESSION because I'm lazy):
>
> bool remove_target = false;
> .....
>
>
I think it's not necessary to add a flag remove_target, here is my changes for
function __iscsi_unbind_session:
@@ -1966,23 +1977,28 @@ static void __iscsi_unbind_session(struct work_struct
*work)
/* 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_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 %s.\n",
+
iscsi_session_target_state_name[session->target_state]);
+ return;
}
-
target_id = session->target_id;
session->target_id = ISCSI_MAX_TARGET;
+ session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
spin_unlock_irqrestore(&session->lock, flags);
mutex_unlock(&ihost->mutex);
scsi_remove_target(&session->dev);
+ spin_lock_irqsave(&session->lock, flags);
+ session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
+ spin_unlock_irqrestore(&session->lock, flags);
+
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");
}
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) {
spin_unlock_irqrestore(&session->lock, flags);
mutex_unlock(&ihost->mutex);
ISCSI_DBG_TRANS_SESSION(session, "Skipping target unbinding:
Session is %s.\n",
iscsi_session_target_state_name[session->target_state]);
return;
}
target_id = session->target_id;
session->target_id = ISCSI_MAX_TARGET;
session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
spin_unlock_irqrestore(&session->lock, flags);
mutex_unlock(&ihost->mutex);
scsi_remove_target(&session->dev);
spin_lock_irqsave(&session->lock, flags);
session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
spin_unlock_irqrestore(&session->lock, flags);
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");
}
--
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 [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/open-iscsi/89692b2b-90f7-e8e8-fa77-f14dbe996b72%40huawei.com.
