librbd: fix LIST_SNAPS_FLAG_WHOLE_OBJECT behavior

Bundling read_whole_object and LIST_SNAPS_FLAG_WHOLE_OBJECT cases
together is wrong:

- In read_whole_object case, calc_snap_set_diff() sets just
  read_whole_object.  Everything else is zeroed out and may require
  resetting to fit with the rest of ObjectListSnapsRequest logic.

- In LIST_SNAPS_FLAG_WHOLE_OBJECT case, only the diff should be
  expanded.  Everything else is set by calc_snap_set_diff() and should
  be used as is.  This goes for end_size in particular -- if it's reset
  to object size, bogus zero extents may be returned as the object
  would appear to have grown.

This is a regression introduced in commit 4429ed4f3f4c ("librbd: switch
diff iterate API to use new snaps list dispatch methods") by way of
commit 66dd53d9c4d9 ("librbd: optionally return full object extent for
any snapshot deltas").

Fixes: https://tracker.ceph.com/issues/63654
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

diff --git a/src/librbd/io/ObjectRequest.cc b/src/librbd/io/ObjectRequest.cc
index 4bd5562bd87412e6b5afb1531da3725b612ff5dc..731b439dd2697889a088522034848a29ad42555b 100644 (file)
--- a/src/librbd/io/ObjectRequest.cc
+++ b/src/librbd/io/ObjectRequest.cc
@@ -834,14 +834,16 @@ void ObjectListSnapsRequest<I>::handle_list_snaps(int r) {
                        end_snap_id, &diff, &end_size, &exists,
                        &clone_end_snap_id, &read_whole_object);
 
-    if (read_whole_object ||
-        (!diff.empty() &&
-         ((m_list_snaps_flags & LIST_SNAPS_FLAG_WHOLE_OBJECT) != 0))) {
+    if (read_whole_object) {
       ldout(cct, 1) << "need to read full object" << dendl;
-      diff.clear();
       diff.insert(0, image_ctx->layout.object_size);
       end_size = image_ctx->layout.object_size;
       clone_end_snap_id = end_snap_id;
+    } else if ((m_list_snaps_flags & LIST_SNAPS_FLAG_WHOLE_OBJECT) != 0 &&
+               !diff.empty()) {
+      ldout(cct, 20) << "expanding diff from " << diff << dendl;
+      diff.clear();
+      diff.insert(0, image_ctx->layout.object_size);
     }
 
     if (exists) {

diff --git a/src/test/librbd/io/test_mock_ObjectRequest.cc b/src/test/librbd/io/test_mock_ObjectRequest.cc
index ca660470df489417a9c59d71c5670f7a61e276c7..5706abe313bdb3a1d8c91b834a90943ca715fe39 100644 (file)
--- a/src/test/librbd/io/test_mock_ObjectRequest.cc
+++ b/src/test/librbd/io/test_mock_ObjectRequest.cc
@@ -1984,6 +1984,56 @@ TEST_F(TestMockIoObjectRequest, ListSnapsWholeObject) {
   }
 }
 
+TEST_F(TestMockIoObjectRequest, ListSnapsWholeObjectEndSize) {
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+
+  MockTestImageCtx mock_image_ctx(*ictx);
+  mock_image_ctx.snaps = {3};
+
+  InSequence seq;
+
+  librados::snap_set_t snap_set;
+  snap_set.seq = 3;
+  librados::clone_info_t clone_info;
+
+  clone_info.cloneid = CEPH_NOSNAP;
+  clone_info.snaps = {};
+  clone_info.overlap = {};
+  // smaller than object extent (i.e. the op) to test end_size handling
+  clone_info.size = mock_image_ctx.layout.object_size - 2;
+  snap_set.clones.push_back(clone_info);
+
+  expect_list_snaps(mock_image_ctx, snap_set, 0);
+
+  {
+    SnapshotDelta snapshot_delta;
+    C_SaferCond ctx;
+    auto req = MockObjectListSnapsRequest::create(
+      &mock_image_ctx, 0, {{0, mock_image_ctx.layout.object_size - 1}},
+      {4, CEPH_NOSNAP}, 0, {}, &snapshot_delta, &ctx);
+    req->send();
+    ASSERT_EQ(0, ctx.wait());
+
+    EXPECT_TRUE(snapshot_delta.empty());
+  }
+
+  expect_list_snaps(mock_image_ctx, snap_set, 0);
+
+  {
+    SnapshotDelta snapshot_delta;
+    C_SaferCond ctx;
+    auto req = MockObjectListSnapsRequest::create(
+      &mock_image_ctx, 0, {{0, mock_image_ctx.layout.object_size - 1}},
+      {4, CEPH_NOSNAP}, LIST_SNAPS_FLAG_WHOLE_OBJECT, {}, &snapshot_delta,
+      &ctx);
+    req->send();
+    ASSERT_EQ(0, ctx.wait());
+
+    EXPECT_TRUE(snapshot_delta.empty());
+  }
+}
+
 } // namespace io
 } // namespace librbd