Re: [PATCH 1/2] scsi: iscsi: introduce session UNBOUND state to avoid multiple unbind event

From: Wenchao Hao
Date: Fri Apr 15 2022 - 05:33:29 EST


On 2022/4/14 23:22, Mike Christie wrote:
> On 4/13/22 8:49 PM, Wenchao Hao wrote:
>> Fix the issue of kernel send multiple ISCSI_KEVENT_UNBIND_SESSION event.
>> If session is in UNBOUND state, do not perform unbind operations anymore,
>> else unbind session and set session to UNBOUND state.
>>
>
> I don't think we want this to be a state because you can have a session
> with no target or it could be partially deleted and it could be in the
> logged in or failed state. If scsi-ml is sending SYNC_CACHEs as part of
> the target/device removal operation, and we lose the session then we could
> go through recovery and the state will go from failed to logged in, and
> your new unbound state will have been overwritten.
>
> I think it might be better to have a new sysfs file, target_state, for
> this where you could have values like scanning, bound, unbinding, and
> unbound, or just a sysfs file, target_bound, that is bool.
>

Thanks for your review, I would modify and send another patch.

>> Reference:https://github.com/open-iscsi/open-iscsi/issues/338
>>
>
> You should add a description of the problem in the commit, because that
> link might be gone one day.
>
>
>> Signed-off-by: Wenchao Hao <haowenchao@xxxxxxxxxx>
>> ---
>> drivers/scsi/scsi_transport_iscsi.c | 19 +++++++++++++++++--
>> include/scsi/scsi_transport_iscsi.h | 1 +
>> 2 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
>> index 27951ea05dd4..97a9fee02efa 100644
>> --- a/drivers/scsi/scsi_transport_iscsi.c
>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>> @@ -1656,6 +1656,7 @@ static struct {
>> { ISCSI_SESSION_LOGGED_IN, "LOGGED_IN" },
>> { ISCSI_SESSION_FAILED, "FAILED" },
>> { ISCSI_SESSION_FREE, "FREE" },
>> + { ISCSI_SESSION_UNBOUND, "UNBOUND" },
>> };
>>
>> static const char *iscsi_session_state_name(int state)
>> @@ -1686,6 +1687,9 @@ int iscsi_session_chkready(struct iscsi_cls_session *session)
>> case ISCSI_SESSION_FREE:
>> err = DID_TRANSPORT_FAILFAST << 16;
>> break;
>> + case ISCSI_SESSION_UNBOUND:
>> + err = DID_NO_CONNECT << 16;
>> + break;
>> default:
>> err = DID_NO_CONNECT << 16;
>> break;
>> @@ -1838,7 +1842,8 @@ int iscsi_block_scsi_eh(struct scsi_cmnd *cmd)
>>
>> spin_lock_irqsave(&session->lock, flags);
>> while (session->state != ISCSI_SESSION_LOGGED_IN) {
>> - if (session->state == ISCSI_SESSION_FREE) {
>> + if ((session->state == ISCSI_SESSION_FREE) ||
>> + (session->state == ISCSI_SESSION_UNBOUND)) {
>> ret = FAST_IO_FAIL;
>> break;
>> }
>> @@ -1869,6 +1874,7 @@ static void session_recovery_timedout(struct work_struct *work)
>> break;
>> case ISCSI_SESSION_LOGGED_IN:
>> case ISCSI_SESSION_FREE:
>> + case ISCSI_SESSION_UNBOUND:
>> /* we raced with the unblock's flush */
>> spin_unlock_irqrestore(&session->lock, flags);
>> return;
>> @@ -1957,6 +1963,14 @@ static void __iscsi_unbind_session(struct work_struct *work)
>> unsigned long flags;
>> unsigned int target_id;
>>
>> + spin_lock_irqsave(&session->lock, flags);
>> + if (session->state == ISCSI_SESSION_UNBOUND) {
>> + spin_unlock_irqrestore(&session->lock, flags);
>> + return;
>> + }
>> + session->state = ISCSI_SESSION_UNBOUND;
>> + spin_unlock_irqrestore(&session->lock, flags);
>> +
>> ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>>
>> /* Prevent new scans and make sure scanning is not in progress */
>> @@ -4329,7 +4343,8 @@ store_priv_session_##field(struct device *dev, \
>> struct iscsi_cls_session *session = \
>> iscsi_dev_to_session(dev->parent); \
>> if ((session->state == ISCSI_SESSION_FREE) || \
>> - (session->state == ISCSI_SESSION_FAILED)) \
>> + (session->state == ISCSI_SESSION_FAILED) || \
>> + (session->state == ISCSI_SESSION_UNBOUND)) \
>> return -EBUSY; \
>> if (strncmp(buf, "off", 3) == 0) { \
>> session->field = -1; \
>> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
>> index 38e4a67f5922..80149643cbcd 100644
>> --- a/include/scsi/scsi_transport_iscsi.h
>> +++ b/include/scsi/scsi_transport_iscsi.h
>> @@ -232,6 +232,7 @@ enum {
>> ISCSI_SESSION_LOGGED_IN,
>> ISCSI_SESSION_FAILED,
>> ISCSI_SESSION_FREE,
>> + ISCSI_SESSION_UNBOUND,
>> };
>>
>> #define ISCSI_MAX_TARGET -1
>
> .