osd: fix watch_lock vs map_lock ordering

watch_lock is inside map_lock (and pg->lock), which means we need to
drop it to take pg->lock here.  That means verifying in
handle_watch_timeout that we haven't raced with another thread canceling
the timeout event, which would be indicated by

 - the entity not appearing in unconnected_watchers
 - the entity having a different (presumably newer) expire time

Fixes: #2103
Signed-off-by: Sage Weil <sage.weil@dreamhost.com>
Reviewed-by: Samuel Just <samuel.just@dreamhost.com>

diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc
index b64b25dc0fc87f2abd9b59e13fae3f2494d67af3..2126010f03c30e38e6cd1483bf1f189b12b62894 100644 (file)
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -2023,7 +2023,11 @@ void OSD::handle_watch_timeout(void *obc,
                               entity_name_t entity,
                               utime_t expire)
 {
+  // watch_lock is inside pg->lock; handle_watch_timeout checks for the race.
+  watch_lock.Unlock();
   pg->lock();
+  watch_lock.Lock();
+
   pg->handle_watch_timeout(obc, entity, expire);
   pg->unlock();
   pg->put();

diff --git a/src/osd/ReplicatedPG.cc b/src/osd/ReplicatedPG.cc
index f39a708b6113b0f7b7d72f7f9738f3ccd5c9a41a..bee16773911a1bb618378083d4dccb74cbc3a8b8 100644 (file)
--- a/src/osd/ReplicatedPG.cc
+++ b/src/osd/ReplicatedPG.cc
@@ -1535,10 +1535,10 @@ void ReplicatedPG::remove_watchers_and_notifies()
     for (map<entity_name_t, OSD::Session *>::iterator witer = obc->watchers.begin();
         witer != obc->watchers.end();
         remove_watcher(obc, (witer++)->first)) ;
-    for (map<entity_name_t, Context *>::iterator iter = obc->unconnected_watchers.begin();
+    for (map<entity_name_t,Watch::C_WatchTimeout*>::iterator iter = obc->unconnected_watchers.begin();
         iter != obc->unconnected_watchers.end();
         ) {
-      map<entity_name_t, Context *>::iterator i = iter++;
+      map<entity_name_t,Watch::C_WatchTimeout*>::iterator i = iter++;
       unregister_unconnected_watcher(obc, i->first);
     }
     for (map<Watch::Notification *, bool>::iterator niter = obc->notifs.begin();
@@ -2980,7 +2980,7 @@ void ReplicatedPG::do_osd_op_effects(OpContext *ctx)
        session->get();
        session->watches[obc] = get_osdmap()->object_locator_to_pg(soid.oid, obc->obs.oi.oloc);
       }
-      map<entity_name_t, Context *>::iterator un_iter =
+      map<entity_name_t,Watch::C_WatchTimeout*>::iterator un_iter =
        obc->unconnected_watchers.find(entity);
       if (un_iter != obc->unconnected_watchers.end()) {
        unregister_unconnected_watcher(obc, un_iter->first);
@@ -3659,10 +3659,10 @@ void ReplicatedPG::register_unconnected_watcher(void *_obc,
   pgid.set_ps(obc->obs.oi.soid.hash);
   get();
   obc->ref++;
-  Context *cb = new Watch::C_WatchTimeout(osd,
-                                         static_cast<void *>(obc),
-                                         this,
-                                         entity, expire);
+  Watch::C_WatchTimeout *cb = new Watch::C_WatchTimeout(osd,
+                                                       static_cast<void *>(obc),
+                                                       this,
+                                                       entity, expire);
   osd->watch_timer.add_event_at(expire, cb);
   obc->unconnected_watchers[entity] = cb;
 }
@@ -3672,6 +3672,14 @@ void ReplicatedPG::handle_watch_timeout(void *_obc,
                                        utime_t expire)
 {
   ObjectContext *obc = static_cast<ObjectContext *>(_obc);
+
+  if (obc->unconnected_watchers.count(entity) == 0 ||
+      obc->unconnected_watchers[entity]->expire != expire) {
+    dout(10) << "handle_watch_timeout must have raced, no/wrong unconnected_watcher " << entity << dendl;
+    put_object_context(obc);
+    return;
+  }
+
   obc->unconnected_watchers.erase(entity);
   obc->obs.oi.watchers.erase(entity);
 

diff --git a/src/osd/ReplicatedPG.h b/src/osd/ReplicatedPG.h
index c1a376d4f4b59e40aaa0ded9aeb44b865e3d9deb..4623aa805132d25b803495df8b4fa6427463bd38 100644 (file)
--- a/src/osd/ReplicatedPG.h
+++ b/src/osd/ReplicatedPG.h
@@ -278,7 +278,7 @@ public:
 
     // any entity in obs.oi.watchers MUST be in either watchers or unconnected_watchers.
     map<entity_name_t, OSD::Session *> watchers;
-    map<entity_name_t, Context *> unconnected_watchers;
+    map<entity_name_t, Watch::C_WatchTimeout *> unconnected_watchers;
     map<Watch::Notification *, bool> notifs;
 
     ObjectContext(const object_info_t &oi_, bool exists_, SnapSetContext *ssc_)

diff --git a/src/osd/Watch.h b/src/osd/Watch.h
index fa1bb99e6bffa566a0eee9f3073a4d0ce181c1c8..8fdfbd710a8e143bad99f49c4ab87df490387b6c 100644 (file)
--- a/src/osd/Watch.h
+++ b/src/osd/Watch.h
@@ -65,8 +65,8 @@ public:
     void *obc;
     void *pg;
     entity_name_t entity;
-    utime_t expire;
   public:
+    utime_t expire;
     C_WatchTimeout(OSD *_osd, void *_obc, void *_pg,
                   entity_name_t _entity, utime_t _expire) :
       osd(_osd), obc(_obc), pg(_pg), entity(_entity), expire(_expire) {}