kernel would send ISCSI_KEVENT_UNBIND_SESSION twice to userspace, for
open-iscsi, this would trigger iscsi_stop twice. We should fix this issue.
Here introduced a new session state ISCSI_SESSION_UNBOUND to address it.
Once session state is ISCSI_KEVENT_UNBIND_SESSION, it means
__iscsi_unbind_session() has been called for this session and do not need
to execute any more.
Reference:https://github.com/open-iscsi/open-iscsi/issues/338
Wenchao Hao (2):
scsi: iscsi: introduce session UNBOUND state to avoid multiple unbind
event
iscsi: set session to FREE state after unbind session in remove
session
drivers/scsi/scsi_transport_iscsi.c | 45 +++++++++++++++++++++--------
include/scsi/scsi_transport_iscsi.h | 1 +
2 files changed, 34 insertions(+), 12 deletions(-)
--
2.32.0
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.
Reference:https://github.com/open-iscsi/open-iscsi/issues/338
Signed-off-by: Wenchao Hao <[email protected]>
---
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
--
2.32.0
__iscsi_unbind_session() set session state to ISCSI_SESSION_UNBOUND, which
would overwrite the ISCSI_SESSION_FREE state.
Signed-off-by: Wenchao Hao <[email protected]>
---
drivers/scsi/scsi_transport_iscsi.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 97a9fee02efa..d8dd9279cea8 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2173,6 +2173,22 @@ void iscsi_remove_session(struct iscsi_cls_session *session)
if (!cancel_work_sync(&session->block_work))
cancel_delayed_work_sync(&session->recovery_work);
cancel_work_sync(&session->unblock_work);
+
+ scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE);
+ /*
+ * qla4xxx can perform it's own scans when it runs in kernel only
+ * mode. Make sure to flush those scans.
+ */
+ flush_work(&session->scan_work);
+
+ /*
+ * flush running unbind operations
+ * if unbind work did not queued, call __iscsi_unbind_session
+ * directly to perform target remove
+ */
+ if (!flush_work(&session->unbind_work))
+ __iscsi_unbind_session(&session->unbind_work);
+
/*
* If we are blocked let commands flow again. The lld or iscsi
* layer should set up the queuecommand to fail commands.
@@ -2183,16 +2199,6 @@ void iscsi_remove_session(struct iscsi_cls_session *session)
session->state = ISCSI_SESSION_FREE;
spin_unlock_irqrestore(&session->lock, flags);
- scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE);
- /*
- * qla4xxx can perform it's own scans when it runs in kernel only
- * mode. Make sure to flush those scans.
- */
- flush_work(&session->scan_work);
- /* flush running unbind operations */
- flush_work(&session->unbind_work);
- __iscsi_unbind_session(&session->unbind_work);
-
/* hw iscsi may not have removed all connections from session */
err = device_for_each_child(&session->dev, NULL,
iscsi_iter_destroy_conn_fn);
--
2.32.0
On 4/13/22 8:49 PM, Wenchao Hao wrote:
> __iscsi_unbind_session() set session state to ISCSI_SESSION_UNBOUND, which
> would overwrite the ISCSI_SESSION_FREE state.
>
> Signed-off-by: Wenchao Hao <[email protected]>
> ---
> drivers/scsi/scsi_transport_iscsi.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 97a9fee02efa..d8dd9279cea8 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2173,6 +2173,22 @@ void iscsi_remove_session(struct iscsi_cls_session *session)
> if (!cancel_work_sync(&session->block_work))
> cancel_delayed_work_sync(&session->recovery_work);
> cancel_work_sync(&session->unblock_work);
> +
> + scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE);
> + /*
> + * qla4xxx can perform it's own scans when it runs in kernel only
> + * mode. Make sure to flush those scans.
> + */
> + flush_work(&session->scan_work);
> +
> + /*
> + * flush running unbind operations
> + * if unbind work did not queued, call __iscsi_unbind_session
> + * directly to perform target remove
We probably don't need the flush_work test because we are going to
normally call __iscsi_unbind_session.
If the unbind work had already run, which is the normal case, then
flush_work returns false and we end up calling __iscsi_unbind_session
like before. That function then checks if the target is really unbound.
So the extra check doesn't normally buy us anything with your patches
because in patch 1 you fixed it so __iscsi_unbind_session doesn't send
the extra event.
> + */
> + if (!flush_work(&session->unbind_work))
> + __iscsi_unbind_session(&session->unbind_work);
> +
> /*
> * If we are blocked let commands flow again. The lld or iscsi
> * layer should set up the queuecommand to fail commands.
> @@ -2183,16 +2199,6 @@ void iscsi_remove_session(struct iscsi_cls_session *session)
> session->state = ISCSI_SESSION_FREE;
> spin_unlock_irqrestore(&session->lock, flags);
>
> - scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE);
> - /*
> - * qla4xxx can perform it's own scans when it runs in kernel only
> - * mode. Make sure to flush those scans.
> - */
> - flush_work(&session->scan_work);
> - /* flush running unbind operations */
> - flush_work(&session->unbind_work);
> - __iscsi_unbind_session(&session->unbind_work);
> -
> /* hw iscsi may not have removed all connections from session */
> err = device_for_each_child(&session->dev, NULL,
> iscsi_iter_destroy_conn_fn);
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.
> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
>
> .
On 4/15/22 4:40 AM, Wenchao Hao wrote:
> On 2022/4/14 23:30, Mike Christie wrote:
>> On 4/13/22 8:49 PM, Wenchao Hao wrote:
>>> __iscsi_unbind_session() set session state to ISCSI_SESSION_UNBOUND, which
>>> would overwrite the ISCSI_SESSION_FREE state.
>>>
>>> Signed-off-by: Wenchao Hao <[email protected]>
>>> ---
>>> drivers/scsi/scsi_transport_iscsi.c | 26 ++++++++++++++++----------
>>> 1 file changed, 16 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
>>> index 97a9fee02efa..d8dd9279cea8 100644
>>> --- a/drivers/scsi/scsi_transport_iscsi.c
>>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>>> @@ -2173,6 +2173,22 @@ void iscsi_remove_session(struct iscsi_cls_session *session)
>>> if (!cancel_work_sync(&session->block_work))
>>> cancel_delayed_work_sync(&session->recovery_work);
>>> cancel_work_sync(&session->unblock_work);
>>> +
>>> + scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE);
>>> + /*
>>> + * qla4xxx can perform it's own scans when it runs in kernel only
>>> + * mode. Make sure to flush those scans.
>>> + */
>>> + flush_work(&session->scan_work);
>>> +
>>> + /*
>>> + * flush running unbind operations
>>> + * if unbind work did not queued, call __iscsi_unbind_session
>>> + * directly to perform target remove
>>
>> We probably don't need the flush_work test because we are going to
>> normally call __iscsi_unbind_session.
>>
>
> I think we still need calling flush_work here. The introduce of flush_work
Above I was saying we don't need to *test* if it returned true/false.
Just always flush and then call __iscsi_unbind_session like we did
originally below. The check you added in __iscsi_unbind_session in the
first patch will detect if it's been called or not.
> is to make sure sysfs objects are removed in an correct order. There is a
> very low probability that __iscsi_unbind_session() triggered by queue_work()
> has not been finished, and iscsi_remove_session() is called. So we need
> flush_work() to make sure __iscsi_unbind_session() has done if it has been
> activated by queue_work().
>
>> If the unbind work had already run, which is the normal case, then
>> flush_work returns false and we end up calling __iscsi_unbind_session
>> like before. That function then checks if the target is really unbound.
>> So the extra check doesn't normally buy us anything with your patches
>> because in patch 1 you fixed it so __iscsi_unbind_session doesn't send
>> the extra event.
>>
>>
>>> + */
>>> + if (!flush_work(&session->unbind_work))
>>> + __iscsi_unbind_session(&session->unbind_work);
>>> +
>>> /*
>>> * If we are blocked let commands flow again. The lld or iscsi
>>> * layer should set up the queuecommand to fail commands.
>>> @@ -2183,16 +2199,6 @@ void iscsi_remove_session(struct iscsi_cls_session *session)
>>> session->state = ISCSI_SESSION_FREE;
>>> spin_unlock_irqrestore(&session->lock, flags);
>>>
>>> - scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE);
>>> - /*
>>> - * qla4xxx can perform it's own scans when it runs in kernel only
>>> - * mode. Make sure to flush those scans.
>>> - */
>>> - flush_work(&session->scan_work);
>>> - /* flush running unbind operations */
>>> - flush_work(&session->unbind_work);
>>> - __iscsi_unbind_session(&session->unbind_work);
>>> -
>>> /* hw iscsi may not have removed all connections from session */
>>> err = device_for_each_child(&session->dev, NULL,
>>> iscsi_iter_destroy_conn_fn);
>>
>> .
>