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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux SCSI]     [Kernel Newbies]     [Linux SCSI Target Infrastructure]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Device Mapper]

  Powered by Linux