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. */


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