Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <79683c37-904e-562f-fc18-98ffda777d08@huawei.com>
Date:   Fri, 28 Oct 2022 17:45:24 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH v5] scsi:iscsi: Fix multiple iscsi session unbind event
 sent to userspace
Content-Language: en-US
To:     Lee Duncan <lduncan@suse.com>, Chris Leech <cleech@redhat.com>,
        "Mike Christie" <michael.christie@oracle.com>,
        "James E . J . Bottomley" <jejb@linux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        <linux-scsi@vger.kernel.org>, <linux-kernel@vger.kernel.org>
CC:     Steffen Maier <maier@linux.ibm.com>, <liuzhiqiang26@huawei.com>,
        <linfeilong@huawei.com>
References: <20221028020026.2870104-1-haowenchao@huawei.com>
 <e6f03525-aad5-c20d-bd8c-a5647898bcf1@suse.com>
From:   Wenchao Hao <haowenchao@huawei.com>
In-Reply-To: <e6f03525-aad5-c20d-bd8c-a5647898bcf1@suse.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk


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?