On Fri, Jan 29, 2010 at 8:59 PM, Mike Christie <micha...@cs.wisc.edu> wrote:
> On 01/28/2010 09:25 AM, Erez Zilber wrote:
>>>
>>>
>>> +struct iscsi_noop_info {
>>> +       struct timeval tv;
>>>
>>>
>>> Can you pass this between userspace and the kernel safely? The fields in
>>> there are longs, and I thought those could be different sizes if you had
>>> something like 32bit userspace and 64 kernels.
>>
>> What do you suggest to do here?
>
> I do not know. Am I even right? After I sent that, I was thinking syscalls
> must pass it around ok, so it must be ok or there must be some way to do it.
> I did not look into it any more though.

I must say that I don't know how to handle the scenario of 32bit
userspace and 64 kernels.

>
>
>> I'm not very familiar with debugfs. What do you want to do with it?
>>
>
> Nothing. I was just saying it is an option. If you were also fine with the
> netlink interface you used, so am I, so no changes needed.
>

The following patch (built over the original patch) fixes the problems
that we discussed (except for the 32-64 problem). If you're okay with
it, I will send a single updated patch.

diff --git a/doc/iscsiadm.8 b/doc/iscsiadm.8
index 6f85cb5..9f60beb 100644
--- a/doc/iscsiadm.8
+++ b/doc/iscsiadm.8
@@ -126,14 +126,14 @@ operator.

 .TP
 \fB\-i\fR, \fB\-\-debuginfo=\fIdebug_info_op\fR
-Specifies a debug infomration operation \fIdebug_info_op\fR.
\fIdebug_info_op\fR must be \fIdump\fR or \fIclear\fR.
+Specifies a debug infomration operation \fIdebug_info_op\fR.
\fIdebug_info_op\fR must be \fIdump_noop\fR or \fIclear_noop\fR.
 .IP
 This option is valid only for the session mode.
 .IP
-\fIdump\fR prints a list of recent Nop-out PDUs that almost timed out
with information about the time that the Nop-out PDU was sent and its
delay.
+\fIdump_noop\fR prints a list of recent Nop-out PDUs that almost
timed out with information about the time that the Nop-out PDU was
sent and its delay.
 A Nop-out PDU (that did not time out) is defined as 'almost timed
out' if its delay is greater than node.session.noop_threshold *
node.conn[0].timeo.noop_out_timeout.
 .IP
-\fIclear\fR clears the list of recent Nop-out PDUs that almost timed
out.
+\fIclear_noop\fR clears the list of recent Nop-out PDUs that almost
timed out.

 .TP
 \fB\-o\fR, \fB\-\-op=\fIop\fR
diff --git a/include/iscsi_if.h b/include/iscsi_if.h
index 7b96692..0b7dfe8 100644
--- a/include/iscsi_if.h
+++ b/include/iscsi_if.h
@@ -328,9 +328,10 @@ enum iscsi_param {
        ISCSI_PARAM_ISID,
        ISCSI_PARAM_INITIATOR_NAME,

+       ISCSI_PARAM_TGT_RESET_TMO,
+
        ISCSI_PARAM_NOOP_THRESHOLD,

-       ISCSI_PARAM_TGT_RESET_TMO,
        /* must always be last */
        ISCSI_PARAM_MAX,
 };
diff --git a/kernel/libiscsi.c b/kernel/libiscsi.c
index 5011139..3f10940 100644
--- a/kernel/libiscsi.c
+++ b/kernel/libiscsi.c
@@ -980,6 +980,15 @@ static void iscsi_send_nopout(struct iscsi_conn
*conn, struct iscsi_nopin *rhdr)
        }
 }

+static unsigned long iscsi_get_ping_delay(unsigned long last_ping)
+{
+       /* Check if there was a jiffies rollover */
+       if (jiffies < last_ping)
+               return ULONG_MAX - last_ping + jiffies;
+       else
+               return jiffies - last_ping;
+}
+
 static int iscsi_nop_out_rsp(struct iscsi_task *task,
                             struct iscsi_nopin *nop, char *data, int
datalen)
 {
@@ -998,8 +1007,7 @@ static int iscsi_nop_out_rsp(struct iscsi_task
*task,
                                   data, datalen))
                        rc = ISCSI_ERR_CONN_FAILED;
        } else {
-               ping_delay = jiffies - conn->last_ping;
-
+               ping_delay = iscsi_get_ping_delay(conn->last_ping);
                /* Check if the ping has almost timed out */
                if (ping_delay >=
                    (session->noop_threshold * (conn->ping_timeout *
HZ)) /
diff --git a/usr/config.h b/usr/config.h
index 90063fe..0ac6ff5 100644
--- a/usr/config.h
+++ b/usr/config.h
@@ -164,11 +164,11 @@ typedef enum discovery_type {
        DISCOVERY_TYPE_FW,
 } discovery_type_e;

-typedef enum noop_op_type {
-       NOOP_OP_NONE,
-       NOOP_OP_DUMP,
-       NOOP_OP_CLEAR,
-} noop_op_type_e;
+typedef enum debug_info_op_type {
+       DBG_INFO_OP_NONE,
+       DBG_INFO_OP_DUMP_NOOP,
+       DBG_INFO_OP_CLEAR_NOOP,
+} debug_info_op_type_e;

 typedef struct conn_rec {
        iscsi_startup_e                         startup;
diff --git a/usr/iscsiadm.c b/usr/iscsiadm.c
index bcbaf27..4670118 100644
--- a/usr/iscsiadm.c
+++ b/usr/iscsiadm.c
@@ -95,7 +95,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)
 {
@@ -179,17 +179,17 @@ str_to_type(char *str)
        return type;
 }

-static noop_op_type_e
-str_to_noop_op(char *str)
+static debug_info_op_type_e
+str_to_debug_info_op(char *str)
 {
-       noop_op_type_e op;
+       debug_info_op_type_e op;

-       if (!strcmp("dump", str))
-               op = NOOP_OP_DUMP;
-       else if (!strcmp("clear", str))
-               op = NOOP_OP_CLEAR;
+       if (!strcmp("dump_noop", str))
+               op = DBG_INFO_OP_DUMP_NOOP;
+       else if (!strcmp("clear_noop", str))
+               op = DBG_INFO_OP_CLEAR_NOOP;
        else
-               op = NOOP_OP_NONE;
+               op = DBG_INFO_OP_NONE;

        return op;
 }
@@ -1545,7 +1545,8 @@ update_fail:
 static int exec_node_op(int op, int do_login, int do_logout,
                        int do_show, int do_rescan, int do_stats,
                        int info_level, struct node_rec *rec,
-                       char *name, char *value, noop_op_type_e
noop_op)
+                       char *name, char *value,
+                       debug_info_op_type_e debug_info_op)
 {
        int rc = 0;
        struct db_set_param set_param;
@@ -1589,20 +1590,20 @@ static int exec_node_op(int op, int do_login,
int do_logout,
        if ((!do_login &&
             !do_logout &&
             op == OP_NOOP &&
-            noop_op == NOOP_OP_NONE) &&
+            debug_info_op == DBG_INFO_OP_NONE) &&
            (!strlen(rec->name) && !strlen(rec->conn[0].address) &&
             !strlen(rec->iface.name))) {
                rc = print_nodes(info_level, rec);
                goto out;
        }

-       switch (noop_op) {
-       case NOOP_OP_DUMP:
+       switch (debug_info_op) {
+       case DBG_INFO_OP_DUMP_NOOP:
                if (for_each_session(rec, session_noop_dump))
                        rc = -1;
                goto out;

-       case NOOP_OP_CLEAR:
+       case DBG_INFO_OP_CLEAR_NOOP:
                if (for_each_session(rec, session_noop_clear))
                        rc = -1;
                goto out;
@@ -1832,7 +1833,7 @@ main(int argc, char **argv)
        struct iface_rec *iface = NULL, *tmp;
        struct node_rec *rec = NULL;
        uint32_t host_no = -1;
-       noop_op_type_e noop_op = NOOP_OP_NONE;
+       debug_info_op_type_e debug_info_op = DBG_INFO_OP_NONE;

        memset(&drec, 0, sizeof(discovery_rec_t));
        INIT_LIST_HEAD(&ifaces);
@@ -1960,11 +1961,12 @@ main(int argc, char **argv)
                        break;
                case 'i':
                        /* Currently, we have only noop info here */
-                       noop_op = str_to_noop_op(optarg);
-                       if (noop_op == NOOP_OP_NONE) {
-                               log_error("Invalid noop operation '%s'. Noop "
-                                         "operation must be 'dump' or "
-                                         "'clear'.", optarg);
+                       debug_info_op = str_to_debug_info_op(optarg);
+                       if (debug_info_op == DBG_INFO_OP_NONE) {
+                               log_error("Invalid debug info operation '%s'. "
+                                         "Debug info operation must be "
+                                         "'dump_noop' or 'clear_noop'.",
+                                         optarg);
                                rc = -1;
                                goto free_ifaces;
                        }
@@ -2201,7 +2203,7 @@ main(int argc, char **argv)

                rc = exec_node_op(op, do_login, do_logout, do_show,
                                  do_rescan, do_stats, info_level, rec,
-                                 name, value, noop_op);
+                                 name, value, debug_info_op);
                break;
        case MODE_SESSION:
                if ((rc = verify_mode_params(argc, argv,
@@ -2260,17 +2262,17 @@ main(int argc, char **argv)
                        /* drop down to node ops */
                        rc = exec_node_op(op, do_login, do_logout, do_show,
                                          do_rescan, do_stats, info_level,
-                                         rec, name, value, noop_op);
+                                         rec, name, value, debug_info_op);
 free_info:
                        free(info);
                        goto out;
                } else {
                        if (do_logout || do_rescan || do_stats ||
-                           noop_op != NOOP_OP_NONE) {
+                           debug_info_op != DBG_INFO_OP_NONE) {
                                rc = exec_node_op(op, do_login, do_logout,
                                                 do_show, do_rescan, do_stats,
                                                 info_level, NULL, name, value,
-                                                 noop_op);
+                                                 debug_info_op);
                                goto out;
                        }

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