librbd: acquire cache_lock before refreshing parent

cache_lock needs to be acquired before snap_lock to avoid
the potential for deadlock.

Fixes: #5488
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit 703ba377e3de4007920f2ed7d8a0780f68676fe2)

Conflicts:
	src/librbd/internal.cc
        resolved by moving int r; in the scope of the block

diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
index 564f3437f0bd981a420362f87b1fbcd01cc75486..3325d73e0ee5b538e705c7bba0f7236794dcf4e6 100644 (file)
--- a/src/librbd/ImageCtx.cc
+++ b/src/librbd/ImageCtx.cc
@@ -710,11 +710,10 @@ namespace librbd {
   }
 
   void ImageCtx::clear_nonexistence_cache() {
+    assert(cache_lock.is_locked());
     if (!object_cacher)
       return;
-    cache_lock.Lock();
     object_cacher->clear_nonexistence(object_set);
-    cache_lock.Unlock();
   }
 
   int ImageCtx::register_watch() {

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index c356c79fe3b5efc1b372a60617df6be85f9cd24e..75d28a45990fa34f806e92a290867fe23f65ea59 100644 (file)
--- a/src/librbd/internal.cc
+++ b/src/librbd/internal.cc
@@ -1349,6 +1349,10 @@ reprotect_and_return_err:
 
   int open_parent(ImageCtx *ictx)
   {
+    assert(ictx->cache_lock.is_locked());
+    assert(ictx->snap_lock.is_wlocked());
+    assert(ictx->parent_lock.is_wlocked());
+
     string pool_name;
     Rados rados(ictx->md_ctx);
 
@@ -1392,11 +1396,13 @@ reprotect_and_return_err:
       return r;
     }
 
+    ictx->parent->cache_lock.Lock();
     ictx->parent->snap_lock.get_write();
     r = ictx->parent->get_snap_name(parent_snap_id, &ictx->parent->snap_name);
     if (r < 0) {
       lderr(ictx->cct) << "parent snapshot does not exist" << dendl;
       ictx->parent->snap_lock.put_write();
+      ictx->parent->cache_lock.Unlock();
       close_image(ictx->parent);
       ictx->parent = NULL;
       return r;
@@ -1410,12 +1416,14 @@ reprotect_and_return_err:
                       << ictx->parent->snap_name << dendl;
       ictx->parent->parent_lock.put_write();
       ictx->parent->snap_lock.put_write();
+      ictx->parent->cache_lock.Unlock();
       close_image(ictx->parent);
       ictx->parent = NULL;
       return r;
     }
     ictx->parent->parent_lock.put_write();
     ictx->parent->snap_lock.put_write();
+    ictx->parent->cache_lock.Unlock();
 
     return 0;
   }
@@ -1853,6 +1861,10 @@ reprotect_and_return_err:
   }
 
   int refresh_parent(ImageCtx *ictx) {
+    assert(ictx->cache_lock.is_locked());
+    assert(ictx->snap_lock.is_wlocked());
+    assert(ictx->parent_lock.is_wlocked());
+
     // close the parent if it changed or this image no longer needs
     // to read from it
     int r;
@@ -1904,10 +1916,11 @@ reprotect_and_return_err:
     vector<uint8_t> snap_protection;
     vector<uint64_t> snap_flags;
     {
-      int r;
-      RWLock::WLocker l(ictx->snap_lock);
+      Mutex::Locker cache_locker(ictx->cache_lock);
+      RWLock::WLocker snap_locker(ictx->snap_lock);
       {
-       RWLock::WLocker l2(ictx->parent_lock);
+       int r;
+       RWLock::WLocker parent_locker(ictx->parent_lock);
        ictx->lockers.clear();
        if (ictx->old_format) {
          r = read_header(ictx->md_ctx, ictx->header_oid, &ictx->header, NULL);
@@ -2058,7 +2071,7 @@ reprotect_and_return_err:
       ictx->object_map.refresh(ictx->snap_id);
 
       ictx->data_ctx.selfmanaged_snap_set_write_ctx(ictx->snapc.seq, ictx->snaps);
-    } // release snap_lock
+    } // release snap_lock and cache_lock
 
     if (new_snap) {
       _flush(ictx);
@@ -2308,10 +2321,11 @@ reprotect_and_return_err:
 
   int _snap_set(ImageCtx *ictx, const char *snap_name)
   {
-    RWLock::WLocker l(ictx->owner_lock);
-    RWLock::RLocker l1(ictx->md_lock);
-    RWLock::WLocker l2(ictx->snap_lock);
-    RWLock::WLocker l3(ictx->parent_lock);
+    RWLock::WLocker owner_locker(ictx->owner_lock);
+    RWLock::RLocker md_locker(ictx->md_lock);
+    Mutex::Locker cache_locker(ictx->cache_lock);
+    RWLock::WLocker snap_locker(ictx->snap_lock);
+    RWLock::WLocker parent_locker(ictx->parent_lock);
     int r;
     if ((snap_name != NULL) && (strlen(snap_name) != 0)) {
       r = ictx->snap_set(snap_name);