From: Wenchao Hao <haowenchao@huawei.com>
To: Mike Christie <michael.christie@oracle.com>,
Wenchao Hao <haowenchao22@gmail.com>
Cc: Lee Duncan <lduncan@suse.com>, Chris Leech <cleech@redhat.com>,
"Martin K . Petersen" <martin.petersen@oracle.com>,
"James E . J . Bottomley" <jejb@linux.ibm.com>,
<open-iscsi@googlegroups.com>, <linux-scsi@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <linfeilong@huawei.com>
Subject: Re: [PATCH v6] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace
Date: Wed, 23 Nov 2022 22:21:47 +0800 [thread overview]
Message-ID: <3a2ba8f0-3a86-36cf-cafd-969298594830@huawei.com> (raw)
In-Reply-To: <856ccad2-19a4-32b4-b41f-5a230a55ee30@oracle.com>
On 2022/11/23 2:15, Mike Christie wrote:
> On 11/22/22 11:29 AM, Wenchao Hao wrote:
>> On Wed, Nov 23, 2022 at 1:04 AM Mike Christie
>> <michael.christie@oracle.com> wrote:
>>>
>>> On 11/21/22 8:17 AM, Wenchao Hao wrote:
>>>> 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) {
>>>
>>> What was the reason for not checking for ALLOCATED and freeing the ida
>>> in that case?
>>>
>>
>> target_state would be in "ALLOCATED" state if iscsid died after add
>> session successfully.
>> When iscsid restarted, if the session's target_state is "ALLOCATED",
>> it should scan
>> the session and the target_state would switch to "SCANNED".
>>
>> So I think we would not call in __iscsi_unbind_session() with
>> session's target_state
>> is ALLOCATED.
>
> Makes sense for the normal case.
>
> The only issue is when __iscsi_unbind_session is called via
> iscsi_remove_session for the cases where userspace didn't do
> the UNBIND event. Some tools don't do unbind or open-iscsi
> sometimes doesn't if the session is down. We will leak the ida,
> so you need some code to handle that.
>
> .
Sorry, I did not take this condition in consideration. I would change
the __iscsi_unbind_session as following:
1. do not check if target_id is ISCSI_MAX_TARGET
2. define remove_target and default set to true, if target_state is ALLOCATED, then set
it to false and continue the unbind flow; else if target_state not SCANNED, just return.
3. set target_state to ISCSI_SESSION_TARGET_UNBOUND after is sent to avoid potential race condition.
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index cd3db9684e52..9264c75ad9ea 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1960,31 +1960,40 @@ static void __iscsi_unbind_session(struct work_struct *work)
struct iscsi_cls_host *ihost = shost->shost_data;
unsigned long flags;
unsigned int target_id;
+ bool remove_target = true;
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_id == ISCSI_MAX_TARGET) {
+ if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
+ remove_target = false;
+ } else 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 unbound/unbinding.\n");
+ return;
}
+ session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
target_id = session->target_id;
session->target_id = ISCSI_MAX_TARGET;
spin_unlock_irqrestore(&session->lock, flags);
mutex_unlock(&ihost->mutex);
- scsi_remove_target(&session->dev);
+ if (remove_target)
+ scsi_remove_target(&session->dev);
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");
+
+ spin_lock_irqsave(&session->lock, flags);
+ session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
+ spin_unlock_irqrestore(&session->lock, flags);
}
And the function would be:
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;
bool remove_target = true;
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_ALLOCATED) {
remove_target = false;
} else 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 unbound/unbinding.\n");
return;
}
session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
target_id = session->target_id;
session->target_id = ISCSI_MAX_TARGET;
spin_unlock_irqrestore(&session->lock, flags);
mutex_unlock(&ihost->mutex);
if (remove_target)
scsi_remove_target(&session->dev);
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");
spin_lock_irqsave(&session->lock, flags);
session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
spin_unlock_irqrestore(&session->lock, flags);
}
next prev parent reply other threads:[~2022-11-23 14:23 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-08 1:44 [PATCH v6] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace Wenchao Hao
2022-11-09 3:47 ` Mike Christie
2022-11-21 14:17 ` Wenchao Hao
2022-11-22 7:02 ` Antw: [EXT] " Ulrich Windl
2022-11-22 16:53 ` Mike Christie
2022-11-22 17:29 ` Wenchao Hao
2022-11-22 18:15 ` Mike Christie
2022-11-23 14:21 ` Wenchao Hao [this message]
2022-11-09 5:08 ` Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3a2ba8f0-3a86-36cf-cafd-969298594830@huawei.com \
--to=haowenchao@huawei.com \
--cc=cleech@redhat.com \
--cc=haowenchao22@gmail.com \
--cc=jejb@linux.ibm.com \
--cc=lduncan@suse.com \
--cc=linfeilong@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=michael.christie@oracle.com \
--cc=open-iscsi@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).