librbd: cancel in-progress maint operations before releasing lock

Ensure that all in-flight maintenance operations (resize, flatten) are
not running when the exclusive lock is released.  The lock will be
released when transitioning to a snapshot, closing the image, or
cooperatively when another client requests the lock.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/librbd/AsyncFlattenRequest.cc b/src/librbd/AsyncFlattenRequest.cc
index eacdc4bdb556d66263472650a50b119a093c69c8..5f262430e231868a1861625b0610aad7b1919859 100644 (file)
--- a/src/librbd/AsyncFlattenRequest.cc
+++ b/src/librbd/AsyncFlattenRequest.cc
@@ -119,7 +119,7 @@ void AsyncFlattenRequest::send() {
       boost::lambda::_1, &m_image_ctx, m_object_size, m_snapc,
       boost::lambda::_2));
   AsyncObjectThrottle *throttle = new AsyncObjectThrottle(
-    context_factory, create_callback_context(), m_prog_ctx, 0,
+    *this, context_factory, create_callback_context(), m_prog_ctx, 0,
     m_overlap_objects);
   throttle->start_ops(cct->_conf->rbd_concurrent_management_ops);
 }

diff --git a/src/librbd/AsyncObjectThrottle.cc b/src/librbd/AsyncObjectThrottle.cc
index 28a6a348d1ea1b11d894c840412bfbd099819f47..4290eb8636c03178d7e4be86821063dfc6e5c26f 100644 (file)
--- a/src/librbd/AsyncObjectThrottle.cc
+++ b/src/librbd/AsyncObjectThrottle.cc
@@ -2,18 +2,20 @@
 // vim: ts=8 sw=2 smarttab
 #include "librbd/AsyncObjectThrottle.h"
 #include "include/rbd/librbd.hpp"
+#include "librbd/AsyncRequest.h"
 
 namespace librbd
 {
 
-AsyncObjectThrottle::AsyncObjectThrottle(const ContextFactory& context_factory,
+AsyncObjectThrottle::AsyncObjectThrottle(const AsyncRequest& async_request,
+                                         const ContextFactory& context_factory,
                                         Context *ctx, ProgressContext &prog_ctx,
                                         uint64_t object_no,
                                         uint64_t end_object_no)
   : m_lock("librbd::AsyncThrottle::m_lock"),
-    m_context_factory(context_factory), m_ctx(ctx), m_prog_ctx(prog_ctx),
-    m_object_no(object_no), m_end_object_no(end_object_no), m_current_ops(0),
-    m_ret(0)
+    m_async_request(async_request), m_context_factory(context_factory),
+    m_ctx(ctx), m_prog_ctx(prog_ctx), m_object_no(object_no),
+    m_end_object_no(end_object_no), m_current_ops(0), m_ret(0)
 {
 }
 
@@ -56,7 +58,11 @@ void AsyncObjectThrottle::finish_op(int r) {
 void AsyncObjectThrottle::start_next_op() {
   bool done = false;
   while (!done) {
-    if (m_ret != 0 || m_object_no >= m_end_object_no) {
+    if (m_async_request.is_canceled() && m_ret == 0) {
+      // allow in-flight ops to complete, but don't start new ops
+      m_ret = -ERESTART;
+      return;
+    } else if (m_ret != 0 || m_object_no >= m_end_object_no) {
       return;
     }
 

diff --git a/src/librbd/AsyncObjectThrottle.h b/src/librbd/AsyncObjectThrottle.h
index c7b5bf69a627ad4a2c37cc12e17b42b5a0187354..83d69d8c773c1cdd5d4c36cd3feb933e58bf9fd7 100644 (file)
--- a/src/librbd/AsyncObjectThrottle.h
+++ b/src/librbd/AsyncObjectThrottle.h
@@ -11,6 +11,7 @@
 
 namespace librbd
 {
+class AsyncRequest;
 class ProgressContext;
 
 class AsyncObjectThrottleFinisher {
@@ -42,7 +43,8 @@ public:
   typedef boost::function<C_AsyncObjectThrottle*(AsyncObjectThrottle&,
                                           uint64_t)> ContextFactory;
 
-  AsyncObjectThrottle(const ContextFactory& context_factory, Context *ctx,
+  AsyncObjectThrottle(const AsyncRequest &async_request,
+                      const ContextFactory& context_factory, Context *ctx,
                      ProgressContext &prog_ctx, uint64_t object_no,
                      uint64_t end_object_no);
 
@@ -51,6 +53,7 @@ public:
 
 private:
   Mutex m_lock;
+  const AsyncRequest &m_async_request;
   ContextFactory m_context_factory;
   Context *m_ctx;
   ProgressContext &m_prog_ctx;

diff --git a/src/librbd/AsyncRequest.cc b/src/librbd/AsyncRequest.cc
index 332d883cbed20ec53c9e44b4f6c1e7288a3b2302..825c8c4e50a5fda4147ab4e5c4262b2faa405845 100644 (file)
--- a/src/librbd/AsyncRequest.cc
+++ b/src/librbd/AsyncRequest.cc
@@ -1,12 +1,26 @@
 // -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
 // vim: ts=8 sw=2 smarttab
 #include "librbd/AsyncRequest.h"
+#include "librbd/ImageCtx.h"
 #include "librbd/internal.h"
 #include <boost/bind.hpp>
 
 namespace librbd
 {
 
+AsyncRequest::AsyncRequest(ImageCtx &image_ctx, Context *on_finish)
+  : m_image_ctx(image_ctx), m_on_finish(on_finish), m_canceled(false),
+    m_xlist_item(this) {
+  Mutex::Locker l(m_image_ctx.async_ops_lock);
+  m_image_ctx.async_requests.push_back(&m_xlist_item);
+}
+
+AsyncRequest::~AsyncRequest() {
+  Mutex::Locker l(m_image_ctx.async_ops_lock);
+  assert(m_xlist_item.remove_myself());
+  m_image_ctx.async_requests_cond.Signal();
+}
+
 librados::AioCompletion *AsyncRequest::create_callback_completion() {
   return librados::Rados::aio_create_completion(create_callback_context(),
                                                NULL, rados_ctx_cb);

diff --git a/src/librbd/AsyncRequest.h b/src/librbd/AsyncRequest.h
index 3fdc85c031b4d824c433890d880f97b6775e1a92..25be0ed6d29d2caa0b8cbe8c0b39aba2b404eeac 100644 (file)
--- a/src/librbd/AsyncRequest.h
+++ b/src/librbd/AsyncRequest.h
@@ -6,6 +6,7 @@
 #include "include/int_types.h"
 #include "include/Context.h"
 #include "include/rados/librados.hpp"
+#include "include/xlist.h"
 
 namespace librbd {
 
@@ -14,15 +15,14 @@ class ImageCtx;
 class AsyncRequest
 {
 public:
-  AsyncRequest(ImageCtx &image_ctx, Context *on_finish)
-    : m_image_ctx(image_ctx), m_on_finish(on_finish)
-  {
-  }
-
-  virtual ~AsyncRequest() {}
+  AsyncRequest(ImageCtx &image_ctx, Context *on_finish);
+  virtual ~AsyncRequest();
 
   void complete(int r) {
-    if (should_complete(r)) {
+    if (m_canceled) {
+      m_on_finish->complete(-ERESTART);
+      delete this;
+    } else if (should_complete(r)) {
       m_on_finish->complete(r);
       delete this;
     }
@@ -30,6 +30,13 @@ public:
 
   virtual void send() = 0;
 
+  inline bool is_canceled() const {
+    return m_canceled;
+  }
+  inline void cancel() {
+    m_canceled = true;
+  }
+
 protected:
   ImageCtx &m_image_ctx;
   Context *m_on_finish;
@@ -38,6 +45,10 @@ protected:
   Context *create_callback_context();
 
   virtual bool should_complete(int r) = 0;
+
+private:
+  bool m_canceled;
+  xlist<AsyncRequest *>::item m_xlist_item;
 };
 
 class C_AsyncRequest : public Context

diff --git a/src/librbd/AsyncTrimRequest.cc b/src/librbd/AsyncTrimRequest.cc
index 3bbab55252582bfdf9228a387ac5e28d57ffe392..14a58a8e3b949b253c2f662a0fc469735d104bae 100644 (file)
--- a/src/librbd/AsyncTrimRequest.cc
+++ b/src/librbd/AsyncTrimRequest.cc
@@ -154,7 +154,7 @@ void AsyncTrimRequest::send_remove_objects() {
     boost::lambda::bind(boost::lambda::new_ptr<AsyncTrimObjectContext>(),
       boost::lambda::_1, &m_image_ctx, boost::lambda::_2));
   AsyncObjectThrottle *throttle = new AsyncObjectThrottle(
-    context_factory, ctx, m_prog_ctx, m_delete_start, m_num_objects);
+    *this, context_factory, ctx, m_prog_ctx, m_delete_start, m_num_objects);
   throttle->start_ops(cct->_conf->rbd_concurrent_management_ops);
 }
 

diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
index 2637d64acc155fd132eb37e2c76da6cfde5e24af..1e499382b8ddbd59387da2d33125101fb147bcd5 100644 (file)
--- a/src/librbd/ImageCtx.cc
+++ b/src/librbd/ImageCtx.cc
@@ -8,6 +8,7 @@
 #include "common/perf_counters.h"
 
 #include "librbd/AsyncOperation.h"
+#include "librbd/AsyncRequest.h"
 #include "librbd/internal.h"
 #include "librbd/ImageCtx.h"
 #include "librbd/ImageWatcher.h"
@@ -710,4 +711,20 @@ namespace librbd {
                    << "count=" << async_ops.size() << dendl;
     async_ops.front()->add_flush_context(on_finish);
   }
+
+  void ImageCtx::cancel_async_requests() {
+    Mutex::Locker l(async_ops_lock);
+    ldout(cct, 10) << "canceling async requests: count="
+                   << async_requests.size() << dendl;
+
+    for (xlist<AsyncRequest*>::iterator it = async_requests.begin();
+         !it.end(); ++it) {
+      ldout(cct, 10) << "canceling async request: " << *it << dendl;
+      (*it)->cancel();
+    }
+
+    while (!async_requests.empty()) {
+      async_requests_cond.Wait(async_ops_lock);
+    }
+  }
 }

diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h
index 781b66b9576cb33fa622bdfe4a87f23a98b7c0ae..c7b48a0e1d3c12df20a9afe94ee46b110c41acc8 100644 (file)
--- a/src/librbd/ImageCtx.h
+++ b/src/librbd/ImageCtx.h
@@ -36,6 +36,7 @@ class PerfCounters;
 namespace librbd {
 
   class AsyncOperation;
+  class AsyncRequest;
   class CopyupRequest;
   class ImageWatcher;
   class ObjectMap;
@@ -112,6 +113,8 @@ namespace librbd {
     std::map<uint64_t, CopyupRequest*> copyup_list;
 
     xlist<AsyncOperation*> async_ops;
+    xlist<AsyncRequest*> async_requests;
+    Cond async_requests_cond;
 
     ObjectMap *object_map;
 
@@ -187,6 +190,8 @@ namespace librbd {
 
     void flush_async_operations();
     void flush_async_operations(Context *on_finish);
+
+    void cancel_async_requests();
   };
 }
 

diff --git a/src/librbd/ImageWatcher.cc b/src/librbd/ImageWatcher.cc
index a1afa1eb8ef048ae6217cee470305fe03a1ab5fe..de30d265b3e8b3030544b2283fe7d87bc96fe20c 100644 (file)
--- a/src/librbd/ImageWatcher.cc
+++ b/src/librbd/ImageWatcher.cc
@@ -73,7 +73,8 @@ bool ImageWatcher::is_lock_supported() const {
 
 bool ImageWatcher::is_lock_owner() const {
   assert(m_image_ctx.owner_lock.is_locked());
-  return (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED);
+  return (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED ||
+          m_lock_owner_state == LOCK_OWNER_STATE_RELEASING);
 }
 
 int ImageWatcher::register_watch() {
@@ -328,6 +329,20 @@ int ImageWatcher::lock() {
   return 0;
 }
 
+void ImageWatcher::prepare_unlock() {
+  assert(m_image_ctx.owner_lock.is_wlocked());
+  if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) {
+    m_lock_owner_state = LOCK_OWNER_STATE_RELEASING;
+  }
+}
+
+void ImageWatcher::cancel_unlock() {
+  assert(m_image_ctx.owner_lock.is_wlocked());
+  if (m_lock_owner_state == LOCK_OWNER_STATE_RELEASING) {
+    m_lock_owner_state = LOCK_OWNER_STATE_LOCKED;
+  }
+}
+
 int ImageWatcher::unlock()
 {
   assert(m_image_ctx.owner_lock.is_wlocked());
@@ -357,6 +372,14 @@ int ImageWatcher::unlock()
 
 void ImageWatcher::release_lock()
 {
+  ldout(m_image_ctx.cct, 10) << "releasing exclusive lock by request" << dendl;
+  {
+    RWLock::WLocker l(m_image_ctx.owner_lock);
+    prepare_unlock();
+  }
+
+  m_image_ctx.cancel_async_requests();
+
   RWLock::WLocker l(m_image_ctx.owner_lock);
   {
     RWLock::WLocker l2(m_image_ctx.md_lock);
@@ -809,7 +832,7 @@ void ImageWatcher::handle_payload(const FlattenPayload &payload,
                                  bufferlist *out) {
 
   RWLock::RLocker l(m_image_ctx.owner_lock);
-  if (is_lock_owner()) {
+  if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) {
     int r = 0;
     bool new_request = false;
     if (payload.async_request_id.client_id == get_client_id()) {
@@ -848,7 +871,7 @@ void ImageWatcher::handle_payload(const FlattenPayload &payload,
 void ImageWatcher::handle_payload(const ResizePayload &payload,
                                  bufferlist *out) {
   RWLock::RLocker l(m_image_ctx.owner_lock);
-  if (is_lock_owner()) {
+  if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) {
     int r = 0;
     bool new_request = false;
     if (payload.async_request_id.client_id == get_client_id()) {
@@ -888,7 +911,7 @@ void ImageWatcher::handle_payload(const ResizePayload &payload,
 void ImageWatcher::handle_payload(const SnapCreatePayload &payload,
                                  bufferlist *out) {
   RWLock::RLocker l(m_image_ctx.owner_lock);
-  if (is_lock_owner()) {
+  if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) {
     ldout(m_image_ctx.cct, 10) << "remote snap_create request: "
                               << payload.snap_name << dendl;
     int r = librbd::snap_create(&m_image_ctx, payload.snap_name.c_str(), false);

diff --git a/src/librbd/ImageWatcher.h b/src/librbd/ImageWatcher.h
index 3a17f7f1722b22fb682e287746f96f320990b009..17573d74daac3d227b47ae1303597ca4212e257f 100644 (file)
--- a/src/librbd/ImageWatcher.h
+++ b/src/librbd/ImageWatcher.h
@@ -40,6 +40,8 @@ namespace librbd {
     int try_lock();
     int request_lock(const boost::function<int(AioCompletion*)>& restart_op,
                     AioCompletion* c);
+    void prepare_unlock();
+    void cancel_unlock();
     int unlock();
 
     void assert_header_locked(librados::ObjectWriteOperation *op);
@@ -56,7 +58,8 @@ namespace librbd {
 
     enum LockOwnerState {
       LOCK_OWNER_STATE_NOT_LOCKED,
-      LOCK_OWNER_STATE_LOCKED
+      LOCK_OWNER_STATE_LOCKED,
+      LOCK_OWNER_STATE_RELEASING
     };
 
     enum WatchState {

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index e4584acc64e038f9f47e839808e642fc5f20cf20..db9199023fffa30c3772eaa17f9016c5654a2d4a 100644 (file)
--- a/src/librbd/internal.cc
+++ b/src/librbd/internal.cc
@@ -2345,6 +2345,19 @@ reprotect_and_return_err:
     // ignore return value, since we may be set to a non-existent
     // snapshot and the user is trying to fix that
     ictx_check(ictx);
+
+    bool unlocking = false;
+    {
+      RWLock::WLocker l(ictx->owner_lock);
+      if (ictx->image_watcher != NULL && ictx->image_watcher->is_lock_owner() &&
+          snap_name != NULL && strlen(snap_name) != 0) {
+        // stop incoming requests since we will release the lock
+        ictx->image_watcher->prepare_unlock();
+        unlocking = true;
+      }
+    }
+
+    ictx->cancel_async_requests();
     ictx->flush_async_operations();
     if (ictx->object_cacher) {
       // complete pending writes before we're set to a snapshot and
@@ -2354,13 +2367,16 @@ reprotect_and_return_err:
     }
     int r = _snap_set(ictx, snap_name);
     if (r < 0) {
+      RWLock::WLocker l(ictx->owner_lock);
+      if (unlocking) {
+        ictx->image_watcher->cancel_unlock();
+      }
       return r;
     }
 
     RWLock::WLocker l(ictx->owner_lock);
     if (ictx->image_watcher != NULL) {
-      if (!ictx->image_watcher->is_lock_supported() &&
-         ictx->image_watcher->is_lock_owner()) {
+      if (unlocking) {
        r = ictx->image_watcher->unlock();
        if (r < 0) {
          lderr(ictx->cct) << "error unlocking image: " << cpp_strerror(r)
@@ -2411,6 +2427,15 @@ reprotect_and_return_err:
   {
     ldout(ictx->cct, 20) << "close_image " << ictx << dendl;
 
+    {
+      RWLock::WLocker l(ictx->owner_lock);
+      if (ictx->image_watcher != NULL && ictx->image_watcher->is_lock_owner()) {
+        // stop incoming requests
+        ictx->image_watcher->prepare_unlock();
+      }
+    }
+
+    ictx->cancel_async_requests();
     ictx->readahead.wait_for_pending();
     if (ictx->object_cacher) {
       ictx->shutdown_cache(); // implicitly flushes

diff --git a/src/test/librbd/test_internal.cc b/src/test/librbd/test_internal.cc
index 2831db5f86aad88aa9a8fefb3395654e4204ded6..ddd06afed8ec340e3aef225c668bdf98ce9ef36a 100644 (file)
--- a/src/test/librbd/test_internal.cc
+++ b/src/test/librbd/test_internal.cc
@@ -281,3 +281,41 @@ TEST_F(TestInternal, AioDiscardRequestsLock) {
   ASSERT_FALSE(is_owner);
   ASSERT_FALSE(c->is_complete());
 }
+
+TEST_F(TestInternal, CancelAsyncResize) {
+  REQUIRE_FEATURE(RBD_FEATURE_EXCLUSIVE_LOCK);
+
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+
+  {
+    RWLock::WLocker l(ictx->owner_lock);
+    ASSERT_EQ(0, ictx->image_watcher->try_lock());
+    ASSERT_TRUE(ictx->image_watcher->is_lock_owner());
+  }
+
+  uint64_t size;
+  ASSERT_EQ(0, librbd::get_size(ictx, &size));
+
+  uint32_t attempts = 0;
+  while (attempts++ < 20 && size > 0) {
+    C_SaferCond ctx;
+    librbd::NoOpProgressContext prog_ctx;
+
+    size -= MIN(size, 1<<18);
+    {
+      RWLock::RLocker l(ictx->owner_lock);
+      ASSERT_EQ(0, librbd::async_resize(ictx, &ctx, size, prog_ctx));
+    }
+
+    // try to interrupt the in-progress resize
+    ictx->cancel_async_requests();
+
+    int r = ctx.wait();
+    if (r == -ERESTART) {
+      std::cout << "detected canceled async request" << std::endl;
+      break;
+    }
+    ASSERT_EQ(0, r);
+  }
+}