From 8cbd92b1fe835b1eb3a898976f9507f51cc115b2 Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Thu, 30 Apr 2015 15:32:38 -0400 Subject: [PATCH] librbd: ObjectMap::aio_update can acquire snap_lock out-of-order Detected during an fsx run where a refresh and CoR were occurring concurrently. The refresh held the snap_lock and was waiting on the object_map_lock, while the CoR held object_map_lock and was waiting for snap_lock. Fixes: #11577 Signed-off-by: Jason Dillaman --- src/librbd/AioRequest.cc | 2 ++ src/librbd/AsyncTrimRequest.cc | 2 ++ src/librbd/CopyupRequest.cc | 1 + src/librbd/ImageWatcher.cc | 7 ++++++- src/librbd/ImageWatcher.h | 1 + src/librbd/ObjectMap.cc | 5 +++-- 6 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/librbd/AioRequest.cc b/src/librbd/AioRequest.cc index fe97dc987db48..ed48c28f02526 100644 --- a/src/librbd/AioRequest.cc +++ b/src/librbd/AioRequest.cc @@ -416,6 +416,7 @@ namespace librbd { m_state = LIBRBD_AIO_WRITE_PRE; FunctionContext *ctx = new FunctionContext( boost::bind(&AioRequest::complete, this, _1)); + RWLock::RLocker snap_locker(m_ictx->snap_lock); RWLock::WLocker object_map_locker(m_ictx->object_map_lock); if (!m_ictx->object_map.aio_update(m_object_no, new_state, current_state, ctx)) { @@ -451,6 +452,7 @@ namespace librbd { m_state = LIBRBD_AIO_WRITE_POST; FunctionContext *ctx = new FunctionContext( boost::bind(&AioRequest::complete, this, _1)); + RWLock::RLocker snap_locker(m_ictx->snap_lock); RWLock::WLocker object_map_locker(m_ictx->object_map_lock); if (!m_ictx->object_map.aio_update(m_object_no, OBJECT_NONEXISTENT, OBJECT_PENDING, ctx)) { diff --git a/src/librbd/AsyncTrimRequest.cc b/src/librbd/AsyncTrimRequest.cc index e403be2593fb3..971846a96163f 100644 --- a/src/librbd/AsyncTrimRequest.cc +++ b/src/librbd/AsyncTrimRequest.cc @@ -172,6 +172,7 @@ void AsyncTrimRequest::send_pre_remove() { } else { // flag the objects as pending deletion Context *ctx = create_callback_context(); + RWLock::RLocker snap_locker(m_image_ctx.snap_lock); RWLock::WLocker object_map_locker(m_image_ctx.object_map_lock); if (!m_image_ctx.object_map.aio_update(m_delete_start, m_num_objects, OBJECT_PENDING, OBJECT_EXISTS, @@ -210,6 +211,7 @@ bool AsyncTrimRequest::send_post_remove() { } else { // flag the pending objects as removed Context *ctx = create_callback_context(); + RWLock::RLocker snap_lock(m_image_ctx.snap_lock); RWLock::WLocker object_map_locker(m_image_ctx.object_map_lock); if (!m_image_ctx.object_map.aio_update(m_delete_start, m_num_objects, OBJECT_NONEXISTENT, diff --git a/src/librbd/CopyupRequest.cc b/src/librbd/CopyupRequest.cc index 1b8ff2529df4c..a63a59bc94bcb 100644 --- a/src/librbd/CopyupRequest.cc +++ b/src/librbd/CopyupRequest.cc @@ -40,6 +40,7 @@ public: uint64_t snap_id = m_snap_ids[m_snap_id_idx]; if (snap_id == CEPH_NOSNAP) { RWLock::RLocker owner_locker(m_image_ctx.owner_lock); + RWLock::RLocker snap_locker(m_image_ctx.snap_lock); RWLock::WLocker object_map_locker(m_image_ctx.object_map_lock); assert(m_image_ctx.image_watcher->is_lock_owner()); bool sent = m_image_ctx.object_map.aio_update(m_object_no, OBJECT_EXISTS, diff --git a/src/librbd/ImageWatcher.cc b/src/librbd/ImageWatcher.cc index 1f90431995604..2b0d93cabaebf 100644 --- a/src/librbd/ImageWatcher.cc +++ b/src/librbd/ImageWatcher.cc @@ -56,8 +56,13 @@ ImageWatcher::~ImageWatcher() } bool ImageWatcher::is_lock_supported() const { - assert(m_image_ctx.owner_lock.is_locked()); RWLock::RLocker l(m_image_ctx.snap_lock); + return is_lock_supported(m_image_ctx.snap_lock); +} + +bool ImageWatcher::is_lock_supported(const RWLock &) const { + assert(m_image_ctx.owner_lock.is_locked()); + assert(m_image_ctx.snap_lock.is_locked()); return ((m_image_ctx.features & RBD_FEATURE_EXCLUSIVE_LOCK) != 0 && !m_image_ctx.read_only && m_image_ctx.snap_id == CEPH_NOSNAP); } diff --git a/src/librbd/ImageWatcher.h b/src/librbd/ImageWatcher.h index 73292b2d98ac2..ded4fe9287636 100644 --- a/src/librbd/ImageWatcher.h +++ b/src/librbd/ImageWatcher.h @@ -31,6 +31,7 @@ namespace librbd { ~ImageWatcher(); bool is_lock_supported() const; + bool is_lock_supported(const RWLock &snap_lock) const; bool is_lock_owner() const; int register_watch(); diff --git a/src/librbd/ObjectMap.cc b/src/librbd/ObjectMap.cc index d34339b0441c1..1aa6d7407968c 100644 --- a/src/librbd/ObjectMap.cc +++ b/src/librbd/ObjectMap.cc @@ -450,9 +450,10 @@ bool ObjectMap::aio_update(uint64_t start_object_no, uint64_t end_object_no, const boost::optional ¤t_state, Context *on_finish) { - assert(m_image_ctx.test_features(RBD_FEATURE_OBJECT_MAP)); + assert(m_image_ctx.snap_lock.is_locked()); + assert((m_image_ctx.features & RBD_FEATURE_OBJECT_MAP) != 0); assert(m_image_ctx.owner_lock.is_locked()); - assert(!m_image_ctx.image_watcher->is_lock_supported() || + assert(!m_image_ctx.image_watcher->is_lock_supported(m_image_ctx.snap_lock) || m_image_ctx.image_watcher->is_lock_owner()); assert(m_image_ctx.object_map_lock.is_wlocked()); assert(start_object_no < end_object_no); -- 2.39.5