From: Jason Dillaman <dillaman@redhat.com>
Date: Mon, 13 Nov 2017 18:28:06 +0000 (-0500)
Subject: librbd: possible deadlock with synchronous maintenance operations
X-Git-Tag: v13.0.1~202^2~1
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=90b7ecd8a8aac9a5c282d44004752ade0c195331;p=ceph.git

librbd: possible deadlock with synchronous maintenance operations

Fixes: http://tracker.ceph.com/issues/22120
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
---

diff --git a/src/librbd/Operations.cc b/src/librbd/Operations.cc
index f6073b578775..d64434df5776 100644
--- a/src/librbd/Operations.cc
+++ b/src/librbd/Operations.cc
@@ -539,9 +539,11 @@ int Operations<I>::rename(const char *dstname) {
       return r;
     }
   } else {
-    RWLock::RLocker owner_lock(m_image_ctx.owner_lock);
     C_SaferCond cond_ctx;
-    execute_rename(dstname, &cond_ctx);
+    {
+      RWLock::RLocker owner_lock(m_image_ctx.owner_lock);
+      execute_rename(dstname, &cond_ctx);
+    }
 
     r = cond_ctx.wait();
     if (r < 0) {
@@ -759,36 +761,35 @@ int Operations<I>::snap_rollback(const cls::rbd::SnapshotNamespace& snap_namespa
   if (r < 0)
     return r;
 
-  RWLock::RLocker owner_locker(m_image_ctx.owner_lock);
+  C_SaferCond cond_ctx;
   {
-    // need to drop snap_lock before invalidating cache
-    RWLock::RLocker snap_locker(m_image_ctx.snap_lock);
-    if (!m_image_ctx.snap_exists) {
-      return -ENOENT;
+    RWLock::RLocker owner_locker(m_image_ctx.owner_lock);
+    {
+      // need to drop snap_lock before invalidating cache
+      RWLock::RLocker snap_locker(m_image_ctx.snap_lock);
+      if (!m_image_ctx.snap_exists) {
+        return -ENOENT;
+      }
+
+      if (m_image_ctx.snap_id != CEPH_NOSNAP || m_image_ctx.read_only) {
+        return -EROFS;
+      }
+
+      uint64_t snap_id = m_image_ctx.get_snap_id(snap_namespace, snap_name);
+      if (snap_id == CEPH_NOSNAP) {
+        lderr(cct) << "No such snapshot found." << dendl;
+        return -ENOENT;
+      }
     }
 
-    if (m_image_ctx.snap_id != CEPH_NOSNAP || m_image_ctx.read_only) {
+    r = prepare_image_update(false);
+    if (r < 0) {
       return -EROFS;
     }
 
-    uint64_t snap_id = m_image_ctx.get_snap_id(snap_namespace, snap_name);
-    if (snap_id == CEPH_NOSNAP) {
-      lderr(cct) << "No such snapshot found." << dendl;
-      return -ENOENT;
-    }
+    execute_snap_rollback(snap_namespace, snap_name, prog_ctx, &cond_ctx);
   }
 
-  r = prepare_image_update();
-  if (r < 0) {
-    return -EROFS;
-  }
-  if (m_image_ctx.exclusive_lock != nullptr &&
-      !m_image_ctx.exclusive_lock->is_lock_owner()) {
-    return -EROFS;
-  }
-
-  C_SaferCond cond_ctx;
-  execute_snap_rollback(snap_namespace, snap_name, prog_ctx, &cond_ctx);
   r = cond_ctx.wait();
   if (r < 0) {
     return r;
@@ -975,9 +976,11 @@ int Operations<I>::snap_rename(const char *srcname, const char *dstname) {
       return r;
     }
   } else {
-    RWLock::RLocker owner_lock(m_image_ctx.owner_lock);
     C_SaferCond cond_ctx;
-    execute_snap_rename(snap_id, dstname, &cond_ctx);
+    {
+      RWLock::RLocker owner_lock(m_image_ctx.owner_lock);
+      execute_snap_rename(snap_id, dstname, &cond_ctx);
+    }
 
     r = cond_ctx.wait();
     if (r < 0) {
@@ -1067,9 +1070,11 @@ int Operations<I>::snap_protect(const cls::rbd::SnapshotNamespace& snap_namespac
       return r;
     }
   } else {
-    RWLock::RLocker owner_lock(m_image_ctx.owner_lock);
     C_SaferCond cond_ctx;
-    execute_snap_protect(snap_namespace, snap_name, &cond_ctx);
+    {
+      RWLock::RLocker owner_lock(m_image_ctx.owner_lock);
+      execute_snap_protect(snap_namespace, snap_name, &cond_ctx);
+    }
 
     r = cond_ctx.wait();
     if (r < 0) {
@@ -1155,9 +1160,11 @@ int Operations<I>::snap_unprotect(const cls::rbd::SnapshotNamespace& snap_namesp
       return r;
     }
   } else {
-    RWLock::RLocker owner_lock(m_image_ctx.owner_lock);
     C_SaferCond cond_ctx;
-    execute_snap_unprotect(snap_namespace, snap_name, &cond_ctx);
+    {
+      RWLock::RLocker owner_lock(m_image_ctx.owner_lock);
+      execute_snap_unprotect(snap_namespace, snap_name, &cond_ctx);
+    }
 
     r = cond_ctx.wait();
     if (r < 0) {
@@ -1216,25 +1223,18 @@ int Operations<I>::snap_set_limit(uint64_t limit) {
     return r;
   }
 
+  C_SaferCond limit_ctx;
   {
     RWLock::RLocker owner_lock(m_image_ctx.owner_lock);
-    C_SaferCond limit_ctx;
-
-    if (m_image_ctx.exclusive_lock != nullptr &&
-	!m_image_ctx.exclusive_lock->is_lock_owner()) {
-      C_SaferCond lock_ctx;
-
-      m_image_ctx.exclusive_lock->acquire_lock(&lock_ctx);
-      r = lock_ctx.wait();
-      if (r < 0) {
-	return r;
-      }
+    r = prepare_image_update(true);
+    if (r < 0) {
+      return r;
     }
 
     execute_snap_set_limit(limit, &limit_ctx);
-    r = limit_ctx.wait();
   }
 
+  r = limit_ctx.wait();
   return r;
 }
 
@@ -1372,25 +1372,18 @@ int Operations<I>::metadata_set(const std::string &key,
     return -EROFS;
   }
 
+  C_SaferCond metadata_ctx;
   {
     RWLock::RLocker owner_lock(m_image_ctx.owner_lock);
-    C_SaferCond metadata_ctx;
-
-    if (m_image_ctx.exclusive_lock != nullptr &&
-	!m_image_ctx.exclusive_lock->is_lock_owner()) {
-      C_SaferCond lock_ctx;
-
-      m_image_ctx.exclusive_lock->acquire_lock(&lock_ctx);
-      r = lock_ctx.wait();
-      if (r < 0) {
-	return r;
-      }
+    r = prepare_image_update(true);
+    if (r < 0) {
+      return r;
     }
 
     execute_metadata_set(key, value, &metadata_ctx);
-    r = metadata_ctx.wait();
   }
 
+  r = metadata_ctx.wait();
   if (config_override && r >= 0) {
     // apply new config key immediately
     r = m_image_ctx.state->refresh_if_required();
@@ -1439,25 +1432,19 @@ int Operations<I>::metadata_remove(const std::string &key) {
   if(r < 0)
     return r;
 
+  C_SaferCond metadata_ctx;
   {
     RWLock::RLocker owner_lock(m_image_ctx.owner_lock);
-    C_SaferCond metadata_ctx;
-
-    if (m_image_ctx.exclusive_lock != nullptr &&
-        !m_image_ctx.exclusive_lock->is_lock_owner()) {
-      C_SaferCond lock_ctx;
-
-      m_image_ctx.exclusive_lock->acquire_lock(&lock_ctx);
-      r = lock_ctx.wait();
-      if (r < 0) {
-        return r;
-      }
+    r = prepare_image_update(true);
+    if (r < 0) {
+      return r;
     }
 
     execute_metadata_remove(key, &metadata_ctx);
-    r = metadata_ctx.wait();
   }
 
+  r = metadata_ctx.wait();
+
   std::string config_key;
   if (util::is_metadata_config_override(key, &config_key) && r >= 0) {
     // apply new config key immediately
@@ -1483,34 +1470,52 @@ void Operations<I>::execute_metadata_remove(const std::string &key,
 }
 
 template <typename I>
-int Operations<I>::prepare_image_update() {
+int Operations<I>::prepare_image_update(bool request_lock) {
   assert(m_image_ctx.owner_lock.is_locked() &&
          !m_image_ctx.owner_lock.is_wlocked());
-  if (m_image_ctx.image_watcher == NULL) {
+  if (m_image_ctx.image_watcher == nullptr) {
     return -EROFS;
   }
 
   // need to upgrade to a write lock
-  bool trying_lock = false;
   C_SaferCond ctx;
   m_image_ctx.owner_lock.put_read();
+  bool attempting_lock = false;
   {
     RWLock::WLocker owner_locker(m_image_ctx.owner_lock);
     if (m_image_ctx.exclusive_lock != nullptr &&
         (!m_image_ctx.exclusive_lock->is_lock_owner() ||
          !m_image_ctx.exclusive_lock->accept_requests())) {
-      m_image_ctx.exclusive_lock->try_acquire_lock(&ctx);
-      trying_lock = true;
+
+      attempting_lock = true;
+      m_image_ctx.exclusive_lock->block_requests(0);
+
+      if (request_lock) {
+        m_image_ctx.exclusive_lock->acquire_lock(&ctx);
+      } else {
+        m_image_ctx.exclusive_lock->try_acquire_lock(&ctx);
+      }
     }
   }
 
   int r = 0;
-  if (trying_lock) {
+  if (attempting_lock) {
     r = ctx.wait();
   }
+
   m_image_ctx.owner_lock.get_read();
+  if (attempting_lock && m_image_ctx.exclusive_lock != nullptr) {
+    m_image_ctx.exclusive_lock->unblock_requests();
+  }
 
-  return r;
+  if (r < 0) {
+    return r;
+  } else if (m_image_ctx.exclusive_lock != nullptr &&
+             !m_image_ctx.exclusive_lock->is_lock_owner()) {
+    return -EROFS;
+  }
+
+  return 0;
 }
 
 template <typename I>
diff --git a/src/librbd/Operations.h b/src/librbd/Operations.h
index 3ffac5c733f6..2b0c725b981b 100644
--- a/src/librbd/Operations.h
+++ b/src/librbd/Operations.h
@@ -101,7 +101,7 @@ public:
   int metadata_remove(const std::string &key);
   void execute_metadata_remove(const std::string &key, Context *on_finish);
 
-  int prepare_image_update();
+  int prepare_image_update(bool request_lock);
 
 private:
   ImageCtxT &m_image_ctx;
diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index 9c24bb7f8184..26546dbd8aa9 100644
--- a/src/librbd/internal.cc
+++ b/src/librbd/internal.cc
@@ -1349,9 +1349,8 @@ int validate_pool(IoCtx &io_ctx, CephContext *cct) {
       image_id = ictx->id;
       ictx->owner_lock.get_read();
       if (ictx->exclusive_lock != nullptr) {
-        r = ictx->operations->prepare_image_update();
-        if (r < 0 || (ictx->exclusive_lock != nullptr &&
-                      !ictx->exclusive_lock->is_lock_owner())) {
+        r = ictx->operations->prepare_image_update(false);
+        if (r < 0) {
 	  lderr(cct) << "cannot obtain exclusive lock - not removing" << dendl;
 	  ictx->owner_lock.put_read();
 	  ictx->state->close();