diff mbox series

[2/2] iscsi: set session to FREE state after unbind session in remove session

Message ID 20220414014947.4168447-3-haowenchao@huawei.com
State New
Headers show
Series Fix multiple iscsi session unbind event sent to userspace | expand

Commit Message

Wenchao Hao April 14, 2022, 1:49 a.m. UTC
__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(-)

Comments

Mike Christie April 14, 2022, 3:30 p.m. UTC | #1
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.

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);
Wenchao Hao April 15, 2022, 9:40 a.m. UTC | #2
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
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);
> 
> .
Mike Christie April 15, 2022, 3:17 p.m. UTC | #3
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);
>>
>> .
>
diff mbox series

Patch

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
+	 */
+	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);