msg/simple: do not stop_and_wait on mark_down

We originally blocked in mark_down for fast dispatch threads
to complete to avoid various races in the code.  Most of these
were in the OSD itself, where we were not prepared to get
messges on connections that had no attached session.  Since
then, the OSD checks have been cleaned up to handle this.
There were other races we were worried about too, but the
details have been lost in the depths of time.

Instead, take the other route: make mark_down never block on
dispatch.  This lets us remove the special case that
was added in order to cope with fast dispatch calling
mark_down on itself.

Now, the only stop_and_wait() user is the shutdown sequence.

Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/src/msg/simple/Pipe.cc b/src/msg/simple/Pipe.cc
index 4f386051811ad1cd82e2e7d7e88dc4eb8a61b118..ee71446ca4444c2326e4d2de9b2d621d8e13826e 100644 (file)
--- a/src/msg/simple/Pipe.cc
+++ b/src/msg/simple/Pipe.cc
@@ -251,8 +251,7 @@ void *Pipe::DelayedDelivery::entry()
 void Pipe::DelayedDelivery::stop_fast_dispatching() {
   Mutex::Locker l(delay_lock);
   stop_fast_dispatching_flag = true;
-  // we can't block if we're the delay thread; see Pipe::stop_and_wait()
-  while (delay_dispatching && !am_self())
+  while (delay_dispatching)
     delay_cond.Wait(delay_lock);
 }
 
@@ -1431,18 +1430,11 @@ void Pipe::stop_and_wait()
   if (state != STATE_CLOSED)
     stop();
   
-  // HACK: we work around an annoying deadlock here.  If the fast
-  // dispatch method calls mark_down() on itself, it can block here
-  // waiting for the reader_dispatching flag to clear... which will
-  // clearly never happen.  Avoid the situation by skipping the wait
-  // if we are marking our *own* connect down. Do the same for the
-  // delayed dispatch thread.
   if (delay_thread) {
     delay_thread->stop_fast_dispatching();
   }
   while (reader_running &&
-        reader_dispatching &&
-        !reader_thread.am_self())
+        reader_dispatching)
     cond.Wait(pipe_lock);
 }
 

diff --git a/src/msg/simple/SimpleMessenger.cc b/src/msg/simple/SimpleMessenger.cc
index cf6bc73711bad753e1167945ba077ced0ce08536..edc8f0fe598a78ddaa1deb0c722edb6aa301293f 100644 (file)
--- a/src/msg/simple/SimpleMessenger.cc
+++ b/src/msg/simple/SimpleMessenger.cc
@@ -581,7 +581,7 @@ void SimpleMessenger::mark_down_all()
     Pipe *p = *q;
     ldout(cct,5) << "mark_down_all accepting_pipe " << p << dendl;
     p->pipe_lock.Lock();
-    p->stop_and_wait();
+    p->stop();
     PipeConnectionRef con = p->connection_state;
     if (con && con->clear_pipe(p))
       dispatch_queue.queue_reset(con.get());
@@ -596,7 +596,7 @@ void SimpleMessenger::mark_down_all()
     rank_pipe.erase(it);
     p->unregister_pipe();
     p->pipe_lock.Lock();
-    p->stop_and_wait();
+    p->stop();
     PipeConnectionRef con = p->connection_state;
     if (con && con->clear_pipe(p))
       dispatch_queue.queue_reset(con.get());
@@ -613,7 +613,7 @@ void SimpleMessenger::mark_down(const entity_addr_t& addr)
     ldout(cct,1) << "mark_down " << addr << " -- " << p << dendl;
     p->unregister_pipe();
     p->pipe_lock.Lock();
-    p->stop_and_wait();
+    p->stop();
     if (p->connection_state) {
       // generate a reset event for the caller in this case, even
       // though they asked for it, since this is the addr-based (and
@@ -640,7 +640,7 @@ void SimpleMessenger::mark_down(Connection *con)
     assert(p->msgr == this);
     p->unregister_pipe();
     p->pipe_lock.Lock();
-    p->stop_and_wait();
+    p->stop();
     if (p->connection_state) {
       // do not generate a reset event for the caller in this case,
       // since they asked for it.