Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <938bca13-2dcc-24c0-51b5-26f7e7238776@oracle.com>
Date:   Wed, 20 Apr 2022 11:28:37 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH v2] scsi: iscsi: Fix multiple iscsi session unbind event
 sent to userspace
Content-Language: en-US
To:     Wenchao Hao <haowenchao@huawei.com>, Lee Duncan <lduncan@suse.com>,
        Chris Leech <cleech@redhat.com>,
        "James E . J . Bottomley" <jejb@linux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        open-iscsi@googlegroups.com, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     linfeilong@huawei.com
References: <20220418000627.474784-1-haowenchao@huawei.com>
From:   Mike Christie <michael.christie@oracle.com>
In-Reply-To: <20220418000627.474784-1-haowenchao@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RXRGUURMNnlDZUhZdUZkNjM0aWszZmxpcm9iYXA0VGxZYW1sVWVJUzVrT2JF?=
 =?utf-8?B?ZTVEUjlibWhYYmJIcnlBRllIbjdLZEtrWkFrYWh3aVlPSndMMFo1SnBnVWor?=
 =?utf-8?B?ejVmL3NIVCs2U0hpNkVRWlZ5US95QzRablowWTkxVUZOSXByYW5aL3QwM25O?=
 =?utf-8?B?N09oNGpPeU5SZDBEdGNzNWpmSEdsQ2JxSm00djRmcTk1ZFNmUXNPRS9IZUtV?=
 =?utf-8?B?VFNCbTJBOXY0d3hNcFVNN1JUUDdNU3RxYmFIZjVLc3U1NjRlelZiV1p3TDFu?=
 =?utf-8?B?SjJyWVlwaU8vMTFSV1RwVmxoVGoyeERqVGJxRGVEQ3hwRFE0ZXgwQ2VQRnRy?=
 =?utf-8?B?ZjRpY1FaWi9lWWJwUVJnc2lyNS9sbzdhOTFPWHltamV2SHBNZTdESUd6SDl1?=
 =?utf-8?B?dEltbFpmTkptRkZGZWVxZFVvV2JFVGxsZUhMTENuRXBxbi9EbzJuWFl5WVlU?=
 =?utf-8?B?U051Z2E1aFNvMFQxMnl0SHJSNW1zWlpqZXhQUHQvZUdUWjY3MHhvRlNIOXRY?=
 =?utf-8?B?K2VsY296QWN6azlVeUxnYjZZWS9xOWdwUkc4Ykxhc21mZWZ1U2JucFBaUVNq?=
 =?utf-8?B?R2xxcG94S1BtSEErL1ZQNEc1dzNBWEtHclArajdSSm5UU08wWmIrNngvamEx?=
 =?utf-8?B?VFZMKzJQWGxpdlAyMFZrSlBUaStDajdZbU8xdkM0ZXE0OTJCNmhnUExlcUh5?=
 =?utf-8?B?MXNoTGxSUjA1OGV6OHE1Q2h3SktrRitHbHl5ZTBCeFFpbm9obGhXSloxQzZ1?=
 =?utf-8?B?REsweGRjSjVMa3BORmUvbzJpTU16RUo4TkUzSFJtMFdNaHE2Znh0RU14Wk9o?=
 =?utf-8?B?SVJSZ2dUOFQ2aEMrZGFxQ3I0cDJ1YUJqeFRxNnM4cVZGanM2aHhhVlVXUlNp?=
 =?utf-8?B?K1ZrdElsOGVTbUNacW9zaW5mOXU2emkrbFd3MEJvQTRVU3JsZEsyNWI2YTRF?=
 =?utf-8?B?aE5YNnBuVnVnVTdqemJtMVRVaDZzR0trdGl3SSs0UDBLdzk1QVFmUEdBTjRM?=
 =?utf-8?B?TEY5R0s5UnpuOXdOUnJiaU11T0VORGxxdUd2WFBTNDY0ZlpId2xBSzE5NlFw?=
 =?utf-8?B?NDhnNXRaQ3lQcThDNmZhVXgxYzY2a3lWb1JzamY3b3hOVTRhdmZ4RFJObWpH?=
 =?utf-8?B?VkJQaHp2bkVkQXFSMzg5dTJMRTZyR2tyK1MwcnM0TWhZRzRic1RRcFptWkhw?=
 =?utf-8?B?VUxCRU1NdTlYRlh2V3VIRUFpVitPU1g2UnduY0EvYjN1cFdIWndFcW94SmQ1?=
 =?utf-8?B?YWZuOUczaGJadGM1ajlNRk55Yll1TWY0T1d4Y294ZkZvbHRHQnlrTlRoeWM1?=
 =?utf-8?B?dXBrZ1FFSE9yVHVrdzZiN0RIZllDcjRPVmxKV285eVNYakpUWUY5UnNFRmkw?=
 =?utf-8?B?dk5vTGRGMlMzaGhVQzZ4Z1lkcTdNRGJKS1BDQ3JEODhaOVh3eURsZ1NmVEZp?=
 =?utf-8?B?aUNRRzhNS2E4NzQzS3pEdHhIQzZKYTNwREJXVDQyc3V2S2I2dE04c0tRT2E3?=
 =?utf-8?B?QVZsUHdwTzJJdTBxL2lIYW14eVYwbmo0RUNwZGNmNlJHY09UK3pDU1hCVlhs?=
 =?utf-8?B?Z1huSVc4bGd4L01GVzIvM2M5VXdmdGRmMUhIQzhtWENJbDZ6MzJ3WlBZTGhy?=
 =?utf-8?B?ektiK2ZOdWVWQ1pzMTJLbzc1TTNNMTdybmt6NnM3cTRiV2R3TVNhU29ONHgw?=
 =?utf-8?B?NHFneE53RGlMSWdKRlpQWkdjdCs0bnVTSDVHRDVwRkxUaC9Dc29Zb2NpRGoy?=
 =?utf-8?B?YWp0V1RrNERIV2ZGWU1PNG1uNWpMZm9LcnBvUGRmelRaR0tWa1dycjB3ZjNv?=
 =?utf-8?B?ZVc0MG5nUjBYRENmMXl3RW05eHRBeVl3Si9IVmgvMWNDV2V3NFhGVndtUTls?=
 =?utf-8?B?YkpsUWpWMXU3RUtnQkZRMm5XUW5lTnB1T080U3lZNGNMU1U4ZjZvblB2ZDRK?=
 =?utf-8?B?U05wcU9IVzVLVWoyT09Wa3NsNkhhemdIQW9iOEQrS2FsdTJPbFkvTDU4b3px?=
 =?utf-8?B?YlBzVDJXTHRkL3RRMmRYSlVjZ085cmtKNjIzZ3FKOWZhcTRUSkRWdW92VFdK?=
 =?utf-8?B?NjJIaTJ1S3RLT3hNc3RsK1B5SzFYUVNaTjdoaFgrRzBBNmRPUlNidXhONmZY?=
 =?utf-8?B?NTAwM0hqeVJEQ2FScnlUdlowdUx6Ti9UNkJVb2twSjNUZWVlK2VPSTA1SCsv?=
 =?utf-8?B?cmVyYTRFcThJakpPKzlYMENLOEMxSG5GVkZTSXJOQnA5T3crL3dKcGE5djJT?=
 =?utf-8?B?NEE4akFmald2dGtBTWVUM1NMZXllU0pvUG9aUkR4WHpoWk9LelBkaU5xNmJn?=
 =?utf-8?B?R2pUOUQveWNZUXVSZWd2Z21CbDNjYkYrTmZiVXlRazRTWkw5YUF0RDI5RzU1?=
 =?utf-8?Q?ECEhLTjtNGOmeR9A=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3ef6b0ee-d76b-4a59-ea30-08da22ead3c5
X-MS-Exchange-CrossTenant-AuthSource: DM5PR10MB1466.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Apr 2022 16:28:39.8251
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: VjasT0+ZH167whf2e5Py+rd3jgOLAyarHkvomTiwxWpxYHdvLwnLx400SkhZslHWsRqs9su0ZnjbKtDGqLBhUEHsBFD1vjVn8gR69EXysP0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5422
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-20_04:2022-04-20,2022-04-20 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 suspectscore=0
 malwarescore=0 mlxlogscore=999 adultscore=0 mlxscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2204200097
X-Proofpoint-ORIG-GUID: 26w_kTCShLVlA9Szzfgrd5x6BmaDkwtS
X-Proofpoint-GUID: 26w_kTCShLVlA9Szzfgrd5x6BmaDkwtS
X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 4/17/22 7:06 PM, Wenchao Hao wrote:
> I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
> for multiple times which should be fixed.
> 
> This patch introduce target_unbound in iscsi_cls_session to make
> sure session would send only one ISCSI_KEVENT_UNBIND_SESSION.
> 
> But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi:
> Report unbind session event when the target has been removed"). The issue
> is iscsid died for any reason after it send unbind session to kernel, once
> iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event.
> 
> Now kernel think iscsi_cls_session has already sent an
> ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which
> would cause userspace unable to logout. Actually the session is in
> invalid state(it's target_id is INVALID), iscsid should not sync this
> session in it's restart.
> 
> So we need to check session's target unbound state during iscsid restart,
> if session is in unbound state, do not sync this session and perform
> session teardown. It's reasonable because once a session is unbound, we
> can not recover it any more(mainly because it's target id is INVALID)
> 
> Changes from V1:
> - Using target_unbound rather than state to indicate session has been
>   unbound
> 
> Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 21 +++++++++++++++++++++
>  include/scsi/scsi_transport_iscsi.h |  1 +
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 2c0dd64159b0..43ba31e595b4 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1958,6 +1958,14 @@ static void __iscsi_unbind_session(struct work_struct *work)
>  
>  	ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>  
> +	spin_lock_irqsave(&session->lock, flags);
> +	if (session->target_unbound) {
> +		spin_unlock_irqrestore(&session->lock, flags);
> +		return;
> +	}
> +	session->target_unbound = 1;

Shoot, sorry, I think I gave you a bad review comment when I said we
could do a bool or state kind or variable.

If we set unbound here and iscsid was restarting at this point then
iscsid really only knows the target removal process is starting up. It
doesn't know that the target is not yet removed. We could be doing sync
caches and/or still tearing down scsi_devices/LUNs.

For the comments I gave you on the userspace PR parts, would it be
easier if this was a state type of value? Above you would set it to
REMOVING. When scsi_remove_target is done then we can set it to
REMOVED. That combined with the session and conn states we can detect
how far we got in the session removal process if iscsid dies in the
middle of it.

What do you think?


> +	spin_unlock_irqrestore(&session->lock, flags);
> +
>  	/* Prevent new scans and make sure scanning is not in progress */
>  	mutex_lock(&ihost->mutex);
>  	spin_lock_irqsave(&session->lock, flags)

...

> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
> index 9acb8422f680..877632c25e56 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -256,6 +256,7 @@ struct iscsi_cls_session {
>  	struct workqueue_struct *workq;
>  
>  	unsigned int target_id;
> +	int target_unbound;   /* make sure unbind session only once */


We don't need the comment since the code using this is so simple
and the name of the variable tells us what it's for.


>  	bool ida_used;
>  
>  	/*