Ok with me.
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
Thanks for doing this and sorry for the late reply.
> * 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
not used
> * 2.6.16-suse.patch
I think Chelsio fixed up the 2.6.14-19_compat.patch patch to work with
the older SLES, so I do not think this is needed. Do not worry about it
though. It is broken.
> * 2.6.20-21_compat.patch
not needed.
>
> Are they still maintained? If not, can we throw (at least some of) them away?
Yeah. Will clean up the repository.
Testing out patch now.
Done.
>
>
> +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?
>
>
>
> + unsigned int elapsed_time_msec;
> + short init;
> +};
>
>
> ping_delay = jiffies - conn->last_ping;
> +
> + /* Check if the ping has almost timed out */
> + if (ping_delay >=
> + (session->noop_threshold * (conn->ping_timeout * HZ)) /
>
> Does this handle jiffies rollover ok? If jiffies rolled over and it is 1 but
> last_ping was ULONG_MAX, then ping_delay is going to be very large.
Fixed.
>
>
>
> +static noop_op_type_e
> +str_to_noop_op(char *str)
> +{
> + noop_op_type_e op;
> +
> + if (!strcmp("dump", str))
> + op = NOOP_OP_DUMP;
> + else if (!strcmp("clear", str))
> + op = NOOP_OP_CLEAR;
> + else
> + op = NOOP_OP_NONE;
> +
> + return op;
> +}
>
> Should we make these names clear that it resets/dumps the noop data. If we
> added a scsi cmd timer option, then we probably want to be able to
> dump/reset one or the other. So rename dump to dump_noop and clear to
> clear_noop.
Fixed. We also need to replace str_to_noop_op with
str_to_debug_info_op (and change the enum accordingly).
>
>
>
> case MODE_NODE:
> if ((rc = verify_mode_params(argc, argv, "RsPIdmlSonvupTUL",
>
> This line needs a 'i'. It looks like you added the node compat support but
> you need to enable it with the 'i'. I added it when I tested your patch and
> it worked fine, so just added it when you send the patch.
>
>
OK
> Also what about debugfs? Did you think you would want the flexibility? Is
> that too many interfaces (sysfs, netlink, bsg and debugfs)?
I'm not very familiar with debugfs. What do you want to do with it?
Erez
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'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.
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;
}