librbd: use snap_lock to protect ImageCtx->flags

This is another step towards eliminating md_lock from the writeback
path. Almost all the places that use ImageCtx->flags already use
snap_lock, so there's no need to create a new lock. For the others,
add a helper, test_flags() that acquires the lock, similar to
test_features().

This also makes sure we look up flags of the snapshot we're operating
on, instead of those for head.

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

diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
index 493ad7551700c7e25a928c241bae0d8371a8a48e..01978dcfc5fd23d709d8086047acffdc3a2503f7 100644 (file)
--- a/src/librbd/ImageCtx.cc
+++ b/src/librbd/ImageCtx.cc
@@ -473,6 +473,7 @@ namespace librbd {
 
   int ImageCtx::get_flags(librados::snap_t _snap_id, uint64_t *_flags) const
   {
+    assert(snap_lock.is_locked());
     if (_snap_id == CEPH_NOSNAP) {
       *_flags = flags;
       return 0;
@@ -485,6 +486,14 @@ namespace librbd {
     return -ENOENT;
   }
 
+  bool ImageCtx::test_flags(uint64_t test_flags) const
+  {
+    RWLock::RLocker l(snap_lock);
+    uint64_t snap_flags;
+    get_flags(snap_id, &snap_flags);
+    return ((snap_flags & test_flags) == test_flags);
+  }
+
   const parent_info* ImageCtx::get_parent_info(snap_t in_snap_id) const
   {
     assert(snap_lock.is_locked());

diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h
index a0ac2bf96592baf8e4d42ee29c33c588a03d971f..3d006982abcb3e0423f4a370e0ae00bd64b6cc44 100644 (file)
--- a/src/librbd/ImageCtx.h
+++ b/src/librbd/ImageCtx.h
@@ -78,7 +78,7 @@ namespace librbd {
                    // 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 and features
+    RWLock snap_lock; // protects snapshot-related member variables, features, and flags
     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
@@ -161,6 +161,7 @@ namespace librbd {
                     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;
+    bool test_flags(uint64_t test_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;
     std::string get_parent_image_id(librados::snap_t in_snap_id) const;

diff --git a/src/librbd/ObjectMap.cc b/src/librbd/ObjectMap.cc
index c90a8b342008054d72fa817d5e5969710d25da9b..edd33b7e76e5064e9a96de799eb02bb24d258b31 100644 (file)
--- a/src/librbd/ObjectMap.cc
+++ b/src/librbd/ObjectMap.cc
@@ -114,7 +114,7 @@ bool ObjectMap::object_may_exist(uint64_t object_no) const
 {
   // Fall back to default logic if object map is disabled or invalid
   if (!m_image_ctx.test_features(RBD_FEATURE_OBJECT_MAP) ||
-      ((m_image_ctx.flags & RBD_FLAG_OBJECT_MAP_INVALID) != 0)) {
+      m_image_ctx.test_flags(RBD_FLAG_OBJECT_MAP_INVALID)) {
     return true;
   }
 
@@ -290,7 +290,10 @@ bool ObjectMap::aio_update(uint64_t start_object_no, uint64_t end_object_no,
 }
 
 void ObjectMap::invalidate() {
-  if ((m_image_ctx.flags & RBD_FLAG_OBJECT_MAP_INVALID) != 0) {
+  assert(m_image_ctx.snap_lock.is_wlocked());
+  uint64_t flags;
+  m_image_ctx.get_flags(m_image_ctx.snap_id, &flags);
+  if ((flags & RBD_FLAG_OBJECT_MAP_INVALID) != 0) {
     return;
   }
 
@@ -351,12 +354,12 @@ bool ObjectMap::Request::should_complete(int r) {
 }
 
 bool ObjectMap::Request::invalidate() {
-  if ((m_image_ctx.flags & RBD_FLAG_OBJECT_MAP_INVALID) != 0) {
+  if (m_image_ctx.test_flags(RBD_FLAG_OBJECT_MAP_INVALID)) {
     return true;
   }
 
   CephContext *cct = m_image_ctx.cct;
-  RWLock::WLocker l(m_image_ctx.md_lock);
+  RWLock::WLocker snap_locker(m_image_ctx.snap_lock);
 
   lderr(cct) << &m_image_ctx << " invalidating object map" << dendl;
   m_state = STATE_INVALIDATE;

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index c51ae75bce2bfce38d18d21d9e3c6e636a1145f7..6db4eba49852f856fa5cce9b9b7f2f429ed379d7 100644 (file)
--- a/src/librbd/internal.cc
+++ b/src/librbd/internal.cc
@@ -319,7 +319,7 @@ namespace librbd {
     }
 
     {
-      RWLock::RLocker l(ictx->md_lock);
+      RWLock::WLocker l(ictx->snap_lock);
       if (ictx->object_map != NULL) {
        ictx->object_map->rollback(snap_id);
       }