librbd: use ImageCtx->snap_lock for ImageCtx->features

This was being protected by md_lock, but that has become too coarse
since it is used to prevent writes from proceeding while flushing
caches for a snapshot. With the addition of ObjectMap and
ImageWatcher, writeback could try to acquire md_lock again, leading to
a deadlock.

Signed-off-by: Josh Durgin <jdurgin@redhat.com>

diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
index 9fbc04ca489287e8e3c6c9e49df59bcbb0f8487d..df75f97d2732442e34252d832f39e4646d0b125d 100644 (file)
--- a/src/librbd/ImageCtx.cc
+++ b/src/librbd/ImageCtx.cc
@@ -439,6 +439,7 @@ namespace librbd {
 
   int ImageCtx::get_features(snap_t in_snap_id, uint64_t *out_features) const
   {
+    assert(snap_lock.is_locked());
     if (in_snap_id == CEPH_NOSNAP) {
       *out_features = features;
       return 0;

diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h
index 781b66b9576cb33fa622bdfe4a87f23a98b7c0ae..a0ac2bf96592baf8e4d42ee29c33c588a03d971f 100644 (file)
--- a/src/librbd/ImageCtx.h
+++ b/src/librbd/ImageCtx.h
@@ -69,15 +69,16 @@ namespace librbd {
 
     /**
      * Lock ordering:
-     * owner_lock, md_lock, cache_lock, snap_lock, parent_lock, refresh_lock,
-     * object_map_lock, async_op_lock
+     *
+     * owner_lock, md_lock, cache_lock, snap_lock, parent_lock,
+     * refresh_lock, object_map_lock, async_op_lock
      */
     RWLock owner_lock; // protects exclusive lock leadership updates
     RWLock md_lock; // protects access to the mutable image metadata that
                    // isn't guarded by other locks below
                    // (size, features, image locks, etc)
     Mutex cache_lock; // used as client_lock for the ObjectCacher
-    RWLock snap_lock; // protects snapshot-related member variables:
+    RWLock snap_lock; // protects snapshot-related member variables and features
     RWLock parent_lock; // protects parent_md and parent
     Mutex refresh_lock; // protects refresh_seq and last_refresh
     RWLock object_map_lock; // protects object map updates
@@ -158,6 +159,7 @@ namespace librbd {
     uint64_t get_image_size(librados::snap_t in_snap_id) const;
     int get_features(librados::snap_t in_snap_id,
                     uint64_t *out_features) const;
+    bool test_features(uint64_t test_features) const;
     int get_flags(librados::snap_t in_snap_id, uint64_t *flags) const;
     const parent_info* get_parent_info(librados::snap_t in_snap_id) const;
     int64_t get_parent_pool_id(librados::snap_t in_snap_id) const;

diff --git a/src/librbd/ObjectMap.cc b/src/librbd/ObjectMap.cc
index 3bdf232a43e282c05ae4a9cfbf19e4ee9238a444..f30c14b4251b9273b9de90d7b80b69ae9a1d4f91 100644 (file)
--- a/src/librbd/ObjectMap.cc
+++ b/src/librbd/ObjectMap.cc
@@ -130,8 +130,11 @@ bool ObjectMap::object_may_exist(uint64_t object_no) const
 }
 
 void ObjectMap::refresh(uint64_t snap_id)
-{ 
-  if ((m_image_ctx.features & RBD_FEATURE_OBJECT_MAP) == 0) {
+{
+  assert(m_image_ctx.snap_lock.is_locked());
+  uint64_t features;
+  m_image_ctx.get_features(m_image_ctx.snap_id, &features);
+  if ((features & RBD_FEATURE_OBJECT_MAP) == 0) {
     return;
   }
 
@@ -164,7 +167,10 @@ void ObjectMap::refresh(uint64_t snap_id)
 }
 
 void ObjectMap::rollback(uint64_t snap_id) {
-  if ((m_image_ctx.features & RBD_FEATURE_OBJECT_MAP) == 0) {
+  assert(m_image_ctx.snap_lock.is_wlocked());
+  uint64_t features;
+  m_image_ctx.get_features(snap_id, &features);
+  if ((features & RBD_FEATURE_OBJECT_MAP) == 0) {
     return;
   }
 
@@ -197,7 +203,10 @@ void ObjectMap::rollback(uint64_t snap_id) {
 }
 
 void ObjectMap::snapshot(uint64_t snap_id) {
-  if ((m_image_ctx.features & RBD_FEATURE_OBJECT_MAP) == 0) {
+  assert(m_image_ctx.snap_lock.is_wlocked());
+  uint64_t features;
+  m_image_ctx.get_features(CEPH_NOSNAP, &features);
+  if ((features & RBD_FEATURE_OBJECT_MAP) == 0) {
     return;
   }
 

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index e4584acc64e038f9f47e839808e642fc5f20cf20..eca70c27cb54a44969e9318a1e1a8450e19826ff 100644 (file)
--- a/src/librbd/internal.cc
+++ b/src/librbd/internal.cc
@@ -1330,8 +1330,7 @@ reprotect_and_return_err:
     int r = ictx_check(ictx);
     if (r < 0)
       return r;
-    RWLock::RLocker l(ictx->md_lock);
-    RWLock::RLocker l2(ictx->snap_lock);
+    RWLock::RLocker l(ictx->snap_lock);
     return ictx->get_features(ictx->snap_id, features);
   }
 
@@ -2191,12 +2190,14 @@ reprotect_and_return_err:
 
     src->md_lock.get_read();
     src->snap_lock.get_read();
+    uint64_t src_features;
+    src->get_features(src->snap_id, &src_features);
     uint64_t src_size = src->get_image_size(src->snap_id);
     src->snap_lock.put_read();
     src->md_lock.put_read();
 
     int r = create(dest_md_ctx, destname, src_size, src->old_format,
-                  src->features, &order, src->stripe_unit, src->stripe_count);
+                  src_features, &order, src->stripe_unit, src->stripe_count);
     if (r < 0) {
       lderr(cct) << "header creation failed" << dendl;
       return r;