librbd: fix split() for SparseExtent and SparseBufferlistExtent

SparseExtents and SparseBufferlist are typedefs for interval_map.  In
both cases, split() handler is broken: for the former the extent isn't
actually split and for the latter incorrect bufferlist is attached to
the split extent.

Fortunately, both SnapshotDelta as produced by ObjectListSnapsRequest
and SparseBufferlist used in a couple of places seem to be collections
where only disjoint intervals are inserted and splitting doesn't occur
(at least in the common case).  But still, this is a landmine waiting
for someone to step on it.

Fixes: https://tracker.ceph.com/issues/64423
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
(cherry picked from commit 746cb28449903ceec2fe3ffcfa5e925eb78eb7b2)

diff --git a/src/librbd/io/Types.h b/src/librbd/io/Types.h
index 7c70986c5846aada3c65dbe19a3c70fc8bea8fb1..c11342141d815b9b0375184f69c8306d2e7d0608 100644 (file)
--- a/src/librbd/io/Types.h
+++ b/src/librbd/io/Types.h
@@ -180,8 +180,9 @@ struct SparseExtent {
 std::ostream& operator<<(std::ostream& os, const SparseExtent& state);
 
 struct SparseExtentSplitMerge {
-  SparseExtent split(uint64_t offset, uint64_t length, SparseExtent &se) const {
-    return SparseExtent(se.state, se.length);
+  SparseExtent split(uint64_t offset, uint64_t length,
+                     const SparseExtent& se) const {
+    return SparseExtent(se.state, length);
   }
 
   bool can_merge(const SparseExtent& left, const SparseExtent& right) const {
@@ -232,10 +233,10 @@ struct SparseBufferlistExtent : public SparseExtent {
 
 struct SparseBufferlistExtentSplitMerge {
   SparseBufferlistExtent split(uint64_t offset, uint64_t length,
-                               SparseBufferlistExtent& sbe) const {
+                               const SparseBufferlistExtent& sbe) const {
     ceph::bufferlist bl;
     if (sbe.state == SPARSE_EXTENT_STATE_DATA) {
-      bl.substr_of(bl, offset, length);
+      bl.substr_of(sbe.bl, offset, length);
     }
     return SparseBufferlistExtent(sbe.state, length, std::move(bl));
   }

diff --git a/src/test/librbd/io/test_mock_ObjectRequest.cc b/src/test/librbd/io/test_mock_ObjectRequest.cc
index c20c825018bb1c82761a5bca9b95ddfceaa2e1cb..3073fee64d25e533e37e417a5d3887cd700aa9a0 100644 (file)
--- a/src/test/librbd/io/test_mock_ObjectRequest.cc
+++ b/src/test/librbd/io/test_mock_ObjectRequest.cc
@@ -2070,6 +2070,97 @@ TEST_F(TestMockIoObjectRequest, ListSnapsNoSnapsInSnapSet) {
   EXPECT_EQ(expected_snapshot_delta, snapshot_delta);
 }
 
+TEST(SparseExtents, Split) {
+  SparseExtents extents;
+  extents.insert(50, 100, {SPARSE_EXTENT_STATE_DATA, 100});
+  extents.erase(80, 30);
+  extents.insert(45, 10, {SPARSE_EXTENT_STATE_ZEROED, 10});
+  extents.insert(140, 20, {SPARSE_EXTENT_STATE_DNE, 20});
+  extents.insert(125, 5, {SPARSE_EXTENT_STATE_ZEROED, 5});
+
+  SparseExtents expected_extents = {
+    {45, {10, {SPARSE_EXTENT_STATE_ZEROED, 10}}},
+    {55, {25, {SPARSE_EXTENT_STATE_DATA, 25}}},
+    {110, {15, {SPARSE_EXTENT_STATE_DATA, 15}}},
+    {125, {5, {SPARSE_EXTENT_STATE_ZEROED, 5}}},
+    {130, {10, {SPARSE_EXTENT_STATE_DATA, 10}}},
+    {140, {20, {SPARSE_EXTENT_STATE_DNE, 20}}}
+  };
+  EXPECT_EQ(expected_extents, extents);
+}
+
+TEST(SparseBufferlist, Split) {
+  bufferlist bl;
+  bl.append(std::string(5, '1'));
+  bl.append(std::string(25, '2'));
+  bl.append(std::string(30, '3'));
+  bl.append(std::string(15, '4'));
+  bl.append(std::string(5, '5'));
+  bl.append(std::string(10, '6'));
+  bl.append(std::string(10, '7'));
+  bufferlist expected_bl1;
+  expected_bl1.append(std::string(25, '2'));
+  bufferlist expected_bl2;
+  expected_bl2.append(std::string(15, '4'));
+  bufferlist expected_bl3;
+  expected_bl3.append(std::string(10, '6'));
+
+  SparseBufferlist extents;
+  extents.insert(50, 100, {SPARSE_EXTENT_STATE_DATA, 100, std::move(bl)});
+  extents.erase(80, 30);
+  extents.insert(45, 10, {SPARSE_EXTENT_STATE_ZEROED, 10});
+  extents.insert(140, 20, {SPARSE_EXTENT_STATE_DNE, 20});
+  extents.insert(125, 5, {SPARSE_EXTENT_STATE_ZEROED, 5});
+
+  SparseBufferlist expected_extents = {
+    {45, {10, {SPARSE_EXTENT_STATE_ZEROED, 10}}},
+    {55, {25, {SPARSE_EXTENT_STATE_DATA, 25, std::move(expected_bl1)}}},
+    {110, {15, {SPARSE_EXTENT_STATE_DATA, 15, std::move(expected_bl2)}}},
+    {125, {5, {SPARSE_EXTENT_STATE_ZEROED, 5}}},
+    {130, {10, {SPARSE_EXTENT_STATE_DATA, 10, std::move(expected_bl3)}}},
+    {140, {20, {SPARSE_EXTENT_STATE_DNE, 20}}}
+  };
+  EXPECT_EQ(expected_extents, extents);
+}
+
+TEST(SparseBufferlist, SplitData) {
+  bufferlist bl1;
+  bl1.append(std::string(100, '1'));
+  bufferlist bl2;
+  bl2.append(std::string(15, '2'));
+  bufferlist bl3;
+  bl3.append(std::string(40, '3'));
+  bufferlist bl4;
+  bl4.append(std::string(10, '4'));
+  bufferlist expected_bl1 = bl2;
+  bufferlist expected_bl2;
+  expected_bl2.append(std::string(35, '1'));
+  bufferlist expected_bl3 = bl4;
+  bufferlist expected_bl4;
+  expected_bl4.append(std::string(30, '1'));
+  bufferlist expected_bl5;
+  expected_bl5.append(std::string(5, '3'));
+  bufferlist expected_bl6;
+  expected_bl6.append(std::string(15, '3'));
+
+  SparseBufferlist extents;
+  extents.insert(50, 100, {SPARSE_EXTENT_STATE_DATA, 100, std::move(bl1)});
+  extents.insert(40, 15, {SPARSE_EXTENT_STATE_DATA, 15, std::move(bl2)});
+  extents.insert(130, 40, {SPARSE_EXTENT_STATE_DATA, 40, std::move(bl3)});
+  extents.erase(135, 20);
+  extents.insert(90, 10, {SPARSE_EXTENT_STATE_DATA, 10, std::move(bl4)});
+
+  SparseBufferlist expected_extents = {
+    {40, {15, {SPARSE_EXTENT_STATE_DATA, 15, std::move(expected_bl1)}}},
+    {55, {35, {SPARSE_EXTENT_STATE_DATA, 35, std::move(expected_bl2)}}},
+    {90, {10, {SPARSE_EXTENT_STATE_DATA, 10, std::move(expected_bl3)}}},
+    {100, {30, {SPARSE_EXTENT_STATE_DATA, 30, std::move(expected_bl4)}}},
+    {130, {5, {SPARSE_EXTENT_STATE_DATA, 5, std::move(expected_bl5)}}},
+    {155, {15, {SPARSE_EXTENT_STATE_DATA, 15, std::move(expected_bl6)}}}
+  };
+  EXPECT_EQ(expected_extents, extents);
+}
+
 } // namespace io
 } // namespace librbd