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

From: Mike Christie
Date: Fri Apr 15 2022 - 11:18:05 EST


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@xxxxxxxxxx>
>>> ---
>>> 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);
>>
>> .
>