b=18844

i=isaac
Landing a patch fixing deadlock in usocklnd on HEAD. The patch also includes:
 - minor code cleanup suggested by Isaac in comment #6 of bug #18844
 - trivial cleanup patch for acceptor.c (att #21983 of bug #14132)
 - minor one-line fix for handling EINTR error of poll(2) - inspected by Shadow.

diff --git a/lnet/ChangeLog b/lnet/ChangeLog
index 36e3dbd..578ab40 100644 (file)
--- a/lnet/ChangeLog
+++ b/lnet/ChangeLog
@@ -17,6 +17,14 @@ Bugzilla   :
 Description: 
 Details    : 
 
 Description: 
 Details    : 
 

+Severity   : normal
+Bugzilla   : 18844
+Description: Fixing deadlock in usocklnd
+Details    : A deadlock was possible in usocklnd due to race condition while
+             tearing connection down. The problem resulted from erroneous
+             assumption that lnet_finalize() could have been called holding
+             some lnd-level locks.
+

 Severity   : major
 Bugzilla   : 13621, 15983
 Description: Protocol V2 of o2iblnd
 Severity   : major
 Bugzilla   : 13621, 15983
 Description: Protocol V2 of o2iblnd

diff --git a/lnet/lnet/acceptor.c b/lnet/lnet/acceptor.c
index c852f16..5b143a9 100644 (file)
--- a/lnet/lnet/acceptor.c
+++ b/lnet/lnet/acceptor.c
@@ -87,6 +87,18 @@ CFS_MODULE_PARM(accept_backlog, "i", int, 0444,
 CFS_MODULE_PARM(accept_timeout, "i", int, 0644,
                 "Acceptor's timeout (seconds)");
 
 CFS_MODULE_PARM(accept_timeout, "i", int, 0644,
                 "Acceptor's timeout (seconds)");
 

+static char *accept_type = NULL;
+
+int
+lnet_acceptor_get_tunables(void)
+{
+        /* Userland acceptor uses 'accept_type' instead of 'accept', due to
+         * conflict with 'accept(2)', but kernel acceptor still uses 'accept'
+         * for compatibility. Hence the trick. */
+        accept_type = accept;
+        return 0;
+}
+

 int
 lnet_acceptor_timeout(void)
 {
 int
 lnet_acceptor_timeout(void)
 {


@@ -221,7 +233,7 @@ EXPORT_SYMBOL(lnet_connect);
 
 #else /* below is multi-threaded user-space code */
 
 
 #else /* below is multi-threaded user-space code */
 

-static char *accept_type    = "secure";
+static char *accept_type = "secure";

 
 int
 lnet_acceptor_get_tunables()
 
 int
 lnet_acceptor_get_tunables()


@@ -418,11 +430,7 @@ lnet_acceptor(void *arg)
 
                 lnet_acceptor_state.pta_sock = NULL;
         } else {
 
                 lnet_acceptor_state.pta_sock = NULL;
         } else {

-#ifdef __KERNEL__
-                LCONSOLE(0, "Accept %s, port %d\n", accept, accept_port);
-#else

                 LCONSOLE(0, "Accept %s, port %d\n", accept_type, accept_port);
                 LCONSOLE(0, "Accept %s, port %d\n", accept_type, accept_port);

-#endif

         }
 
         /* set init status and unblock parent */
         }
 
         /* set init status and unblock parent */


@@ -517,23 +525,18 @@ lnet_acceptor_start(void)
 
         LASSERT (lnet_acceptor_state.pta_sock == NULL);
 
 
         LASSERT (lnet_acceptor_state.pta_sock == NULL);
 

-#ifndef __KERNEL__
-        /* kernel version uses CFS_MODULE_PARM */

         rc = lnet_acceptor_get_tunables();
         if (rc != 0)
                 return rc;
 
         rc = lnet_acceptor_get_tunables();
         if (rc != 0)
                 return rc;
 

+#ifndef __KERNEL__

         /* Do nothing if we're liblustre clients */
         if ((the_lnet.ln_pid & LNET_PID_USERFLAG) != 0)
                 return 0;
 #endif
         cfs_init_completion(&lnet_acceptor_state.pta_signal);
 
         /* Do nothing if we're liblustre clients */
         if ((the_lnet.ln_pid & LNET_PID_USERFLAG) != 0)
                 return 0;
 #endif
         cfs_init_completion(&lnet_acceptor_state.pta_signal);
 

-#ifdef __KERNEL__
-        rc = accept2secure(accept, &secure);
-#else

         rc = accept2secure(accept_type, &secure);
         rc = accept2secure(accept_type, &secure);

-#endif

         if (rc <= 0) {
                 cfs_fini_completion(&lnet_acceptor_state.pta_signal);
                 return rc;
         if (rc <= 0) {
                 cfs_fini_completion(&lnet_acceptor_state.pta_signal);
                 return rc;

diff --git a/lnet/ulnds/socklnd/conn.c b/lnet/ulnds/socklnd/conn.c
index 4d6e01d..48c9a1e 100644 (file)
--- a/lnet/ulnds/socklnd/conn.c
+++ b/lnet/ulnds/socklnd/conn.c
@@ -128,6 +128,8 @@ usocklnd_tear_peer_conn(usock_conn_t *conn)
         lnet_process_id_t id;
         int               decref_flag  = 0;
         int               killall_flag = 0;
         lnet_process_id_t id;
         int               decref_flag  = 0;
         int               killall_flag = 0;

+        void             *rx_lnetmsg   = NULL; 
+        CFS_LIST_HEAD    (zombie_txs);

 
         if (peer == NULL) /* nothing to tear */
                 return;
 
         if (peer == NULL) /* nothing to tear */
                 return;


@@ -142,11 +144,12 @@ usocklnd_tear_peer_conn(usock_conn_t *conn)
                 if (conn->uc_rx_state == UC_RX_LNET_PAYLOAD) {
                         /* change state not to finalize twice */
                         conn->uc_rx_state = UC_RX_KSM_HEADER;
                 if (conn->uc_rx_state == UC_RX_LNET_PAYLOAD) {
                         /* change state not to finalize twice */
                         conn->uc_rx_state = UC_RX_KSM_HEADER;

-                        lnet_finalize(peer->up_ni, conn->uc_rx_lnetmsg, -EIO);
+                        /* stash lnetmsg while holding locks */
+                        rx_lnetmsg = conn->uc_rx_lnetmsg;

                 }
 
                 }
 

-                usocklnd_destroy_txlist(peer->up_ni,
-                                        &conn->uc_tx_list);
+                /* we cannot finilize txs right now (bug #18844) */
+                list_splice_init(&conn->uc_tx_list, &zombie_txs);

 
                 peer->up_conns[idx] = NULL;
                 conn->uc_peer = NULL;
 
                 peer->up_conns[idx] = NULL;
                 conn->uc_peer = NULL;


@@ -154,6 +157,9 @@ usocklnd_tear_peer_conn(usock_conn_t *conn)
 
                 if(conn->uc_errored && !peer->up_errored)
                         peer->up_errored = killall_flag = 1;
 
                 if(conn->uc_errored && !peer->up_errored)
                         peer->up_errored = killall_flag = 1;

+
+                /* prevent queueing new txs to this conn */
+                conn->uc_errored = 1;

         }
 
         pthread_mutex_unlock(&conn->uc_lock);
         }
 
         pthread_mutex_unlock(&conn->uc_lock);


@@ -166,6 +172,11 @@ usocklnd_tear_peer_conn(usock_conn_t *conn)
         if (!decref_flag)
                 return;
 
         if (!decref_flag)
                 return;
 

+        if (rx_lnetmsg != NULL)
+                lnet_finalize(ni, rx_lnetmsg, -EIO);
+        
+        usocklnd_destroy_txlist(ni, &zombie_txs);
+

         usocklnd_conn_decref(conn);
         usocklnd_peer_decref(peer);
 
         usocklnd_conn_decref(conn);
         usocklnd_peer_decref(peer);
 


@@ -850,6 +861,13 @@ usocklnd_find_or_create_conn(usock_peer_t *peer, int type,
 
         LASSERT(tx == NULL || zc_ack == NULL);
         if (tx != NULL) {
 
         LASSERT(tx == NULL || zc_ack == NULL);
         if (tx != NULL) {

+                /* usocklnd_tear_peer_conn() could signal us stop queueing */
+                if (conn->uc_errored) {
+                        rc = -EIO;
+                        pthread_mutex_unlock(&conn->uc_lock);
+                        goto find_or_create_conn_failed;
+                }
+

                 usocklnd_enqueue_tx(conn, tx, send_immediately);
         } else {
                 rc = usocklnd_enqueue_zcack(conn, zc_ack);
                 usocklnd_enqueue_tx(conn, tx, send_immediately);
         } else {
                 rc = usocklnd_enqueue_zcack(conn, zc_ack);

diff --git a/lnet/ulnds/socklnd/handlers.c b/lnet/ulnds/socklnd/handlers.c
index e9eda46..521e5b2 100644 (file)
--- a/lnet/ulnds/socklnd/handlers.c
+++ b/lnet/ulnds/socklnd/handlers.c
@@ -1000,7 +1000,8 @@ usocklnd_read_data(usock_conn_t *conn)
                 nob = libcfs_sock_readv(conn->uc_sock,
                                         conn->uc_rx_iov, conn->uc_rx_niov);
                 if (nob <= 0) {/* read nothing or error */
                 nob = libcfs_sock_readv(conn->uc_sock,
                                         conn->uc_rx_iov, conn->uc_rx_niov);
                 if (nob <= 0) {/* read nothing or error */

-                        conn->uc_errored = 1;
+                        if (nob < 0)
+                                conn->uc_errored = 1;

                         return nob;
                 }
 
                         return nob;
                 }
 

diff --git a/lnet/ulnds/socklnd/poll.c b/lnet/ulnds/socklnd/poll.c
index 2f15f1b..01ea259 100644 (file)
--- a/lnet/ulnds/socklnd/poll.c
+++ b/lnet/ulnds/socklnd/poll.c
@@ -113,7 +113,7 @@ usocklnd_poll_thread(void *arg)
                           pt_data->upt_nfds,
                           usock_tuns.ut_poll_timeout * 1000);
 
                           pt_data->upt_nfds,
                           usock_tuns.ut_poll_timeout * 1000);
 

-                if (rc < 0) {
+                if (rc < 0 && errno != EINTR) {

                         CERROR("Cannot poll(2): errno=%d\n", errno);
                         break;
                 }
                         CERROR("Cannot poll(2): errno=%d\n", errno);
                         break;
                 }