Mike Christie wrote:
> Hannes Reinecke wrote:
>> iscsi_tcp_task_xmit() doesn't check for pending TMF
>> tasks, so we might happily continue sending R2T data
>> even though we've already aborted the command.
>>
>> Signed-off-by: Hannes Reinecke <h...@suse.de>
> 
> 
> The patch is better than how we stop all r2t processing right now so 
> even if the problem is just us not checking the suspend bit right, I 
> think this patch makes a nice improvement.
> 
> Some comments.
> 
> 
>> ---
>>  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)
>> +            return FAILED;
>> +
>> +    /* Check for matching task */
>> +    if (ISCSI_TM_FUNC_VALUE(hdr) == ISCSI_TM_FUNC_ABORT_TASK) {
>> +            if (task->cmdsn != hdr->refcmdsn)
>> +                    return FAILED;
>> +    }
>> +
>> +    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;
>> +
>>      r2t = iscsi_tcp_get_curr_r2t(task);
>>      if (r2t == NULL) {
>>              /* Waiting for more R2Ts to arrive. */
> 

And then you can also remove this:

                 if (conn->session->fast_abort && conn->tmf_state != 
TMF_INITIAL)
                         break;


in iscsi_data_xmit

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