From: Wenchao Hao <haowenchao@huawei.com>
To: Mike Christie <michael.christie@oracle.com>,
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>
Cc: <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: Mon, 21 Nov 2022 22:17:23 +0800 [thread overview]
Message-ID: <89692b2b-90f7-e8e8-fa77-f14dbe996b72@huawei.com> (raw)
In-Reply-To: <ad54a5dc-b18f-e0e6-4391-1214e5729562@oracle.com>
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:
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");
}
next prev parent reply other threads:[~2022-11-21 14:17 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 [this message]
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
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=89692b2b-90f7-e8e8-fa77-f14dbe996b72@huawei.com \
--to=haowenchao@huawei.com \
--cc=cleech@redhat.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).