osd: fix backoff vs reset race

In OSD::ms_handle_reset, we clear session->con before removing any
backoffs.  That means we have to check if con has been cleared after any
call to have_backoff, lest we race with ms_handle_reset and it removes the
backoffs but we don't realize our client session is disconnected.

Introduce a helper to do both these checks in a safe way, simplifying
callers while we're at it.

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

diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc
index 77f8630f213be3ab5653e1c05a7b3ca8bc04411e..33b8b759824afb3d9941196597048880bbf87443 100644 (file)
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -4946,7 +4946,7 @@ bool OSD::ms_handle_reset(Connection *con)
   session->con.reset(NULL);  // break con <-> session ref cycle
   // note that we break session->con *before* the session_handle_reset
   // cleanup below.  this avoids a race between us and
-  // PG::add_backoff, split_backoff, etc.
+  // PG::add_backoff, Session::check_backoff, etc.
   session_handle_reset(session);
   session->put();
   return true;

diff --git a/src/osd/PrimaryLogPG.cc b/src/osd/PrimaryLogPG.cc
index 5ca2a8eced24f1e88bec0b8a52cab60f030a70f4..64711f2091e38f33ec1e2084794d6d01efb9c84a 100644 (file)
--- a/src/osd/PrimaryLogPG.cc
+++ b/src/osd/PrimaryLogPG.cc
@@ -1621,11 +1621,8 @@ void PrimaryLogPG::do_request(
     session->put();  // get_priv takes a ref, and so does the SessionRef
 
     if (op->get_req()->get_type() == CEPH_MSG_OSD_OP) {
-      BackoffRef b(session->have_backoff(info.pgid,
-                                        info.pgid.pgid.get_hobj_start()));
-      if (b) {
-       dout(10) << " have backoff " << *b << " " << *m << dendl;
-       assert(!b->is_acked() || !g_conf->osd_debug_crash_on_ignored_backoff);
+      if (session->check_backoff(cct, info.pgid,
+                                info.pgid.pgid.get_hobj_start(), m)) {
        return;
       }
 
@@ -1784,10 +1781,7 @@ void PrimaryLogPG::do_op(OpRequestRef& op)
     }
     session->put();  // get_priv() takes a ref, and so does the intrusive_ptr
 
-    BackoffRef b(session->have_backoff(info.pgid, head));
-    if (b) {
-      dout(10) << __func__ << " have backoff " << *b << " " << *m << dendl;
-      assert(!b->is_acked() || !g_conf->osd_debug_crash_on_ignored_backoff);
+    if (session->check_backoff(cct, info.pgid, head, m)) {
       return;
     }
   }

diff --git a/src/osd/Session.cc b/src/osd/Session.cc
index abe1a0c9ebaeff6abb92e817c9159a6d669090df..2de4427134480f17f35f1b0a80124468ce84ab12 100644 (file)
--- a/src/osd/Session.cc
+++ b/src/osd/Session.cc
@@ -81,3 +81,23 @@ void Session::ack_backoff(
   }
   assert(!backoff_count == backoffs.empty());
 }
+
+bool Session::check_backoff(
+  CephContext *cct, spg_t pgid, const hobject_t& oid, Message *m)
+{
+  BackoffRef b(have_backoff(pgid, oid));
+  if (b) {
+    dout(10) << __func__ << " session " << this << " has backoff " << *b
+            << " for " << *m << dendl;
+    assert(!b->is_acked() || !g_conf->osd_debug_crash_on_ignored_backoff);
+    return true;
+  }
+  // we may race with ms_handle_reset.  it clears session->con before removing
+  // backoffs, so if we see con is cleared here we have to abort this
+  // request.
+  if (!con) {
+    dout(10) << __func__ << " session " << this << " disconnected" << dendl;
+    return true;
+  }
+  return false;
+}

diff --git a/src/osd/Session.h b/src/osd/Session.h
index 91648e60b73c10a13915e5dc619a00aa143902aa..257bc34a15505853e21696e494f7298f559ec04d 100644 (file)
--- a/src/osd/Session.h
+++ b/src/osd/Session.h
@@ -194,6 +194,9 @@ struct Session : public RefCountedObject {
     return nullptr;
   }
 
+  bool check_backoff(
+    CephContext *cct, spg_t pgid, const hobject_t& oid, Message *m);
+
   void add_backoff(BackoffRef b) {
     Mutex::Locker l(backoff_lock);
     assert(!backoff_count == backoffs.empty());