>>> "'Wenchao Hao' via open-iscsi" <open-iscsi@xxxxxxxxxxxxxxxx> schrieb am
21.11.2022 um 15:17 in Nachricht
<89692b2b-90f7-e8e8-fa77-f14dbe996b72@xxxxxxxxxx>:
> 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:
Actually I think with a modern optimizing compiler there should be little difference in the code created.
>
> 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 open-iscsi+unsubscribe@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/open-iscsi/89692b2b-90f7-e8e8-fa77-f14dbe99
> 6b72%40huawei.com.
