[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c90423a5-b89a-6c51-20f5-4945b85349b2@suse.com>
Date: Fri, 28 Oct 2022 08:05:04 -0700
From: Lee Duncan <lduncan@...e.com>
To: Wenchao Hao <haowenchao@...wei.com>,
Chris Leech <cleech@...hat.com>,
Mike Christie <michael.christie@...cle.com>,
"James E . J . Bottomley" <jejb@...ux.ibm.com>,
"Martin K . Petersen" <martin.petersen@...cle.com>,
linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: Steffen Maier <maier@...ux.ibm.com>, liuzhiqiang26@...wei.com,
linfeilong@...wei.com
Subject: Re: [PATCH v5] scsi:iscsi: Fix multiple iscsi session unbind event
sent to userspace
On 10/28/22 02:45, Wenchao Hao wrote:
>
> On 2022/10/28 0:18, Lee Duncan wrote:
>> On 10/27/22 19:00, Wenchao Hao wrote:
>>> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
>>> index cd3db9684e52..2e0d1cd6d4ea 100644
>>> --- a/drivers/scsi/scsi_transport_iscsi.c
>>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>>> @@ -1676,6 +1676,30 @@ static const char *iscsi_session_state_name(int state)
>>> return name;
>>> }
>>> +static struct {
>>> + int value;
>>> + char *name;
>>> +} iscsi_session_target_state_names[] = {
>>> + { ISCSI_SESSION_TARGET_UNBOUND, "UNBOUND" },
>>> + { ISCSI_SESSION_TARGET_ALLOCATED, "ALLOCATED" },
>>> + { ISCSI_SESSION_TARGET_BOUND, "BOUND" },
>>> + { ISCSI_SESSION_TARGET_UNBINDING, "UNBINDING" },
>>> +};
>>> +
>>> +static const char *iscsi_session_target_state_name(int state)
>>> +{
>>> + int i;
>>> + char *name = NULL;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(iscsi_session_target_state_names); i++) {
>>> + if (iscsi_session_target_state_names[i].value == state) {
>>> + name = iscsi_session_target_state_names[i].name;
>>> + break;
>>> + }
>>> + }
>>> + return name;
>>> +}
>>
>> It seems like it might be more efficient to use the target state as the array index, so you don't have to loop to find the name, e.g. something like:
>>
>>> static char* iscsi_session_target_state_names[] = {
>>> .ISCSI_SESSION_TARGET_UNBOUND = "UNBOUND",
>>> .ISCSI_SESSION_TARGET_ALLOCATED = "ALLOCATED",
>>> .ISCSI_SESSION_TARGET_BOUND = "BOUND",
>>> .ISCSI_SESSION_TARGET_UNBINDING = "UNBINDING",
>>> };
>>
>> I know there are only 4 states, and it's only used for sysfs, so not sure it matters much.
>>
>
> It's a better implement, I would update it.
>
>>> @@ -1961,6 +1987,15 @@ static void __iscsi_unbind_session(struct work_struct *work)
>>> unsigned long flags;
>>> unsigned int target_id;
>>> + spin_lock_irqsave(&session->lock, flags);
>>> + if (session->target_state != ISCSI_SESSION_TARGET_BOUND) {
>>> + spin_unlock_irqrestore(&session->lock, flags);
>>> + ISCSI_DBG_TRANS_SESSION(session, "Abort unbind sesison\n");
>>
>> It'd be nice if this said more, since debugging "Abort unbind sessions" would require finding the sources. How about "Abort unbind session: not bound", for example?
>>
>
> Of course, I would updated.
>
>>> @@ -264,6 +271,7 @@ struct iscsi_cls_session {
>>> */
>>> pid_t creator;
>>> int state;
>>> + int target_state; /* session target bind state */
>>> int sid; /* session id */
>>> void *dd_data; /* LLD private data */
>>> struct device dev; /* sysfs transport/container device */
>>
>> Thank you for sticking with this. It is very much appreciated.
>
> I should apologize for taking this issue off for a long time because of my slow response.
>
> What's more, should I add an acked-by or reviewed-by in my next patch?
Please add my Reviewed-by tag after you make these changes.
And no problem taking a while. I appreciate your effort!
--
Lee Duncan
Powered by blists - more mailing lists