CLD: fix hang in ncld_sess_open if session open fails

PATCH Applied Pete Zaitcev 2 participants 2 messages Apr 14, 2010 Library code

lore

Share & tools

lore

Pete Zaitcev

author

2010-04-14 00:22 UTC #0 lore raw

PATCH 1/1 CLD: fix hang in ncld_sess_open if session open fails

The problem turned out to be two-fold, with the same symptom.

Firstly, we use is_open to signal that the caller thread may proceed, but this is incorrect in case the thread open fails:
we still want the caller thread to proceed and deliver the error indicator from ncld_sess_open to the application. So, let's split is_up from the condition variable mechanism. It continues to mean that the session is open and up, and open_done is introduced for the waiting mechanism.

In addition, we forgot to take a mutex around a call into the cldc layer. It manifested itself in timers not firing, and so we would hang waiting for an answer from CLD server.

Signed-off-by Pete Zaitcev <[email protected]>

---
 include/ncld.h |    1 +
 lib/cldc.c     |   15 +++++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

This, I think, can go either before or after the e2e verbose. My previous patch added a HAIL_VERBOSE in a strategic location, but it's not really necessary and I took it out.

include/ncld.h +1 -0

diff -urp -X dontdiff cld-m/include/ncld.h cld-tip/include/ncld.h
@@ -36,6 +36,7 @@ struct ncld_sess {
 	GCond			*cond;
 	GThread			*thread;
 	bool			is_up;
+	bool			open_done;
 	int			errc;
 	GList			*handles;
 	int			to_thread[2];

lib/cldc.c +13 -2

diff -urp -X dontdiff cld-m/lib/cldc.c cld-tip/lib/cldc.c
@@ -1491,6 +1496,8 @@ static void ncld_p_event(void *priv, str
 	if (what == CE_SESS_FAILED) {
 		if (nsess->udp->sess != csp)
 			abort();
+		if (!nsess->is_up)
+			return;
 		nsess->is_up = false;
 		/* XXX wake up all I/O waiters here */
 		/*

@@ -1519,9 +1526,15 @@ static int ncld_new_sess(struct cldc_cal
 	/*
 	 * All callbacks from cldc layer run on the context of the thread
 	 * with nsess->mutex locked, so we don't lock it again here.
+	 *
+	 * We set is_up on the context that is invoked from cldc,
+	 * so the flag has a sane meaning in case we immediately
+	 * get a session failure event.
 	 */
 	nsess->errc = errc;
-	nsess->is_up = true;
+	nsess->open_done = true;
+	if (!errc)
+		nsess->is_up = true;
 	g_cond_broadcast(nsess->cond);
 	return 0;
 }

@@ -1529,7 +1542,7 @@ static int ncld_new_sess(struct cldc_cal
 static int ncld_wait_session(struct ncld_sess *nsess)
 {
 	g_mutex_lock(nsess->mutex);
-	while (!nsess->is_up)
+	while (!nsess->open_done)
 		g_cond_wait(nsess->cond, nsess->mutex);
 	g_mutex_unlock(nsess->mutex);
 	return nsess->errc;

@@ -1620,6 +1633,7 @@ struct ncld_sess *ncld_sess_open(const c
 		goto out_thread;
 	}
 
+	g_mutex_lock(nsess->mutex);
 	memset(&copts, 0, sizeof(copts));
 	copts.cb = ncld_new_sess;
 	copts.private = nsess;

@@ -1627,9 +1641,11 @@ struct ncld_sess *ncld_sess_open(const c
 			      nsess->udp->addr, nsess->udp->addr_len,
 			      cld_user, cld_key, nsess, log,
 			      &nsess->udp->sess)) {
+		g_mutex_unlock(nsess->mutex);
 		err = 1024;
 		goto out_session;
 	}
+	g_mutex_unlock(nsess->mutex);
 
 	rc = ncld_wait_session(nsess);
 	if (rc) {

Jeff Garzik

Applied

2010-04-14 04:01 UTC #1 lore raw

Quoted 16 lines · Pete Zaitcev

The problem turned out to be two-fold, with the same symptom.
Firstly, we use is_open to signal that the caller thread may proceed, but this is incorrect in case the thread open fails:
we still want the caller thread to proceed and deliver the error indicator from ncld_sess_open to the application. So, let's split is_up from the condition variable mechanism. It continues to mean that the session is open and up, and open_done is introduced for the waiting mechanism.
In addition, we forgot to take a mutex around a call into the cldc layer. It manifested itself in timers not firing, and so we would hang waiting for an answer from CLD server.
Signed-off-by Pete Zaitcev<[email protected]>

Quoted 8 lines

---
  include/ncld.h |    1 +
  lib/cldc.c     |   15 +++++++++++++--
  2 files changed, 14 insertions(+), 2 deletions(-)
This, I think, can go either before or after the e2e verbose. My previous patch added a HAIL_VERBOSE in a strategic location, but it's not really necessary and I took it out.

applied