librbd: moved skip partial discard logic to object request

This allows the journal object dispatch layer to properly complete
skipped extents and commit the associated event.

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

diff --git a/src/librbd/io/ImageRequest.cc b/src/librbd/io/ImageRequest.cc
index ea53f44c566795963ffeec34542b0529e9fa2c01..496b43dcec3741ac54d84e57be1ebebe6c615798 100644 (file)
--- a/src/librbd/io/ImageRequest.cc
+++ b/src/librbd/io/ImageRequest.cc
@@ -276,7 +276,7 @@ void AbstractImageWriteRequest<I>::send_request() {
                   image_ctx.journal->is_journal_appending());
   }
 
-  int ret = prune_object_extents(object_extents);
+  int ret = validate_object_extents(object_extents);
   if (ret < 0) {
     aio_comp->fail(ret);
     return;
@@ -406,28 +406,6 @@ uint64_t ImageDiscardRequest<I>::append_journal_event(bool synchronous) {
   return tid;
 }
 
-template <typename I>
-int ImageDiscardRequest<I>::prune_object_extents(ObjectExtents &object_extents) {
-  I &image_ctx = this->m_image_ctx;
-  CephContext *cct = image_ctx.cct;
-  if (!this->m_skip_partial_discard) {
-    return 0;
-  }
-
-  for (auto p = object_extents.begin(); p != object_extents.end(); ) {
-    if (p->offset + p->length < image_ctx.layout.object_size) {
-      ldout(cct, 20) << "oid " << p->oid << " " << p->offset << "~"
-                    << p->length << " from " << p->buffer_extents
-                    << ": skip partial discard" << dendl;
-      p = object_extents.erase(p);
-    } else {
-      ++p;
-    }
-  }
-
-  return 0;
-}
-
 template <typename I>
 void ImageDiscardRequest<I>::send_image_cache_request() {
   I &image_ctx = this->m_image_ctx;
@@ -447,11 +425,14 @@ ObjectDispatchSpec *ImageDiscardRequest<I>::create_object_request(
     const ObjectExtent &object_extent, const ::SnapContext &snapc,
     uint64_t journal_tid, Context *on_finish) {
   I &image_ctx = this->m_image_ctx;
+  int discard_flags = OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE;
+  if (m_skip_partial_discard) {
+    discard_flags |=  OBJECT_DISCARD_FLAG_SKIP_PARTIAL;
+  }
   auto req = ObjectDispatchSpec::create_discard(
     &image_ctx, OBJECT_DISPATCH_LAYER_NONE, object_extent.oid.name,
     object_extent.objectno, object_extent.offset, object_extent.length, snapc,
-    OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE, journal_tid,
-    this->m_trace, on_finish);
+    discard_flags, journal_tid, this->m_trace, on_finish);
   return req;
 }
 
@@ -663,7 +644,8 @@ void ImageCompareAndWriteRequest<I>::update_stats(size_t length) {
 }
 
 template <typename I>
-int ImageCompareAndWriteRequest<I>::prune_object_extents(ObjectExtents &object_extents) {
+int ImageCompareAndWriteRequest<I>::validate_object_extents(
+    const ObjectExtents &object_extents) const {
   if (object_extents.size() > 1)
     return -EINVAL;
 

diff --git a/src/librbd/io/ImageRequest.h b/src/librbd/io/ImageRequest.h
index 3123ebd9c6df869938e7dc72dead31a61f3e0833..7a934e5bebb103fcc174939d684ea57613fa68eb 100644 (file)
--- a/src/librbd/io/ImageRequest.h
+++ b/src/librbd/io/ImageRequest.h
@@ -139,7 +139,8 @@ protected:
 
   void send_request() override;
 
-  virtual int prune_object_extents(ObjectExtents &object_extents) {
+  virtual int validate_object_extents(
+      const ObjectExtents &object_extents) const {
     return 0;
   }
 
@@ -219,8 +220,6 @@ protected:
     return "aio_discard";
   }
 
-  int prune_object_extents(ObjectExtents &object_extents) override;
-
   void send_image_cache_request() override;
 
   ObjectDispatchSpec *create_object_request(
@@ -335,7 +334,9 @@ protected:
     return "aio_compare_and_write";
   }
 
-  int prune_object_extents(ObjectExtents &object_extents) override;
+  int validate_object_extents(
+      const ObjectExtents &object_extents) const override;
+
 private:
   bufferlist m_cmp_bl;
   bufferlist m_bl;

diff --git a/src/librbd/io/ObjectDispatch.cc b/src/librbd/io/ObjectDispatch.cc
index 31090238ed8a14c459374a452a7b67f1cf691233..85c8b034ec442c9cc29187b53c34958b14da6b58 100644 (file)
--- a/src/librbd/io/ObjectDispatch.cc
+++ b/src/librbd/io/ObjectDispatch.cc
@@ -58,16 +58,10 @@ bool ObjectDispatch<I>::discard(
   auto cct = m_image_ctx->cct;
   ldout(cct, 20) << oid << " " << object_off << "~" << object_len << dendl;
 
-  bool disable_clone_remove = (
-    (discard_flags & OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE) != 0);
-  bool update_object_map = (
-    (discard_flags & OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE) == 0);
-
   *dispatch_result = DISPATCH_RESULT_COMPLETE;
   auto req = new ObjectDiscardRequest<I>(m_image_ctx, oid, object_no,
                                          object_off, object_len, snapc,
-                                         disable_clone_remove,
-                                         update_object_map, parent_trace,
+                                         discard_flags, parent_trace,
                                          on_dispatched);
   req->send();
   return true;

diff --git a/src/librbd/io/ObjectRequest.cc b/src/librbd/io/ObjectRequest.cc
index 320daedb25c667405d7a1f01382a4fdfb0ddcda6..23f38ee6414169ef168bbbe64cad0c1c45261052 100644 (file)
--- a/src/librbd/io/ObjectRequest.cc
+++ b/src/librbd/io/ObjectRequest.cc
@@ -64,14 +64,12 @@ ObjectRequest<I>::create_discard(I *ictx, const std::string &oid,
                                  uint64_t object_no, uint64_t object_off,
                                  uint64_t object_len,
                                  const ::SnapContext &snapc,
-                                 bool disable_clone_remove,
-                                 bool update_object_map,
+                                 int discard_flags,
                                  const ZTracer::Trace &parent_trace,
                                  Context *completion) {
   return new ObjectDiscardRequest<I>(ictx, oid, object_no, object_off,
-                                     object_len, snapc, disable_clone_remove,
-                                     update_object_map, parent_trace,
-                                     completion);
+                                     object_len, snapc, discard_flags,
+                                     parent_trace, completion);
 }
 
 template <typename I>
@@ -624,6 +622,21 @@ void ObjectWriteRequest<I>::add_write_ops(librados::ObjectWriteOperation *wr) {
   wr->set_op_flags2(m_op_flags);
 }
 
+template <typename I>
+void ObjectDiscardRequest<I>::send() {
+  I *image_ctx = this->m_ictx;
+  auto cct = image_ctx->cct;
+  if ((m_discard_flags & OBJECT_DISCARD_FLAG_SKIP_PARTIAL) != 0 &&
+      this->m_object_off + this->m_object_len < image_ctx->layout.object_size) {
+    ldout(cct, 20) << "oid " << this->m_oid << " " << this->m_object_off << "~"
+                  << this->m_object_len << ": skip partial discard" << dendl;
+    this->async_finish(0);
+    return;
+  }
+
+  AbstractObjectWriteRequest<I>::send();
+}
+
 template <typename I>
 void ObjectWriteSameRequest<I>::add_write_ops(
     librados::ObjectWriteOperation *wr) {

diff --git a/src/librbd/io/ObjectRequest.h b/src/librbd/io/ObjectRequest.h
index 9e8f3996c5e3ccc7282955e3beb37d9723a25d86..3dc98b7df2646592006212b9443ba2c1fa4cf1d3 100644 (file)
--- a/src/librbd/io/ObjectRequest.h
+++ b/src/librbd/io/ObjectRequest.h
@@ -40,8 +40,8 @@ public:
   static ObjectRequest* create_discard(
       ImageCtxT *ictx, const std::string &oid, uint64_t object_no,
       uint64_t object_off, uint64_t object_len, const ::SnapContext &snapc,
-      bool disable_clone_remove, bool update_object_map,
-      const ZTracer::Trace &parent_trace, Context *completion);
+      int discard_flags, const ZTracer::Trace &parent_trace,
+      Context *completion);
   static ObjectRequest* create_write_same(
       ImageCtxT *ictx, const std::string &oid, uint64_t object_no,
       uint64_t object_off, uint64_t object_len, ceph::bufferlist&& data,
@@ -294,14 +294,15 @@ public:
   ObjectDiscardRequest(ImageCtxT *ictx, const std::string &oid,
                        uint64_t object_no, uint64_t object_off,
                        uint64_t object_len, const ::SnapContext &snapc,
-                       bool disable_clone_remove, bool update_object_map,
-                       const ZTracer::Trace &parent_trace, Context *completion)
+                       int discard_flags, const ZTracer::Trace &parent_trace,
+                       Context *completion)
     : AbstractObjectWriteRequest<ImageCtxT>(ictx, oid, object_no, object_off,
                                             object_len, snapc, "discard",
                                             parent_trace, completion),
-      m_update_object_map(update_object_map) {
+      m_discard_flags(discard_flags) {
     if (this->m_full_object) {
-      if (disable_clone_remove && this->has_parent()) {
+      if ((m_discard_flags & OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE) != 0 &&
+          this->has_parent()) {
         // need to hide the parent object instead of child object
         m_discard_action = DISCARD_ACTION_REMOVE_TRUNCATE;
         this->m_object_len = 0;
@@ -337,12 +338,15 @@ public:
     return OBJECT_EXISTS;
   }
 
+  void send() override;
+
 protected:
   bool is_no_op_for_nonexistent_object() const override {
     return (!this->has_parent());
   }
   bool is_object_map_update_enabled() const override {
-    return m_update_object_map;
+    return (
+      (m_discard_flags & OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE) == 0);
   }
   bool is_non_existent_post_write_object_map_state() const override {
     return (m_discard_action == DISCARD_ACTION_REMOVE);
@@ -379,7 +383,7 @@ private:
   };
 
   DiscardAction m_discard_action;
-  bool m_update_object_map;
+  int m_discard_flags;
 
 };
 

diff --git a/src/librbd/io/Types.h b/src/librbd/io/Types.h
index b32a5372d66bbc988b1713b4e8cba3f8dc38f741..ac6231709940b72f95dbc11fa16fc7f155ac292e 100644 (file)
--- a/src/librbd/io/Types.h
+++ b/src/librbd/io/Types.h
@@ -52,7 +52,8 @@ enum ObjectDispatchLayer {
 
 enum {
   OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE      = 1UL << 0,
-  OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE = 1UL << 1
+  OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE = 1UL << 1,
+  OBJECT_DISCARD_FLAG_SKIP_PARTIAL              = 1UL << 2
 };
 
 enum {

diff --git a/src/test/librbd/io/test_mock_ImageRequest.cc b/src/test/librbd/io/test_mock_ImageRequest.cc
index c84ca96f7ce89fce1e406e137a7f216d04a0ed59..a785de2dbaa63879dfdc6356bfdfa527b5ebcdae 100644 (file)
--- a/src/test/librbd/io/test_mock_ImageRequest.cc
+++ b/src/test/librbd/io/test_mock_ImageRequest.cc
@@ -129,9 +129,7 @@ TEST_F(TestMockIoImageRequest, AioDiscardJournalAppendDisabled) {
 
   InSequence seq;
   expect_is_journal_appending(mock_journal, false);
-  if (!ictx->skip_partial_discard) {
-    expect_object_request_send(mock_image_ctx, 0);
-  }
+  expect_object_request_send(mock_image_ctx, 0);
 
   C_SaferCond aio_comp_ctx;
   AioCompletion *aio_comp = AioCompletion::create_and_start(

diff --git a/src/test/librbd/io/test_mock_ObjectRequest.cc b/src/test/librbd/io/test_mock_ObjectRequest.cc
index 0958ac2fe635c28892ca29452b6dbfb8f3d0e72b..496af7fb72bec506e5bc084664d83c6a3dd27269 100644 (file)
--- a/src/test/librbd/io/test_mock_ObjectRequest.cc
+++ b/src/test/librbd/io/test_mock_ObjectRequest.cc
@@ -836,8 +836,7 @@ TEST_F(TestMockIoObjectRequest, DiscardRemove) {
   C_SaferCond ctx;
   auto req = MockObjectDiscardRequest::create_discard(
     &mock_image_ctx, ictx->get_object_name(0), 0, 0,
-    mock_image_ctx.get_object_size(), mock_image_ctx.snapc, false, true, {},
-    &ctx);
+    mock_image_ctx.get_object_size(), mock_image_ctx.snapc, 0, {}, &ctx);
   req->send();
   ASSERT_EQ(0, ctx.wait());
 }
@@ -883,8 +882,9 @@ TEST_F(TestMockIoObjectRequest, DiscardRemoveTruncate) {
   C_SaferCond ctx;
   auto req = MockObjectDiscardRequest::create_discard(
     &mock_image_ctx, ictx->get_object_name(0), 0, 0,
-    mock_image_ctx.get_object_size(), mock_image_ctx.snapc, true, true, {},
-    &ctx);
+    mock_image_ctx.get_object_size(), mock_image_ctx.snapc,
+    OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE |
+      OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE, {}, &ctx);
   req->send();
   ASSERT_EQ(0, ctx.wait());
 }
@@ -916,8 +916,7 @@ TEST_F(TestMockIoObjectRequest, DiscardTruncate) {
   C_SaferCond ctx;
   auto req = MockObjectDiscardRequest::create_discard(
     &mock_image_ctx, ictx->get_object_name(0), 0, 1,
-    mock_image_ctx.get_object_size() - 1, mock_image_ctx.snapc, false, true, {},
-    &ctx);
+    mock_image_ctx.get_object_size() - 1, mock_image_ctx.snapc, 0, {}, &ctx);
   req->send();
   ASSERT_EQ(0, ctx.wait());
 }
@@ -949,7 +948,7 @@ TEST_F(TestMockIoObjectRequest, DiscardZero) {
   C_SaferCond ctx;
   auto req = MockObjectDiscardRequest::create_discard(
     &mock_image_ctx, ictx->get_object_name(0), 0, 1, 1, mock_image_ctx.snapc,
-    false, true, {}, &ctx);
+    0, {}, &ctx);
   req->send();
   ASSERT_EQ(0, ctx.wait());
 }
@@ -978,8 +977,9 @@ TEST_F(TestMockIoObjectRequest, DiscardDisableObjectMapUpdate) {
   C_SaferCond ctx;
   auto req = MockObjectDiscardRequest::create_discard(
     &mock_image_ctx, ictx->get_object_name(0), 0, 0,
-    mock_image_ctx.get_object_size(), mock_image_ctx.snapc, true, false, {},
-    &ctx);
+    mock_image_ctx.get_object_size(), mock_image_ctx.snapc,
+    OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE |
+      OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE, {}, &ctx);
   req->send();
   ASSERT_EQ(0, ctx.wait());
 }
@@ -1008,12 +1008,45 @@ TEST_F(TestMockIoObjectRequest, DiscardNoOp) {
   C_SaferCond ctx;
   auto req = MockObjectDiscardRequest::create_discard(
     &mock_image_ctx, ictx->get_object_name(0), 0, 0,
-    mock_image_ctx.get_object_size(), mock_image_ctx.snapc, true, false, {},
+    mock_image_ctx.get_object_size(), mock_image_ctx.snapc,
+    OBJECT_DISCARD_FLAG_DISABLE_CLONE_REMOVE |
+      OBJECT_DISCARD_FLAG_DISABLE_OBJECT_MAP_UPDATE, {},
     &ctx);
   req->send();
   ASSERT_EQ(0, ctx.wait());
 }
 
+TEST_F(TestMockIoObjectRequest, SkipPartialDiscard) {
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+
+  MockTestImageCtx mock_image_ctx(*ictx);
+  expect_get_object_size(mock_image_ctx);
+
+  MockExclusiveLock mock_exclusive_lock;
+  if (ictx->test_features(RBD_FEATURE_EXCLUSIVE_LOCK)) {
+    mock_image_ctx.exclusive_lock = &mock_exclusive_lock;
+    expect_is_lock_owner(mock_exclusive_lock);
+  }
+
+  MockObjectMap mock_object_map;
+  if (ictx->test_features(RBD_FEATURE_OBJECT_MAP)) {
+    mock_image_ctx.object_map = &mock_object_map;
+  }
+
+  expect_op_work_queue(mock_image_ctx);
+
+  InSequence seq;
+  expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 0, 0);
+
+  C_SaferCond ctx;
+  auto req = MockObjectDiscardRequest::create_discard(
+    &mock_image_ctx, ictx->get_object_name(0), 0, 0, 1, mock_image_ctx.snapc,
+    OBJECT_DISCARD_FLAG_SKIP_PARTIAL, {}, &ctx);
+  req->send();
+  ASSERT_EQ(0, ctx.wait());
+}
+
 TEST_F(TestMockIoObjectRequest, WriteSame) {
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));