Mike Christie wrote:
> 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.
> 
Why not for ABORT TASK? Surely the LUN must be identical there, too?

>>>> +          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.
> 
Okay, haven't looked to closely here. My test harness is sending LU Resets,
so this path isn't really exercised.

>>>> +
>>>> +  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.
> 
Ok, will be doing it.

> 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.
> 
Oh, ok.

> 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.
> 
Sigh. Why do you have to make is so complicated ...
My patch was easy and simple originally. And now this :-)

Alright, will see to have it updated.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                   zSeries & Storage
h...@suse.de                          +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

--~--~---------~--~----~------------~-------~--~----~
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