RE: [PATCH v2] target: core: remove from tmr_list at lun unlink

From: Dmitriy Bogdanov <d.bogdanov@yadro.com>
To: Mike Christie <michael.christie@oracle.com>,
	Martin Petersen <martin.petersen@oracle.com>,
	"target-devel@vger.kernel.org" <target-devel@vger.kernel.org>
Cc: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	"linux@yadro.com" <linux@yadro.com>,
	Roman Bolshakov <r.bolshakov@yadro.com>
Subject: RE: [PATCH v2] target: core: remove from tmr_list at lun unlink
Date: Wed, 21 Apr 2021 15:07:09 +0000	[thread overview]
Message-ID: <39c7c7c7671742c183cfddd4a4a68ee9@yadro.com> (raw)
In-Reply-To: <a91500ce-5ff8-8f97-03dc-74ae097d22e2@oracle.com>

Hi Mike,

>>> --- a/drivers/target/iscsi/iscsi_target.c
>>> +++ b/drivers/target/iscsi/iscsi_target.c
>>> @@ -2142,7 +2142,7 @@ iscsit_handle_task_mgt_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
>>>  	 * TMR TASK_REASSIGN.
>>>  	 */
>>>  	iscsit_add_cmd_to_response_queue(cmd, conn, cmd->i_state);
>>> -	target_put_sess_cmd(&cmd->se_cmd);
>>> +	transport_generic_free_cmd(&cmd->se_cmd, false);
>>>  	return 0;
>>>  }
>> 
>> Doh. I see how I got all confused. I guess this path leaks the lun_ref 
>> taken by transport_lookup_tmr_lun. It looks like an old issue and 
>> nothing to do with your patch.

>> I'm not sure if we are supposed to be calling 
>> transport_generic_free_cmd twice. It looks like it works ok, because your patch added the "cmd->se_lun = NULL"
>> in transport_lun_remove_cmd, so we won't do a double list deletion.
>> It feels dirty though. I can feel Bart saying, "Mike did you see the 
>> comment at the top of the function". :)
>> 
>> Maybe it's best to more cleanly unwind what was setup in the rror 
>> path. I think we can fix lun_ref leak too.
>> 
>> So instead of doing transport_generic_free_cmd above do 
>> transport_lun_remove_cmd to match/undo the transport_lookup_tmr_lun call in iscsit_handle_task_mgt_cmd?
>
>Shoot. I'm all over the place. I think the root issue is my original comment on the v1 patch was wrong.
>On a failure we would still do:
>iscsit_free_cmd -> transport_generic_free_cmd -> transport_lun_remove_cmd
>right? So we don't need any change in the iscsi target. It should all just work.

iscsit_free_cmd will be called only at response completion(next incoming iscsi command): iscsit_ack_from_expstatsn -> iscsit_free_cmd.
That produces some unacceptable delay of lun unlinking from cmd. There is a bug report to the similar behavior:
http://lkml.iu.edu/hypermail/linux/kernel/2002.0/05272.html
Because of that complain, the commit 83f85b8ec305, that solves the same crash as I am fixing,  was reverted.

So, this piece of patch has some indirect relation :)
I will extract it to a separate patch in the coming patchset on TMF handling.

BR,
 Dmitry