Hey,

If you base your fix on this patch here are some other comments.


Mike Christie wrote:

>>> ---
>>>  drivers/scsi/libiscsi_tcp.c |   29 +++++++++++++++++++++++++++++
>>>  1 files changed, 29 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
>>> index 2e0746d..83ddb44 100644
>>> --- a/drivers/scsi/libiscsi_tcp.c
>>> +++ b/drivers/scsi/libiscsi_tcp.c
>>> @@ -1000,6 +1000,30 @@ static struct iscsi_r2t_info 
>>> *iscsi_tcp_get_curr_r2t(struct iscsi_task *task)
>>>     return r2t;
>>>  }
>>>  
>>> +static int iscsi_tcp_check_tmf_task(struct iscsi_task *task)
>>> +{
>>> +   struct iscsi_conn *conn = task->conn;
>>> +   struct iscsi_tm *hdr = &conn->tmhdr;
>>> +   unsigned int hdr_lun, task_lun;
>>> +
>>> +   if (hdr->opcode != (ISCSI_OP_SCSI_TMFUNC | ISCSI_OP_IMMEDIATE))
>> Could you just mask the opcode off and not assume other bits are set or 
>> not set?
>>
>> If ((hdr->opcode & ISCSI_OPCODE_MASK) == ISCSI_OP_SCSI_TMFUNC)
>>
>>
>>
>>> +           return FAILED;
>> Could you not reuse the scsi eh return values here. Just do 0 and a EXXX 
>> value.
>>
>>
>>> +
>>> +   /* Check for matching LUN */
>>> +   hdr_lun = scsilun_to_int((struct scsi_lun *)hdr->lun);
>>> +   task_lun = scsilun_to_int((struct scsi_lun *)task->lun);
>>> +   if (hdr_lun != task_lun)

You will want to modify this so it only checks the lun if the tmf is a 
lu reset.

>>> +           return FAILED;
>>> +
>>> +   /* Check for matching task */
>>> +   if (ISCSI_TM_FUNC_VALUE(hdr) == ISCSI_TM_FUNC_ABORT_TASK) {
>>> +           if (task->cmdsn != hdr->refcmdsn)
>>> +                   return FAILED;
>>> +   }


You will want to add some check for data-outs when a abort is sent and 
fast_abort is set. The data-out does not have a cmdsn field, so this 
check would miss it.

>>> +
>>> +   return SUCCESS;
>>> +}
>>> +
>>>  /**
>>>   * iscsi_tcp_task_xmit - xmit normal PDU task
>>>   * @task: iscsi command task
>>> @@ -1032,6 +1056,11 @@ flush:
>>>     if (task->sc->sc_data_direction != DMA_TO_DEVICE)
>>>             return 0;
>>>  
>>> +   /* Check for pending TMF */
>>
>> Could you add a check if fast_abort is set then do this? If it is not 
>> set, it means targets want us to respond to r2ts while the tmf is in flight.
>>
>>
>>> +   if (conn->tmf_state != TMF_INITIAL &&
>>> +       iscsi_tcp_check_tmf_task(task) == SUCCESS)
>>> +           return 0;


You will want to move this test to the xmit_pdu call, so each pdu is 
checked.

And to take advantage of your new code you will want to modify the 
return value for iscsi_tcp_task_xmit so that it can indicate to libiscsi 
to not set the conn->task. If you leave it set, then we will continue 
retrying this same task, but with your code we can run tasks for other 
luns or send data for other tasks that are not affected by the tmf being 
run.

But for the conn->task change be careful about partially completed pdus. 
If you are in the middle of sending a pdu and then a tmf is queued, you 
cannot just break from the current pdu in the middle of it.

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~----------~----~----~----~------~----~------~--~---

Reply via email to