Rx: make conn_call_lock and conn_data_lock usage consistent
authorJeffrey Altman <jaltman@your-file-system.com>
Sat, 17 Apr 2010 02:42:34 +0000 (22:42 -0400)
committerDerrick Brashear <shadow@dementia.org>
Sat, 17 Apr 2010 14:09:29 +0000 (07:09 -0700)
The rx_connection.flags field is protected by the conn_data_lock
but the conn_data_lock is not held everywhere the conn flags
field is altered.  This produces a race that can result in a
deadlock when waiter flags are inadvertently prevented from being
cleared.

The conn_call_lock usage in rx_EndCall which was removed in
Change e169708681eb1bbbb31951b95f68e861a4b01c7e must be restored.
If rx_EndCall never obtains the conn_call_lock it can't ensure
that the thread in rx_NewCall actively checking the calls will
not end up blocking when there is now a call channel that can
be reused.  This usage of conn_call_lock can be removed only
if a true producer/consumer model is implemented.

Change-Id: Id093fd6c4a42afb833c775411be0cd406abf4ef7
Reviewed-on: http://gerrit.openafs.org/1766
Reviewed-by: Jeffrey Altman <jaltman@openafs.org>
Tested-by: Jeffrey Altman <jaltman@openafs.org>
Reviewed-by: Derrick Brashear <shadow@dementia.org>
Tested-by: Derrick Brashear <shadow@dementia.org>

src/rx/rx.c
src/rx/rx.h

index 9ff827f..14d685c 100644 (file)
@@ -2076,6 +2076,9 @@ rx_EndCall(struct rx_call *call, afs_int32 rc)
         * have checked this call, found it active and by the time it
         * goes to sleep, will have missed the signal.
         */
+        MUTEX_EXIT(&call->lock);
+        MUTEX_ENTER(&conn->conn_call_lock);
+        MUTEX_ENTER(&call->lock);
        MUTEX_ENTER(&conn->conn_data_lock);
        conn->flags |= RX_CONN_BUSY;
        if (conn->flags & RX_CONN_MAKECALL_WAITING) {
@@ -2116,7 +2119,10 @@ rx_EndCall(struct rx_call *call, afs_int32 rc)
     CALL_RELE(call, RX_CALL_REFCOUNT_BEGIN);
     MUTEX_EXIT(&call->lock);
     if (conn->type == RX_CLIENT_CONNECTION) {
+       MUTEX_ENTER(&conn->conn_data_lock);
        conn->flags &= ~RX_CONN_BUSY;
+       MUTEX_EXIT(&conn->conn_data_lock);
+        MUTEX_EXIT(&conn->conn_call_lock);
     }
     USERPRI;
     /*
@@ -2385,8 +2391,8 @@ rxi_FreeCall(struct rx_call *call)
      * If someone else destroys a connection, they either have no
      * call lock held or are going through this section of code.
      */
+    MUTEX_ENTER(&conn->conn_data_lock);
     if (conn->flags & RX_CONN_DESTROY_ME && !(conn->flags & RX_CONN_MAKECALL_WAITING)) {
-       MUTEX_ENTER(&conn->conn_data_lock);
        conn->refCount++;
        MUTEX_EXIT(&conn->conn_data_lock);
 #ifdef RX_ENABLE_LOCKS
@@ -2397,6 +2403,8 @@ rxi_FreeCall(struct rx_call *call)
 #else /* RX_ENABLE_LOCKS */
        rxi_DestroyConnection(conn);
 #endif /* RX_ENABLE_LOCKS */
+    } else {
+       MUTEX_EXIT(&conn->conn_data_lock);
     }
 }
 
@@ -3153,6 +3161,7 @@ rxi_IsConnInteresting(struct rx_connection *aconn)
 
     if (aconn->flags & (RX_CONN_MAKECALL_WAITING | RX_CONN_DESTROY_ME))
        return 1;
+
     for (i = 0; i < RX_MAXCALLS; i++) {
        tcall = aconn->call[i];
        if (tcall) {
index 343a18e..b51822a 100644 (file)
@@ -254,7 +254,7 @@ struct rx_connection {
     struct rx_service *service;        /* used by servers only */
     u_short serviceId;         /* To stamp on requests (clients only) */
     afs_uint32 refCount;               /* Reference count */
-    u_char flags;              /* Defined below */
+    u_char flags;              /* Defined below - (conn_data_lock) */
     u_char type;               /* Type of connection, defined below */
     u_char secondsUntilPing;   /* how often to ping for each active call */
     u_char securityIndex;      /* corresponds to the security class of the */