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.
