librbd: prevent concurrent AIO callbacks to external clients

With the various threads and conditional IO paths within librbd, it was
possible for external AIO callbacks to be concurrently executed from
different librbd threads. These callbacks should be serialized to reduce
the unexpected potential for data corruption.

Fixes: http://tracker.ceph.com/issues/40417
Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
index 5a8bf337745a7954f1d6bb8d27e530b87de23d47..8c5a07eacaf53c207cd2c9c8aeab45fb1f9b2295 100644 (file)
--- a/src/librbd/ImageCtx.cc
+++ b/src/librbd/ImageCtx.cc
@@ -122,7 +122,8 @@ public:
       operations(new Operations<>(*this)),
       exclusive_lock(nullptr), object_map(nullptr),
       io_work_queue(nullptr), op_work_queue(nullptr),
-      completed_reqs(32),
+      external_callback_completions(32),
+      event_socket_completions(32),
       asok_hook(nullptr),
       trace_endpoint("librbd")
   {

diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h
index 32f5f5f2b965558427502dfa80adee1c9702c5ce..2ae215fcab192c293c486070755db59dd1ba0f79 100644 (file)
--- a/src/librbd/ImageCtx.h
+++ b/src/librbd/ImageCtx.h
@@ -172,9 +172,14 @@ namespace librbd {
 
     ContextWQ *op_work_queue;
 
-    boost::lockfree::queue<
+    typedef boost::lockfree::queue<
       io::AioCompletion*,
-      boost::lockfree::allocator<ceph::allocator<void>>> completed_reqs;
+      boost::lockfree::allocator<ceph::allocator<void>>> Completions;
+
+    Completions external_callback_completions;
+    std::atomic<bool> external_callback_in_progress = {false};
+
+    Completions event_socket_completions;
     EventSocket event_socket;
 
     bool ignore_migrating = false;

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index dadf699c29d723ec1c149067b6455cd45e236f64..1cefd073b528a19b26977fb75132b27cfedf9f80 100644 (file)
--- a/src/librbd/internal.cc
+++ b/src/librbd/internal.cc
@@ -1839,7 +1839,7 @@ int validate_pool(IoCtx &io_ctx, CephContext *cct) {
     ldout(cct, 20) << __func__ << " " << ictx << " numcomp = " << numcomp
                    << dendl;
     int i = 0;
-    while (i < numcomp && ictx->completed_reqs.pop(comps[i])) {
+    while (i < numcomp && ictx->event_socket_completions.pop(comps[i])) {
       ++i;
     }
 

diff --git a/src/librbd/io/AioCompletion.cc b/src/librbd/io/AioCompletion.cc
index 011b005dfd4acf0049e89651b4de4826527f649b..44ad7b72fdb01c14c093d8c5d82376408474513f 100644 (file)
--- a/src/librbd/io/AioCompletion.cc
+++ b/src/librbd/io/AioCompletion.cc
@@ -99,11 +99,15 @@ void AioCompletion::complete() {
 
   state = AIO_STATE_CALLBACK;
   if (complete_cb) {
-    complete_cb(rbd_comp, complete_arg);
+    if (external_callback) {
+      complete_external_callback();
+    } else {
+      complete_cb(rbd_comp, complete_arg);
+    }
   }
 
   if (ictx != nullptr && event_notify && ictx->event_socket.is_valid()) {
-    ictx->completed_reqs.push(this);
+    ictx->event_socket_completions.push(this);
     ictx->event_socket.notify();
   }
   state = AIO_STATE_COMPLETE;
@@ -242,5 +246,30 @@ ssize_t AioCompletion::get_return_value() {
   return r;
 }
 
+void AioCompletion::complete_external_callback() {
+  // ensure librbd external users never experience concurrent callbacks
+  // from multiple librbd-internal threads.
+  ictx->external_callback_completions.push(this);
+
+  while (true) {
+    if (ictx->external_callback_in_progress.exchange(true)) {
+      // another thread is concurrently invoking external callbacks
+      break;
+    }
+
+    AioCompletion* aio_comp;
+    while (ictx->external_callback_completions.pop(aio_comp)) {
+      aio_comp->complete_cb(aio_comp->rbd_comp, aio_comp->complete_arg);
+    }
+
+    ictx->external_callback_in_progress.store(false);
+    if (ictx->external_callback_completions.empty()) {
+      // queue still empty implies we didn't have a race between the last failed
+      // pop and resetting the in-progress state
+      break;
+    }
+  }
+}
+
 } // namespace io
 } // namespace librbd

diff --git a/src/librbd/io/AioCompletion.h b/src/librbd/io/AioCompletion.h
index 551596a0de83e7c49c93ef39949477089a49d780..19e978edbbcf59e7d9c59cb47e55d6bb4d0731da 100644 (file)
--- a/src/librbd/io/AioCompletion.h
+++ b/src/librbd/io/AioCompletion.h
@@ -70,6 +70,7 @@ struct AioCompletion {
 
   bool event_notify = false;
   bool was_armed = false;
+  bool external_callback = false;
 
   template <typename T, void (T::*MF)(int)>
   static void callback_adapter(completion_t cb, void *arg) {
@@ -176,6 +177,7 @@ struct AioCompletion {
 
 private:
   void queue_complete();
+  void complete_external_callback();
 
 };
 

diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc
index b12404fcaaafc6f497d022993a4b85aef9f8aac4..47f5cd5a7cc8f240f3a725b8b2a918b74deeda00 100644 (file)
--- a/src/librbd/librbd.cc
+++ b/src/librbd/librbd.cc
@@ -1155,8 +1155,10 @@ namespace librbd {
 
   RBD::AioCompletion::AioCompletion(void *cb_arg, callback_t complete_cb)
   {
-    pc = reinterpret_cast<void*>(librbd::io::AioCompletion::create(
-      cb_arg, complete_cb, this));
+    auto aio_comp = librbd::io::AioCompletion::create(
+      cb_arg, complete_cb, this);
+    aio_comp->external_callback = true;
+    pc = reinterpret_cast<void*>(aio_comp);
   }
 
   bool RBD::AioCompletion::is_complete()