Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <8d3c80f0-7ec1-4b6e-3e3f-87be1ef8c8cb@oracle.com>
Date:   Fri, 15 Apr 2022 10:17:40 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH 2/2] iscsi: set session to FREE state after unbind session
 in remove session
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: <20220414014947.4168447-1-haowenchao@huawei.com>
 <20220414014947.4168447-3-haowenchao@huawei.com>
 <a8087705-2cea-f01c-ce67-639e97edc30a@oracle.com>
 <f587206a-4479-1748-9211-086d79249b95@huawei.com>
From:   Mike Christie <michael.christie@oracle.com>
In-Reply-To: <f587206a-4479-1748-9211-086d79249b95@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?UmFCSU9GcWM3QVN5R3hEVXpxeDJhZjEyazJ5NFA3emx0V3JScGlUeEtTUWJK?=
 =?utf-8?B?SlJ6dTR1emNLZzlxeUZCVHM4bUtqNFJRSjF2WlVLbTBFWEFnOGhnb1JUVWJv?=
 =?utf-8?B?bUd3NWNGOXZZeVpNTWVOODRlZ01UcjVxV0RBemNVUGk3ek5kOGxzQlIreGZK?=
 =?utf-8?B?VFlmNkZDelFHWituTzJFTkV1NFdrRmJqZVZUSDkrMkVPaHhqNnhhOU1BYXB4?=
 =?utf-8?B?QURTM2orRWJmNHdoWFdxRGczamtsRHdwSENWZlBOUzlLZ2t5cHNwKzFtRkZ6?=
 =?utf-8?B?MmdoSFd6NUdJMFJpd2p0RE5ickp1OHJZRFVWdjNNZXRpTnp3V0xDamRoeUNu?=
 =?utf-8?B?RnRuWVZlUkhnNXBhcnc3UjlGcTVvYzUyekxsTCtwb3E1bVJuaWxnOHM0SndG?=
 =?utf-8?B?enBHZmxVeE93bDkwenc4eDhPcGdmeWNxa21uVXFxNmdyVzc4QWo1M0hlWHpz?=
 =?utf-8?B?VGtlTEJCVUd3Q0xaWmk2T1poSHYxdEduK0JZMG5NTUpzS3RXNnhZeHhLNThG?=
 =?utf-8?B?R1oxYnhvKy9WMjdKZUlDa1FHbGI3LzVLY1pZcG5rZXc2bXRPWGhDQWhiKzR0?=
 =?utf-8?B?WnlIQUlLVzhYdThTMkNaNDgxclhwbFNpMFkzQjNsK2RISW5HTjljZjlaZ0c1?=
 =?utf-8?B?Z1NlNjNTNmpHblVIOW9FOEt2OWFMd0krU20xUEhwYjA4RHI5c0tROFBGWDQ0?=
 =?utf-8?B?Zlp6ZW5leEdSeUdROTNIeEFHSHlyMlErT0laUGEvS1Q2VU1BTGZRNEthbGZ2?=
 =?utf-8?B?cDdIT3dvL21Sd09OcFNIRDdhOEFRTTBtUGYzUkF2aW0xM3ZyMWNQQW15eTRR?=
 =?utf-8?B?c3pDSW9OVUk5TlhYa1RaZWFQdzk0SVhOYXo3SEREdy9YR0oxVGRPOUhpZy9m?=
 =?utf-8?B?ZTE5WUFoTTFhZ0xhN2NTTFpYMEx2NStkczV2ZDJMM2pCRHdPODV1a2g2MEQz?=
 =?utf-8?B?aWxyTnJqbjJiTEVocmwrR3p6dEtZeGR0ck5mcVpsVFd0d2lNTlJ6ZzFaVmZ1?=
 =?utf-8?B?alVrSittVkFrMzZGeWh0cnlsaEpUMVdnODRCQmFYcVRrWnA5NjNLajJUK0VK?=
 =?utf-8?B?VWxTbUVZdXlHQ1c2L25QdDNoK256bGlHemV6SFRVYW1YanJxV01UQW9TdFNr?=
 =?utf-8?B?a2Vra0hxcEFGOVh4dE1USFFDSkdBYllkNEJZNFdKaEc1K2dFaU5aTE00SURa?=
 =?utf-8?B?eC9EenIvSlJHdWhYRU9VMmNsMUNmd0I4bHEydEhBSE53enc5RHFiQmVYWjhG?=
 =?utf-8?B?L0VrMjZocXo1bXZWK2YyUzU0RXNTMHhWTmFCYyttSFJ3K1J2Q25NaDZCcGQ0?=
 =?utf-8?B?VGRXU3pOcStyQ3NEeU51VUNLSnB6UUxsZDRHRllacWE1RVVrRFovVHpIMytZ?=
 =?utf-8?B?Zm9iKzd6QmpTSkVHaitTL1c0ZE9ETFhldTFWOFJYYTlxSkNyZkMwMlAyVW5X?=
 =?utf-8?B?d1NNQkR1ekJuQWcwYnNUTnd6UzhPRG5pUzY5YzMrLy9QZm14TmhYa0NuN0hh?=
 =?utf-8?B?elBWMmJOWmFhNithRjZudE0yYTBuZm5LZ1VZeXVlZnZGQ3ZYRTFUbU16S1pQ?=
 =?utf-8?B?M2ZGMWt2dUx5YXIxVlBDNUNEd2lHRThmdVNPNHVoVjY1MlR2dHF6Um1YdGhO?=
 =?utf-8?B?NEQzeXVVSlZ5VVVENnlyQzJDLzEvd1JYNWZLaklMaVBoVGFMY3hwekNxam5q?=
 =?utf-8?B?Z2l2S0Vvb1BtRHJJZWRkL0Z5eGFxUTFHSjdlZUN0d2lZQXplRDZQVlVLNU9t?=
 =?utf-8?B?MUJ4RUR3QUJsNUFBRUVCTDVxRUZqNGNQcmNGVDVSdXVGTjF3R0x4STZvQ2Fj?=
 =?utf-8?B?c2pPSjh3SHVjemVkaGhPYjh3cE9pTnNQNmtMcHZSa3dtY0JuaFE4R2VNUm83?=
 =?utf-8?B?cFVEQW9Ka3hKQ1BKSW1rSC9VajFCQ1Z1RmlJM1hlTU9PaExLSUhkclJ2a1RT?=
 =?utf-8?B?Q01Gazg2US95Q2orbVpYbmY1aEo0N0tlUVMwOHk5Nk5TTzh0RC9vYkxrNkVD?=
 =?utf-8?B?NTFyQ1ovUUVabWJGSEFndXhZRVV3V2EyTzZrT3hIS1hqWEliVzUyMzRJV0Na?=
 =?utf-8?B?Z3hEZktQeTlBQ21yMXJUaEExTUhHM0gvMTEwUUZNekZTd2E4MUthRDg0MlRl?=
 =?utf-8?B?SE1NMDZCYTNZdWowRE9SMW9kTUVnNVBIVHFMNUNvVkNUS2sxUmVDKzVDWnZm?=
 =?utf-8?B?VGdnY0hBWlRPWFpTTHA2RmZmRVUyd2RRU1ZXN2lVN2hiZmFJeVFCc3hPYkxv?=
 =?utf-8?B?VGtNdWJzRlBKKzdlMmVRU2Z2dHlZckE5bXNjM2F4S09hN1VnTEw1ZWQrSW5h?=
 =?utf-8?B?cVZKVVdtSVdKV0NOQ0pOWUhEbnBKOFVTL1lRM0hmWGZ0S05kbVpJV3NVREpH?=
 =?utf-8?Q?gG8fqOWPMCbGyQAg=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9b0178d5-7d57-48c0-82b5-08da1ef31620
X-MS-Exchange-CrossTenant-AuthSource: DM5PR10MB1466.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Apr 2022 15:17:42.5649
 (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: WyPTe66QkShQRgSg5xntpc6RATIF/AQX/bm79W1h8ItaDoYfgc2ogM6/wl5qdSooBIIYMS6iiBn6grU7aYG62/5YVTE71mHjPMCwzs4BmHc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB4960
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-15_01:2022-04-14,2022-04-15 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0
 malwarescore=0 mlxlogscore=999 spamscore=0 adultscore=0 phishscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2204150090
X-Proofpoint-ORIG-GUID: gpy_jpmXIQJZmMBNHF3u_obmfJagi3d4
X-Proofpoint-GUID: gpy_jpmXIQJZmMBNHF3u_obmfJagi3d4
X-Spam-Status: No, score=-5.8 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,
	T_SCC_BODY_TEXT_LINE 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/15/22 4:40 AM, Wenchao Hao wrote:
> On 2022/4/14 23:30, Mike Christie wrote:
>> On 4/13/22 8:49 PM, Wenchao Hao wrote:
>>> __iscsi_unbind_session() set session state to ISCSI_SESSION_UNBOUND, which
>>> would overwrite the ISCSI_SESSION_FREE state.
>>>
>>> Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
>>> ---
>>>  drivers/scsi/scsi_transport_iscsi.c | 26 ++++++++++++++++----------
>>>  1 file changed, 16 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
>>> index 97a9fee02efa..d8dd9279cea8 100644
>>> --- a/drivers/scsi/scsi_transport_iscsi.c
>>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>>> @@ -2173,6 +2173,22 @@ void iscsi_remove_session(struct iscsi_cls_session *session)
>>>  	if (!cancel_work_sync(&session->block_work))
>>>  		cancel_delayed_work_sync(&session->recovery_work);
>>>  	cancel_work_sync(&session->unblock_work);
>>> +
>>> +	scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE);
>>> +	/*
>>> +	 * qla4xxx can perform it's own scans when it runs in kernel only
>>> +	 * mode. Make sure to flush those scans.
>>> +	 */
>>> +	flush_work(&session->scan_work);
>>> +
>>> +	/*
>>> +	 * flush running unbind operations
>>> +	 * if unbind work did not queued, call __iscsi_unbind_session
>>> +	 * directly to perform target remove
>>
>> We probably don't need the flush_work test because we are going to
>> normally call __iscsi_unbind_session.
>>
> 
> I think we still need calling flush_work here. The introduce of flush_work

Above I was saying we don't need to *test* if it returned true/false.
Just always flush and then call __iscsi_unbind_session like we did
originally below. The check you added in __iscsi_unbind_session in the
first patch will detect if it's been called or not.


> is to make sure sysfs objects are removed in an correct order. There is a
> very low probability that __iscsi_unbind_session() triggered by queue_work()
> has not been finished, and iscsi_remove_session() is called. So we need
> flush_work() to make sure __iscsi_unbind_session() has done if it has been
> activated by queue_work().
> 
>> If the unbind work had already run, which is the normal case, then
>> flush_work returns false and we end up calling __iscsi_unbind_session
>> like before. That function then checks if the target is really unbound.
>> So the extra check doesn't normally buy us anything with your patches
>> because in patch 1 you fixed it so __iscsi_unbind_session doesn't send
>> the extra event.
>>
>>
>>> +	 */
>>> +	if (!flush_work(&session->unbind_work))
>>> +		__iscsi_unbind_session(&session->unbind_work);
>>> +
>>>  	/*
>>>  	 * If we are blocked let commands flow again. The lld or iscsi
>>>  	 * layer should set up the queuecommand to fail commands.
>>> @@ -2183,16 +2199,6 @@ void iscsi_remove_session(struct iscsi_cls_session *session)
>>>  	session->state = ISCSI_SESSION_FREE;
>>>  	spin_unlock_irqrestore(&session->lock, flags);
>>>  
>>> -	scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE);
>>> -	/*
>>> -	 * qla4xxx can perform it's own scans when it runs in kernel only
>>> -	 * mode. Make sure to flush those scans.
>>> -	 */
>>> -	flush_work(&session->scan_work);
>>> -	/* flush running unbind operations */
>>> -	flush_work(&session->unbind_work);
>>> -	__iscsi_unbind_session(&session->unbind_work);
>>> -
>>>  	/* hw iscsi may not have removed all connections from session */
>>>  	err = device_for_each_child(&session->dev, NULL,
>>>  				    iscsi_iter_destroy_conn_fn);
>>
>> .
>