librbd: sparse bufferlist read should return image-based extent map

It's currently returning a buffer-based extent map which is fine under
the existing use-case for copy-up but it does not support more advanced
features that need to know the actual image-extents for any data.

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

diff --git a/src/librbd/io/CopyupRequest.cc b/src/librbd/io/CopyupRequest.cc
index 11e5e3733b951886f77f12406d80dbef230daea8..c03b4d0b7dc699f619946ef466827f6030416b81 100644 (file)
--- a/src/librbd/io/CopyupRequest.cc
+++ b/src/librbd/io/CopyupRequest.cc
@@ -214,6 +214,19 @@ void CopyupRequest<I>::handle_read_from_parent(int r) {
     return;
   }
 
+  // convert the image-extent extent map to object-extents
+  ExtentMap image_extent_map;
+  image_extent_map.swap(m_copyup_extent_map);
+  for (auto [image_offset, image_length] : image_extent_map) {
+    striper::LightweightObjectExtents object_extents;
+    Striper::file_to_extents(
+      cct, &m_image_ctx->layout, image_offset, image_length, 0, 0,
+      &object_extents);
+    for (auto& object_extent : object_extents) {
+      m_copyup_extent_map[object_extent.offset] = object_extent.length;
+    }
+  }
+
   // copyup() will affect snapshots only if parent data is not all
   // zeros.
   if (!m_copyup_is_zero) {

diff --git a/src/librbd/io/ImageRequest.cc b/src/librbd/io/ImageRequest.cc
index f57857c323a56b29eed4bdaaea14cf7aa673b0c0..51f37dce0e8f7f5ebcc2b5ee2cee2f02adc1802f 100644 (file)
--- a/src/librbd/io/ImageRequest.cc
+++ b/src/librbd/io/ImageRequest.cc
@@ -438,12 +438,6 @@ int ImageReadRequest<I>::clip_request() {
     return r;
   }
 
-  uint64_t buffer_length = 0;
-  auto &image_extents = this->m_image_extents;
-  for (auto &image_extent : image_extents) {
-    buffer_length += image_extent.second;
-  }
-  this->m_aio_comp->read_result.set_clip_length(buffer_length);
   return 0;
 }
 
@@ -471,8 +465,11 @@ void ImageReadRequest<I>::send_request() {
     buffer_ofs += extent.second;
   }
 
-  // issue the requests
   AioCompletion *aio_comp = this->m_aio_comp;
+  aio_comp->read_result.set_clip_length(buffer_ofs);
+  aio_comp->read_result.set_image_extents(image_extents);
+
+  // issue the requests
   aio_comp->set_request_count(object_extents.size());
   for (auto &oe : object_extents) {
     ldout(cct, 20) << data_object_name(&image_ctx, oe.object_no) << " "

diff --git a/src/librbd/io/ReadResult.cc b/src/librbd/io/ReadResult.cc
index d9a96293522793330c2fcc08e0c4ee3bb274acbc..3b5913573ed709dc4bd81e7b325100313b94417a 100644 (file)
--- a/src/librbd/io/ReadResult.cc
+++ b/src/librbd/io/ReadResult.cc
@@ -32,6 +32,22 @@ struct ReadResult::SetClipLengthVisitor : public boost::static_visitor<void> {
   }
 };
 
+struct ReadResult::SetImageExtentsVisitor : public boost::static_visitor<void> {
+  Extents image_extents;
+
+  explicit SetImageExtentsVisitor(const Extents& image_extents)
+    : image_extents(image_extents) {
+  }
+
+  void operator()(SparseBufferlist &sbl) const {
+    sbl.image_extents = image_extents;
+  }
+
+  template <typename T>
+  void operator()(T &t) const {
+  }
+};
+
 struct ReadResult::AssembleResultVisitor : public boost::static_visitor<void> {
   CephContext *cct;
   Striper::StripedReadResult &destriper;
@@ -81,10 +97,50 @@ struct ReadResult::AssembleResultVisitor : public boost::static_visitor<void> {
   void operator()(SparseBufferlist &sparse_bufferlist) const {
     sparse_bufferlist.extent_map->clear();
     sparse_bufferlist.bl->clear();
-    destriper.assemble_result(cct, sparse_bufferlist.extent_map,
-                              sparse_bufferlist.bl);
+    auto buffer_extents_length = destriper.assemble_result(
+      cct, sparse_bufferlist.extent_map, sparse_bufferlist.bl);
+
+    ExtentMap buffer_extent_map;
+    buffer_extent_map.swap(*sparse_bufferlist.extent_map);
+    ldout(cct, 20) << "image_extents="
+                   << sparse_bufferlist.image_extents << ", "
+                   << "buffer_extent_map=" << buffer_extent_map << dendl;
+
+    // The extent-map is logically addressed by buffer-extents not image- or
+    // object-extents. Translate this address mapping to image-extent
+    // logical addressing since it's tied to an image-extent read
+    uint64_t buffer_offset = 0;
+    auto bem_it = buffer_extent_map.begin();
+    for (auto [image_offset, image_length] : sparse_bufferlist.image_extents) {
+      while (bem_it != buffer_extent_map.end()) {
+        auto [buffer_extent_offset, buffer_extent_length] = *bem_it;
+
+        if (buffer_offset + image_length <= buffer_extent_offset) {
+          // skip any image extent that is not included in the results
+          break;
+        }
+
+        // current buffer-extent should be within the current image-extent
+        ceph_assert(buffer_offset <= buffer_extent_offset &&
+                    buffer_offset + image_length >=
+                      buffer_extent_offset + buffer_extent_length);
+        auto image_extent_offset =
+          image_offset + (buffer_extent_offset - buffer_offset);
+        ldout(cct, 20) << "mapping buffer extent " << buffer_extent_offset
+                       << "~" << buffer_extent_length << " to image extent "
+                       << image_extent_offset << "~" << buffer_extent_length
+                       << dendl;
+        (*sparse_bufferlist.extent_map)[image_extent_offset] =
+          buffer_extent_length;
+        ++bem_it;
+      }
+
+      buffer_offset += image_length;
+    }
+    ceph_assert(buffer_offset == buffer_extents_length);
+    ceph_assert(bem_it == buffer_extent_map.end());
 
-    ldout(cct, 20) << "moved resulting " << sparse_bufferlist.extent_map->size()
+    ldout(cct, 20) << "moved resulting " << *sparse_bufferlist.extent_map
                    << " extents of total " << sparse_bufferlist.bl->length()
                    << " bytes to bl "
                    << reinterpret_cast<void*>(sparse_bufferlist.bl) << dendl;
@@ -179,6 +235,10 @@ void ReadResult::set_clip_length(size_t length) {
   boost::apply_visitor(SetClipLengthVisitor(length), m_buffer);
 }
 
+void ReadResult::set_image_extents(const Extents& image_extents) {
+  boost::apply_visitor(SetImageExtentsVisitor(image_extents), m_buffer);
+}
+
 void ReadResult::assemble_result(CephContext *cct) {
   boost::apply_visitor(AssembleResultVisitor(cct, m_destriper), m_buffer);
 }

diff --git a/src/librbd/io/ReadResult.h b/src/librbd/io/ReadResult.h
index f4a34a0506bf21c5ff61dcaf6acef2e5bb1f0c1d..ad10f36c1126f1daa811c4cf278f7b55193ac07d 100644 (file)
--- a/src/librbd/io/ReadResult.h
+++ b/src/librbd/io/ReadResult.h
@@ -59,6 +59,8 @@ public:
   ReadResult(std::map<uint64_t, uint64_t> *extent_map, ceph::bufferlist *bl);
 
   void set_clip_length(size_t length);
+  void set_image_extents(const Extents& image_extents);
+
   void assemble_result(CephContext *cct);
 
 private:
@@ -93,6 +95,8 @@ private:
     std::map<uint64_t, uint64_t> *extent_map;
     ceph::bufferlist *bl;
 
+    Extents image_extents;
+
     SparseBufferlist(std::map<uint64_t, uint64_t> *extent_map,
                      ceph::bufferlist *bl)
       : extent_map(extent_map), bl(bl) {
@@ -105,6 +109,7 @@ private:
                          Bufferlist,
                          SparseBufferlist> Buffer;
   struct SetClipLengthVisitor;
+  struct SetImageExtentsVisitor;
   struct AssembleResultVisitor;
 
   Buffer m_buffer;

diff --git a/src/osdc/Striper.cc b/src/osdc/Striper.cc
index d0d2ab47c37e03355758ced48245354d4043e736..e653fe43f9ee908692fef46eec0eb7fc08dc8b12 100644 (file)
--- a/src/osdc/Striper.cc
+++ b/src/osdc/Striper.cc
@@ -507,7 +507,7 @@ void Striper::StripedReadResult::assemble_result(CephContext *cct, char *buffer,
   ceph_assert(curr == 0);
 }
 
-void Striper::StripedReadResult::assemble_result(
+uint64_t Striper::StripedReadResult::assemble_result(
     CephContext *cct, std::map<uint64_t, uint64_t> *extent_map,
     bufferlist *bl)
 {
@@ -521,4 +521,5 @@ void Striper::StripedReadResult::assemble_result(
     }
   }
   partial.clear();
+  return total_intended_len;
 }

diff --git a/src/osdc/Striper.h b/src/osdc/Striper.h
index 9cb48352c4cc8470a3b442ebdb1e4a4792c8f94f..c7822361a4a4cb1cccc7cea4b3aa651198292a44 100644 (file)
--- a/src/osdc/Striper.h
+++ b/src/osdc/Striper.h
@@ -120,9 +120,9 @@
        */
       void assemble_result(CephContext *cct, char *buffer, size_t len);
 
-      void assemble_result(CephContext *cct,
-                           std::map<uint64_t, uint64_t> *extent_map,
-                           ceph::buffer::list *bl);
+      uint64_t assemble_result(CephContext *cct,
+                               std::map<uint64_t, uint64_t> *extent_map,
+                               ceph::buffer::list *bl);
     };
 
   };

diff --git a/src/test/librbd/io/test_mock_CopyupRequest.cc b/src/test/librbd/io/test_mock_CopyupRequest.cc
index 69aaab7c8eed22ee226309f0a589cef3e5b80c47..8f20bba9a484a3856e7639d3e284b2b410389fe7 100644 (file)
--- a/src/test/librbd/io/test_mock_CopyupRequest.cc
+++ b/src/test/librbd/io/test_mock_CopyupRequest.cc
@@ -200,6 +200,7 @@ struct TestMockIoCopyupRequest : public TestMockFixture {
         [&mock_image_ctx, image_extents, data, r](
             AioCompletion* aio_comp, ReadResult* read_result) {
           aio_comp->read_result = std::move(*read_result);
+          aio_comp->read_result.set_image_extents(image_extents);
           aio_comp->set_request_count(1);
           auto ctx = new ReadResult::C_ImageReadRequest(aio_comp,
                                                         image_extents);