On 02/12/2015 06:33 PM, Chris Leech wrote: > It looks like the communication with iscsiuio has a similar case where > a polling function reschedules itself, following up with a patch to fix > the delay there.
Have you been able to hit that iscsiuio reschedule code path? There was a bug where we could call actor_timer multiple times on undeleted timer. I made the attached patch (made over github tree). The patch is a mashup of 3 patches, but basically that uio poll code could reschedule itself and the connect related code could. The connect code path is the one that could do the double actor_timer. While fixing up the connect code, I noticed that we could just kill the uio poll code and retry like how we do for that module loading race case. If you can hit that uio poll case, is the test setup in beaker by any chance? -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
>From 858767df0e63b2f08ec829e0bfdf201b75a60968 Mon Sep 17 00:00:00 2001 From: Mike Christie <[email protected]> Date: Thu, 5 Feb 2015 02:11:16 -0600 Subject: [PATCH] iscsid: make sure actor is delated before rescheduling iscsi_conn_connect() be called from a login_timer that is not deleted. This causes it to be scheduled multiple times. This patch adds a new function actor_timer_mod to handle both deletion and rescheduling. This patch also then removed the uio poll code which was using the login_timer to schedule itself. This uio retry, is now just done in the generic initial login retry. --- usr/actor.c | 8 ++++ usr/actor.h | 3 +- usr/initiator.c | 122 +++++-------------------------------------------- usr/initiator.h | 1 - usr/initiator_common.c | 8 ++-- 5 files changed, 26 insertions(+), 116 deletions(-) diff --git a/usr/actor.c b/usr/actor.c index 37b5024..21cd819 100644 --- a/usr/actor.c +++ b/usr/actor.c @@ -182,6 +182,14 @@ actor_timer(actor_t *thread, uint32_t timeout_secs, void (*callback)(void *), actor_schedule_private(thread, timeout_secs, 0); } +void +actor_timer_mod(actor_t *thread, uint32_t new_timeout_secs, void *data) +{ + actor_delete(thread); + thread->data = data; + actor_schedule_private(thread, new_timeout_secs, 0); +} + /* * Execute all items that have expired. * diff --git a/usr/actor.h b/usr/actor.h index 7283dce..f572f2e 100644 --- a/usr/actor.h +++ b/usr/actor.h @@ -43,7 +43,8 @@ extern void actor_schedule_head(actor_t *thread); extern void actor_schedule(actor_t *thread); extern void actor_timer(actor_t *thread, uint32_t delay_secs, void (*callback)(void *), void *data); -extern int actor_timer_mod(actor_t *thread, uint32_t new_delay_secs, void *data); +extern void actor_timer_mod(actor_t *thread, uint32_t new_delay_secs, + void *data); extern void actor_poll(void); #endif /* ACTOR_H */ diff --git a/usr/initiator.c b/usr/initiator.c index 1aadc9b..f70db4d 100644 --- a/usr/initiator.c +++ b/usr/initiator.c @@ -262,6 +262,7 @@ __session_conn_create(iscsi_session_t *session, int cid) conn->state = ISCSI_CONN_STATE_FREE; conn->session = session; + actor_init(&conn->login_timer, iscsi_login_timedout, NULL); /* * TODO: we must export the socket_fd/transport_eph from sysfs * so if iscsid is resyncing up we can pick that up and cleanup up @@ -528,9 +529,7 @@ queue_delayed_reopen(queue_task_t *qtask, int delay) * iscsi_login_eh can handle the login resched as * if it were login time out */ - actor_delete(&conn->login_timer); - actor_timer(&conn->login_timer, delay, - iscsi_login_timedout, qtask); + actor_timer_mod(&conn->login_timer, delay, qtask); } static int iscsi_conn_connect(struct iscsi_conn *conn, queue_task_t *qtask) @@ -565,53 +564,10 @@ static int iscsi_conn_connect(struct iscsi_conn *conn, queue_task_t *qtask) iscsi_sched_ev_context(ev_context, conn, 0, EV_CONN_POLL); log_debug(3, "Setting login timer %p timeout %d", &conn->login_timer, conn->login_timeout); - actor_timer(&conn->login_timer, conn->login_timeout, - iscsi_login_timedout, qtask); + actor_timer_mod(&conn->login_timer, conn->login_timeout, qtask); return 0; } -static void iscsi_uio_poll_login_timedout(void *data) -{ - struct queue_task *qtask = data; - struct iscsi_conn *conn = qtask->conn; - iscsi_session_t *session = conn->session; - - log_debug(3, "timeout waiting for UIO ...\n"); - mgmt_ipc_write_rsp(qtask, ISCSI_ERR_TRANS_TIMEOUT); - conn_delete_timers(conn); - __session_destroy(session); -} - -static int iscsi_sched_uio_poll(queue_task_t *qtask) -{ - struct iscsi_conn *conn = qtask->conn; - struct iscsi_session *session = conn->session; - struct iscsi_transport *t = session->t; - struct iscsi_ev_context *ev_context; - - if (!t->template->set_net_config) - return 0; - - ev_context = iscsi_ev_context_get(conn, 0); - if (!ev_context) { - /* while reopening the recv pool should be full */ - log_error("BUG: __session_conn_reopen could " - "not get conn context for recv."); - return -ENOMEM; - } - - ev_context->data = qtask; - conn->state = ISCSI_CONN_STATE_XPT_WAIT; - - iscsi_sched_ev_context(ev_context, conn, 0, EV_UIO_POLL); - - log_debug(3, "Setting login UIO poll timer %p timeout %d", - &conn->login_timer, conn->login_timeout); - actor_timer(&conn->login_timer, conn->login_timeout, - iscsi_uio_poll_login_timedout, qtask); - return -EAGAIN; -} - static void __session_conn_reopen(iscsi_conn_t *conn, queue_task_t *qtask, int do_stop, int redirected) @@ -1740,53 +1696,6 @@ failed_login: } -static void session_conn_uio_poll(void *data) -{ - struct iscsi_ev_context *ev_context = data; - iscsi_conn_t *conn = ev_context->conn; - struct iscsi_session *session = conn->session; - queue_task_t *qtask = ev_context->data; - int rc; - - log_debug(4, "retrying uio poll"); - rc = iscsi_set_net_config(session->t, session, - &conn->session->nrec.iface); - if (rc != 0) { - if (rc == ISCSI_ERR_AGAIN) { - ev_context->data = qtask; - iscsi_sched_ev_context(ev_context, conn, 2, - EV_UIO_POLL); - return; - } else { - log_error("session_conn_uio_poll() " - "connection failure [0x%x]", rc); - actor_delete(&conn->login_timer); - iscsi_login_eh(conn, qtask, ISCSI_ERR_INTERNAL); - iscsi_ev_context_put(ev_context); - return; - } - } - - iscsi_ev_context_put(ev_context); - actor_delete(&conn->login_timer); - log_debug(4, "UIO ready trying connect"); - - /* uIP is ready try to connect */ - if (gettimeofday(&conn->initial_connect_time, NULL)) - log_error("Could not get initial connect time. If " - "login errors iscsid may give up the initial " - "login early. You should manually login."); - - conn->state = ISCSI_CONN_STATE_XPT_WAIT; - if (iscsi_conn_connect(conn, qtask)) { - int delay = ISCSI_CONN_ERR_REOPEN_DELAY; - - log_debug(4, "Waiting %u seconds before trying to reconnect.\n", - delay); - queue_delayed_reopen(qtask, delay); - } -} - static int iscsi_sched_ev_context(struct iscsi_ev_context *ev_context, struct iscsi_conn *conn, unsigned long tmo, int event) @@ -1825,12 +1734,7 @@ static int iscsi_sched_ev_context(struct iscsi_ev_context *ev_context, break; case EV_CONN_POLL: actor_init(&ev_context->actor, session_conn_poll, - ev_context); - actor_schedule(&ev_context->actor); - break; - case EV_UIO_POLL: - actor_init(&ev_context->actor, session_conn_uio_poll, - ev_context); + ev_context); actor_schedule(&ev_context->actor); break; case EV_CONN_LOGOUT_TIMER: @@ -1970,14 +1874,12 @@ static int __session_login_task(node_rec_t *rec, queue_task_t *qtask) rc = iscsi_host_set_net_params(&rec->iface, session); if (rc == ISCSI_ERR_AGAIN) { - iscsi_sched_uio_poll(qtask); /* - * Cannot block iscsid, so caller is going to internally - * retry the operation. + * host/iscsiuio not ready. Cannot block iscsid, so caller is + * going to internally retry the operation. */ - qtask->rsp.command = MGMT_IPC_SESSION_LOGIN; - qtask->rsp.err = ISCSI_SUCCESS; - return ISCSI_SUCCESS; + __session_destroy(session); + return ISCSI_ERR_HOST_NOT_FOUND; } else if (rc) { __session_destroy(session); return ISCSI_ERR_LOGIN; @@ -2024,17 +1926,17 @@ session_login_task(node_rec_t *rec, queue_task_t *qtask) static void session_login_task_retry(void *data) { struct login_task_retry_info *info = data; + struct node_rec *rec = info->rec; int rc; - rc = __session_login_task(info->rec, info->qtask); + rc = __session_login_task(rec, info->qtask); if (rc == ISCSI_ERR_HOST_NOT_FOUND) { - if (info->retry_count == 5) { + if (info->retry_count == rec->conn[0].timeo.login_timeout) { /* give up */ goto write_rsp; } - rc = queue_session_login_task_retry(info, info->rec, - info->qtask); + rc = queue_session_login_task_retry(info, rec, info->qtask); if (rc) goto write_rsp; /* we are going to internally retry */ diff --git a/usr/initiator.h b/usr/initiator.h index c34625b..c11d77f 100644 --- a/usr/initiator.h +++ b/usr/initiator.h @@ -83,7 +83,6 @@ typedef enum iscsi_event_e { EV_CONN_LOGOUT_TIMER, EV_CONN_STOP, EV_CONN_LOGIN, - EV_UIO_POLL, } iscsi_event_e; struct queue_task; diff --git a/usr/initiator_common.c b/usr/initiator_common.c index eb03b23..98ca636 100644 --- a/usr/initiator_common.c +++ b/usr/initiator_common.c @@ -249,7 +249,7 @@ int iscsi_setup_portal(struct iscsi_conn *conn, char *address, int port) return 0; } -int host_set_param(struct iscsi_transport *t, +static int host_set_param(struct iscsi_transport *t, uint32_t host_no, int param, char *value, int type) { @@ -261,7 +261,7 @@ int host_set_param(struct iscsi_transport *t, log_error("can't set operational parameter %d for " "host %d, retcode %d (%d)", param, host_no, rc, errno); - return rc; + return ISCSI_ERR_INVAL; } return 0; } @@ -677,13 +677,13 @@ int iscsi_host_set_net_params(struct iface_rec *iface, log_warning("Please set the iface.ipaddress for iface " "%s, then retry the login command.\n", iface->name); - return EINVAL; + return ISCSI_ERR_INVAL; } else if (t->template->set_host_ip == SET_HOST_IP_OPT) { log_info("Optional iface.ipaddress for iface %s " "not set.\n", iface->name); return 0; } else { - return EINVAL; + return ISCSI_ERR_INVAL; } } -- 1.8.3.1
