librbd: discard should wait for in-flight cache writeback to complete

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(derived from commit 0e04de4bb9c08755bd0915776bd538ce8f7c2b5b)

diff --git a/src/librbd/io/ImageRequest.cc b/src/librbd/io/ImageRequest.cc
index 883bdd758dde02708af3ff9de78f79dc4b1632a8..b75f5aa06c128932ac23a7cf50c3d5cf82c1ad14 100644 (file)
--- a/src/librbd/io/ImageRequest.cc
+++ b/src/librbd/io/ImageRequest.cc
@@ -28,32 +28,75 @@ using util::get_image_ctx;
 namespace {
 
 template <typename ImageCtxT = ImageCtx>
-struct C_DiscardJournalCommit : public Context {
+struct C_DiscardRequest {
   typedef std::vector<ObjectExtent> ObjectExtents;
 
   ImageCtxT &image_ctx;
-  AioCompletion *aio_comp;
+  AioCompletion *aio_completion;
   ObjectExtents object_extents;
 
-  C_DiscardJournalCommit(ImageCtxT &_image_ctx, AioCompletion *_aio_comp,
-                         const ObjectExtents &_object_extents, uint64_t tid)
-    : image_ctx(_image_ctx), aio_comp(_aio_comp),
-      object_extents(_object_extents) {
+  C_DiscardRequest(ImageCtxT &image_ctx, AioCompletion *aio_completion,
+                   const ObjectExtents &object_extents)
+    : image_ctx(image_ctx), aio_completion(aio_completion),
+      object_extents(object_extents) {
+    aio_completion->add_request();
+  }
+
+  void send() {
+    discard_writeback();
+  }
+
+  void discard_writeback() {
+    CephContext *cct = image_ctx.cct;
+    ldout(cct, 20) << "C_DiscardRequest: delaying cache discard until "
+                   << "writeback flushed" << dendl;
+
+    // ensure we aren't holding the cache lock post-write
+    auto ctx = util::create_async_context_callback(
+      image_ctx, util::create_context_callback<
+        C_DiscardRequest<ImageCtxT>,
+        &C_DiscardRequest<ImageCtxT>::handle_discard_writeback>(this));
+
+    // ensure any in-flight writeback is complete before advancing
+    // the discard request
+    Mutex::Locker cache_locker(image_ctx.cache_lock);
+    image_ctx.object_cacher->discard_writeback(image_ctx.object_set,
+                                               object_extents, ctx);
+  }
+
+  void handle_discard_writeback(int r) {
+    CephContext *cct = image_ctx.cct;
+    ldout(cct, 20) << "C_DiscardRequest: writeback flushed: " << r << dendl;
+
+    {
+      Mutex::Locker cache_locker(image_ctx.cache_lock);
+      image_ctx.object_cacher->discard_set(image_ctx.object_set,
+                                           object_extents);
+    }
+    aio_completion->complete_request(r);
+    delete this;
+  }
+};
+
+template <typename ImageCtxT = ImageCtx>
+struct C_DiscardJournalCommit : public Context {
+  ImageCtxT &image_ctx;
+  C_DiscardRequest<ImageCtxT> *discard_request;
+
+  C_DiscardJournalCommit(ImageCtxT &_image_ctx,
+                         C_DiscardRequest<ImageCtxT> *discard_request,
+                         uint64_t tid)
+    : image_ctx(_image_ctx), discard_request(discard_request) {
     CephContext *cct = image_ctx.cct;
     ldout(cct, 20) << "delaying cache discard until journal tid " << tid << " "
                    << "safe" << dendl;
-
-    aio_comp->add_request();
   }
 
   void finish(int r) override {
     CephContext *cct = image_ctx.cct;
     ldout(cct, 20) << "C_DiscardJournalCommit: "
                    << "journal committed: discarding from cache" << dendl;
-
-    Mutex::Locker cache_locker(image_ctx.cache_lock);
-    image_ctx.object_cacher->discard_set(image_ctx.object_set, object_extents);
-    aio_comp->complete_request(r);
+    discard_request->send();
   }
 };
 
@@ -402,7 +445,7 @@ void AbstractImageWriteRequest<I>::send_request() {
   if (!object_extents.empty()) {
     uint64_t journal_tid = 0;
     aio_comp->set_request_count(
-      object_extents.size() + get_object_cache_request_count(journaling));
+      object_extents.size() + get_object_cache_request_count());
 
     ObjectRequests requests;
     send_object_requests(object_extents, snapc,
@@ -598,10 +641,10 @@ int ImageDiscardRequest<I>::prune_object_extents(ObjectExtents &object_extents)
 }
 
 template <typename I>
-uint32_t ImageDiscardRequest<I>::get_object_cache_request_count(bool journaling) const {
-  // extra completion request is required for tracking journal commit
+uint32_t ImageDiscardRequest<I>::get_object_cache_request_count() const {
+  // extra completion request is required for tracking discard
   I &image_ctx = this->m_image_ctx;
-  return (image_ctx.object_cacher != nullptr && journaling ? 1 : 0);
+  return (image_ctx.object_cacher != nullptr ? 1 : 0);
 }
 
 template <typename I>
@@ -622,17 +665,15 @@ template <typename I>
 void ImageDiscardRequest<I>::send_object_cache_requests(
     const ObjectExtents &object_extents, uint64_t journal_tid) {
   I &image_ctx = this->m_image_ctx;
+  auto aio_comp = this->m_aio_comp;
+  auto req = new C_DiscardRequest<I>(image_ctx, aio_comp, object_extents);
   if (journal_tid == 0) {
-    Mutex::Locker cache_locker(image_ctx.cache_lock);
-    image_ctx.object_cacher->discard_set(image_ctx.object_set,
-                                         object_extents);
+    req->send();
   } else {
     // cannot discard from cache until journal has committed
     assert(image_ctx.journal != NULL);
-    AioCompletion *aio_comp = this->m_aio_comp;
     image_ctx.journal->wait_event(
-      journal_tid, new C_DiscardJournalCommit<I>(image_ctx, aio_comp,
-                                                 object_extents, journal_tid));
+      journal_tid, new C_DiscardJournalCommit<I>(image_ctx, req, journal_tid));
   }
 }
 
@@ -881,15 +922,22 @@ uint64_t ImageCompareAndWriteRequest<I>::append_journal_event(
   return tid;
 }
 
+template <typename I>
+uint32_t ImageCompareAndWriteRequest<I>::get_object_cache_request_count() const {
+  // extra completion request is required for tracking discard
+  I &image_ctx = this->m_image_ctx;
+  return (image_ctx.object_cacher != nullptr ? 1 : 0);
+}
+
 template <typename I>
 void ImageCompareAndWriteRequest<I>::send_object_cache_requests(
   const ObjectExtents &object_extents, uint64_t journal_tid) {
   I &image_ctx = this->m_image_ctx;
 
   if (image_ctx.object_cacher != NULL) {
-    Mutex::Locker cache_locker(image_ctx.cache_lock);
-    image_ctx.object_cacher->discard_set(image_ctx.object_set,
-                                         object_extents);
+    auto aio_comp = this->m_aio_comp;
+    auto req = new C_DiscardRequest<I>(image_ctx, aio_comp, object_extents);
+    req->send();
   }
 }
 

diff --git a/src/librbd/io/ImageRequest.h b/src/librbd/io/ImageRequest.h
index 9d707a3f8f5f83967d5de46d33eaff54ab2e9a5c..24239d3e5be7c34cd830738d620d7dead1961152 100644 (file)
--- a/src/librbd/io/ImageRequest.h
+++ b/src/librbd/io/ImageRequest.h
@@ -183,7 +183,7 @@ protected:
   virtual int prune_object_extents(ObjectExtents &object_extents) {
     return 0;
   }
-  virtual uint32_t get_object_cache_request_count(bool journaling) const {
+  virtual uint32_t get_object_cache_request_count() const {
     return 0;
   }
   virtual void send_object_cache_requests(const ObjectExtents &object_extents,
@@ -278,7 +278,7 @@ protected:
 
   void send_image_cache_request() override;
 
-  uint32_t get_object_cache_request_count(bool journaling) const override;
+  uint32_t get_object_cache_request_count() const override;
   void send_object_cache_requests(const ObjectExtents &object_extents,
                                   uint64_t journal_tid) override;
 
@@ -386,6 +386,7 @@ public:
 protected:
   void send_image_cache_request() override;
 
+  uint32_t get_object_cache_request_count() const override;
   void send_object_cache_requests(const ObjectExtents &object_extents,
                                   uint64_t journal_tid) override;
 

diff --git a/src/test/librbd/io/test_mock_ImageRequest.cc b/src/test/librbd/io/test_mock_ImageRequest.cc
index e85ae259cd0210a2ff0c81790fb145ac73438c2d..8460926d19568ea59eb01209f3e7cee931ff7da5 100644 (file)
--- a/src/test/librbd/io/test_mock_ImageRequest.cc
+++ b/src/test/librbd/io/test_mock_ImageRequest.cc
@@ -248,6 +248,8 @@ TEST_F(TestMockIoImageRequest, AioDiscardJournalAppendDisabled) {
   MockJournal mock_journal;
   mock_image_ctx.journal = &mock_journal;
 
+  expect_op_work_queue(mock_image_ctx);
+
   InSequence seq;
   expect_is_journal_appending(mock_journal, false);
   if (!ictx->skip_partial_discard) {
@@ -342,6 +344,8 @@ TEST_F(TestMockIoImageRequest, AioCompareAndWriteJournalAppendDisabled) {
   MockJournal mock_journal;
   mock_image_ctx.journal = &mock_journal;
 
+  expect_op_work_queue(mock_image_ctx);
+
   InSequence seq;
   expect_is_journal_appending(mock_journal, false);
   expect_object_request_send(mock_image_ctx, mock_aio_object_request, 0);