On Sun, Dec 20, 2009 at 3:57 PM, Erez Zilber <erezzi.l...@gmail.com> wrote:
> On Thu, Dec 17, 2009 at 5:01 AM, Mike Christie <micha...@cs.wisc.edu> wrote:
>> Erez Zilber wrote:
>>> On Tue, Dec 15, 2009 at 10:34 AM, Mike Christie <micha...@cs.wisc.edu> 
>>> wrote:
>>>> Erez Zilber wrote:
>>>>> Maintain a list of nop-out PDUs that almost timed out.
>>>>> With this information, you can understand and debug the
>>>>> whole system: you can check your target and see what caused
>>>>> it to be so slow on that specific time, you can see if your
>>>>> network was very busy during that time etc.
>>>>>
>>>>> Signed-off-by: Erez Zilber <erezzi.l...@gmail.com>
>>>>>
>>>> Sorry for the late reply. Thanks for doing this work!
>>>>
>>>>
>>>> @@ -88,11 +89,12 @@ static struct option const long_options[] =
>>>>         {"killiscsid", required_argument, NULL, 'k'},
>>>>         {"debug", required_argument, NULL, 'd'},
>>>>         {"show", no_argument, NULL, 'S'},
>>>> +       {"noopinfo", required_argument, NULL, 'N'},
>>>>         {"version", no_argument, NULL, 'V'},
>>>>         {"help", no_argument, NULL, 'h'},
>>>>         {NULL, 0, NULL, 0},
>>>>  };
>>>>
>>>>
>>>> Do you think you want something more generic like a get-debug-info type
>>>> of feature? Maybe in the future we could also show how many times a scsi
>>>> command has timed out or almost timed out (user can adjust scsi timer
>>>> then) or how many times the scsi eh has fired and how many times a
>>>> abort, lu or target reset has timed out?
>>>>
>>>> So maybe it would be get like get stats where then the noop info is the
>>>> first stat and in the future we will have more?
>>>
>>> Sounds good. Let's change it to {"debuginfo", required_argument, NULL, 
>>> 'i'}. OK?
>>
>> Ok with me.
>>
>>
>>>>
>>>>
>>>>
>>>> @@ -994,8 +997,25 @@ static int iscsi_nop_out_rsp(struct iscsi_task *task,
>>>>                 if (iscsi_recv_pdu(conn->cls_conn, (struct iscsi_hdr *)nop,
>>>>                                    data, datalen))
>>>>                         rc = ISCSI_ERR_CONN_FAILED;
>>>> -       } else
>>>> +       } else {
>>>> +               ping_delay = jiffies - conn->last_ping;
>>>> +
>>>> +               /* Check if the ping has almost timed out */
>>>> +               if (ping_delay >=
>>>> +                   (session->noop_threshold * (conn->ping_timeout * HZ)) /
>>>> +                   100) {
>>>> +                       mutex_lock(&conn->noop_info_mutex);
>>>>
>>>>
>>>>
>>>> We annot use a mutex here, because we run from a bottom half (softirq
>>>> for iscsi and a tasklet for iser), and we cannot sleep in a bh since
>>>> there is no process context.
>>>
>>> You're right. I'll replace the mutex with a spinlock.
>>>
>>>>
>>>> +                       idx = conn->noop_info_arr_idx;
>>>> +                       conn->noop_info_arr[idx].tv = conn->tmp_noop_tv;
>>>> +                       conn->noop_info_arr[idx].elapsed_time_msec =
>>>> ping_delay;
>>>>
>>>> I think ping_delay is just in jiffies, so you have to do some sort of
>>>> jiffies_to_msec call (I cannot rememeber the name of the helper but
>>>> there is one).
>>>
>>> I think it should be: conn->noop_info_arr[idx].elapsed_time_msec =
>>> ping_delay * HZ / 1000
>>
>>
>>
>> I think the function is jiffies_to_msecs()
>>
>>
>>
>>>
>>>>
>>>> +                       conn->noop_info_arr[idx].init = 1;
>>>> +                       conn->noop_info_arr_idx =
>>>> +                               (conn->noop_info_arr_idx + 1) %
>>>> NOOP_INFO_ARR_LEN;
>>>>
>>>>
>>>> I am not getting the reason for the division?
>>>
>>> Are you talking about "conn->noop_info_arr_idx =
>>> (conn->noop_info_arr_idx + 1) % NOOP_INFO_ARR_LEN"? It's a cyclic
>>> array.
>>>
>>
>> Ah, nevermind :)
>>
>> --
>>
>
> Attached with the fixes that we discussed. I tested it with a 2.6.18
> kernel. I was able to apply the following patches:
>
> * 2.6.14-23_compat.patch
> * 2.6.24_compat.patch
> * 2.6.26_compat.patch
> * 2.6.27_compat.patch
>
> What about the following patches?
> * 2.6.14-19_compat.patch
> * 2.6.16-suse.patch
> * 2.6.20-21_compat.patch
>
> Are they still maintained? If not, can we throw (at least some of) them away?
>
> Erez
>

Found a small (and stupid) bug in my patch:

diff --git a/usr/iscsiadm.c b/usr/iscsiadm.c
index 6188f43..ee4cfc4 100644
--- a/usr/iscsiadm.c
+++ b/usr/iscsiadm.c
@@ -94,7 +94,7 @@ static struct option const long_options[] =
        {"help", no_argument, NULL, 'h'},
        {NULL, 0, NULL, 0},
 };
-static char *short_options = "RlVhm:p:P:T:H:I:U:k:L:d:r:n:v:o:sSt:u:i:";
+static char *short_options = "RlVhm:p:P:T:H:I:U:k:L:d:r:n:v:o:sSt:ui:";

 static void usage(int status)
 {

Mike - I will send an updated patch after you review the rest of the patch.

Erez

--

You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To post to this group, send email to open-is...@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?hl=en.


Reply via email to