From: Josh Durgin <jdurgin@redhat.com>
Date: Tue, 24 Feb 2015 02:44:05 +0000 (-0800)
Subject: librbd: use ImageCtx->snap_lock for ImageCtx->features
X-Git-Tag: v0.93~6^2~17
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=cffd93a32f7fdb0224b5eca0ce9c46a7fc1381a4;p=ceph.git

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 9fbc04ca4892..df75f97d2732 100644
--- 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 781b66b9576c..a0ac2bf96592 100644
--- 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 3bdf232a43e2..f30c14b4251b 100644
--- 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 e4584acc64e0..eca70c27cb54 100644
--- 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;