Hannes Reinecke wrote:
> 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?

It is. That is not the reason for the comment.

Currently when we send a tmf, the initiator basically stops all IO (at 
least that is what it wanted to do but you found a bug). I thought the 
goal of this patch was to allow as much IO as possible. If you check the 
LUN while an abort is sent, then you would prevent all IO to the LUN 
from being executed instead of just the PDUs related to the specific 
command being aborted. So while a abort is sent you do not want data-out 
pdus sent for the task and you of course do not want the scsi cmd pdu 
(we check that already before we prep the tmf). But you can send new 
scsi cmd pdus for new tasks or send data-outs for other tasks.

If we just wanted to stop all IO then you could just set fast_abort and 
go with my patch dont-exec-on-success.patch, which stops new commands 
from being executed while the tmf response is being processed 
(previously we only stopped IO while the TMF was in progress).



> 
>>>>> +         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 :-)
> 

If you just want IO stopped then the attached patch 
dont-exec-on-success.patch I sent should plug up the last hole. Did you 
try it? For the normal scsi eh path, this should be fine, because we are 
only run when the host is stopped and IO perf is going to be poor. If a 
lu reset or other tmf is injected with sg_reset for clustering then 
probably you want to keep IO going and not assume the host is almost 
dead, so your approach is nice.

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

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index a751f62..80e5c1f 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1295,6 +1295,12 @@ check_mgmt:
 
 	/* process pending command queue */
 	while (!list_empty(&conn->cmdqueue)) {
+		/* when this transistions back to initial the eh will
+		 * start us up again
+		 */
+		if (conn->tmf_state == TMF_SUCCESS)
+			break;
+
 		if (conn->tmf_state == TMF_QUEUED)
 			break;
 
@@ -1329,6 +1335,12 @@ check_mgmt:
 	}
 
 	while (!list_empty(&conn->requeue)) {
+		/* when this transistions back to initial the eh will
+		 * start us up again
+		 */
+		if (conn->tmf_state == TMF_SUCCESS)
+			break;
+
 		if (conn->session->fast_abort && conn->tmf_state != TMF_INITIAL)
 			break;
 

Reply via email to