librbd: data corruption on mirroring cloned images #53672

nbalacha · 2023-09-26T11:34:47Z

Parent data is not synced to the non-primary after a copyup operation on the primary image with snapshot based rbd-mirroring. Writing to a non-existent object in an rbd clone image triggers a copyup operation.The new write data and the data from the parent are combined and written to the new clone object and any existing snapshots on the clone are updated with the parent data.
The rbd-mirror daemon uses deep_copy with subsets of snapshots. If a copyup has modified the already synced older snapshot with the parent data, a snapshot diff will only return the new data that was written to the clone. As the deep_copy writes data to the dst object using rados calls instead of rbd apis, a copyup is not performed and the parent data is lost. The fix involves triggering a copyup on the object if the image is a clone. This does not affect other deep_copy users as they copy the image and all the existing snapshots as a whole.

Fixes: https://tracker.ceph.com/issues/61891

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

nbalacha · 2023-09-26T11:36:27Z

I'm working on the following:

This requires that the parent image has been synced before the cloned image sync begins. If both are enabled at the same time and the parent image object has not been created/synced before the trigger_copyup is called on the clone, the read on the parent fails with ENOENT leading to the original data corruption.
Additional unit tests

trociny · 2023-09-26T11:39:01Z

@nbalacha Do we have a tracker ticket for this? We will definitely want to create a ticket in tracker.ceph.com for this issue and link it here.

satoru-takeuchi · 2023-09-26T23:58:16Z

@nbalacha I guess this PR is for the following issue. Is it correct?
https://tracker.ceph.com/issues/61891

In addition, I'm glad if the backport tag in this will be filled.

nbalacha · 2023-09-27T03:54:28Z

@nbalacha Do we have a tracker ticket for this? We will definitely want to create a ticket in tracker.ceph.com for this issue and link it here.

Done.

nbalacha · 2023-09-27T03:55:02Z

@nbalacha I guess this PR is for the following issue. Is it correct? https://tracker.ceph.com/issues/61891

In addition, I'm glad if the backport tag in this will be filled.

Thank you @satoru-takeuchi . I will check about the backport tag.

nbalacha · 2023-10-09T06:03:45Z

Still WIP. The new tests do not succeed atm.

idryomov · 2023-10-09T10:27:13Z

src/librbd/deep_copy/ObjectCopyRequest.cc

+    if (io::util::trigger_copyup(
+      m_dst_image_ctx, m_dst_object_number, io_context, ctx)) {
+      return;
+    }


(Not a review, just something that came up in a conversation about mocking)

This would hang (and leak some memory) if io::util::trigger_copyup() returns false because you just bail from ObjectCopyRequest::trigger_copyup() in that case. If it's "not possible" for io::util::trigger_copyup() to return false here, an assert would be nice.

nbalacha · 2023-11-02T13:08:59Z

@trociny , can you please take a look and let me know if this is the right approach?

trociny

In general the approach seems right to me. There are comments though.

trociny · 2023-11-03T15:12:14Z

src/librbd/deep_copy/ObjectCopyRequest.cc

+    bool r = io::util::trigger_copyup(
+      m_dst_image_ctx, m_dst_object_number, io_context, ctx);
+    if (r == false) {
+      // No parent found


you need to delete ctx manually here otherwise you will leak a memory when trigger_copyup is failed (as Ilya mentioned).

trociny · 2023-11-03T15:16:40Z

src/librbd/deep_copy/ObjectCopyRequest.cc

+      m_dst_image_ctx, m_dst_object_number, io_context, ctx);
+    if (r == false) {
+      // No parent found
+      send_update_object_map();


nit: I would add return here for clarity.

trociny · 2023-11-03T15:19:43Z

src/librbd/deep_copy/ObjectCopyRequest.h

@@ -135,6 +135,9 @@ class ObjectCopyRequest {
  void send_update_object_map();
  void handle_update_object_map(int r);

+  void trigger_copyup();


You also need to update the state diagram above.

trociny · 2023-11-03T15:24:15Z

src/librbd/deep_copy/ObjectCopyRequest.cc

+ldout(m_cct, 20) << "NITHYA: m_src_snap_id_start: " << m_src_snap_id_start << ", parent:" << m_src_image_ctx->parent << dendl;
+  if (m_src_snap_id_start != 0 && m_src_image_ctx->parent != nullptr) {
+    ldout(m_cct, 20) << dendl;
+    auto io_context = m_dst_image_ctx->duplicate_data_io_context();


Why do you need to duplicate data io context instead of just using it?

trociny · 2023-11-03T15:36:41Z

src/librbd/deep_copy/ObjectCopyRequest.cc

+  // For rbd-mirror scenarios, we trigger a copyup on the object and write the parent
+  // data to the object before writing the new clone image data.
+ldout(m_cct, 20) << "NITHYA: m_src_snap_id_start: " << m_src_snap_id_start << ", parent:" << m_src_image_ctx->parent << dendl;
+  if (m_src_snap_id_start != 0 && m_src_image_ctx->parent != nullptr) {


I think you need to check m_dst_image_ctx->parent instead? Also, I think you should be stricter, i.e. you should try to trigger copyup only when the dst_snap_id_start is "attached" to the parent. Otherwise you will trigger copyup on every new mirrored snapshot.

I'm not sure I understand the first point. The trigger_copyup needs to be called only on cloned images hence the check for a non-null src parent. The dst image would be a clone as well as the mirror daemon requires the parent to be mirrored in this scenario.

trigger copyup only when the dst_snap_id_start is "attached" to the parent.
I'll look into this.

But why not to check the destination image for the parent (where we will need to trigger copyup)? Note, I am thinking here about a more generic scenario, not only the current rbd-mirror behavior. E.g. someone did rbd deep-copy with --flatten option (i.e. the destination image will not have a parent), and then later deep-copy only newer snapshots (like mirroring does). Currently we don't provide this feature but someone might want it for rbd deep-copy or some new application.

BTW, rbd-mirror might want this flatten option too, e.g. when the parent image is not mirrored. My point is that you cannot assume that the destination image will always have a parent if the source image has it.

IIUC , the flatten option with mirroring and the incremental snapshot deep copy are possible future features. We are currently not targeting mirroring without the parent images being mirrored.
I would like to work on the current issue where the parent is also required to be mirrored as we have a requirement to get this fixed and address the other scenarios later.

trociny · 2023-11-03T15:41:04Z

src/librbd/deep_copy/ObjectCopyRequest.cc

+  // This causes the rbd-mirror to ignore the parent data and only sync the new write
+  // to the clone, causing data corruption.
+  // For rbd-mirror scenarios, we trigger a copyup on the object and write the parent
+  // data to the object before writing the new clone image data.


nit: I would not use rbd-mirror/mirroring words here, it may be any other application or tool that is doing deep copy for a clone. BTW, I have not checked but I suppose the rbd deep-copy could be also extended to provide similar functionality.

Maybe I can clarify that further in the comment. The issue is seen only with rbd-mirroring as of now. The regular deep copy/migration etc work correctly.

It is not about clarifying. It is about mentioning a specific application (rbd-mirror) in librbd code. Just describe the scenario but don't specify it is "rbd-mirror" scenario. Actually I am not sure we need this comment at all, but I would leave this to Ilya to decide.

I believe this comment is required to help understand what is going on. The lack of comments in the code makes it difficult to figure out why something is being done.
I have rewritten it to remove mention of rbd-mirror, however I personally think we need to have something written up as to which components use these functions and how.

github-actions · 2024-01-22T08:01:48Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

nbalacha · 2024-01-24T10:30:05Z

@trociny , I have run into an issue where user snaps on the clone are not being mirrored correctly. I cannot find anything in the logs to indicate why. Do you have any idea as to what I should be looking at?

The following test fails:

bin/rbd --cluster site-a create -s 8M data/src1
bin/rbd --cluster site-a map -t nbd data/src1

xfs_io -d -c 'pwrite -S 0xadad -b 4M 0 5M' /dev/nbd0

bin/rbd --cluster site-a snap create data/src1@snap1
bin/rbd --cluster site-a snap protect data/src1@snap1
bin/rbd --cluster site-a unmap -t nbd data/src1

bin/rbd --cluster site-a clone data/src1@snap1 data/clone1
bin/rbd --cluster site-a map -t nbd data/clone1


bin/ceph --cluster site-b config set global debug_rbd 20
bin/ceph --cluster site-b config set global debug_rbd_mirror 20

bin/rbd --cluster site-a mirror image enable data/src1 snapshot
sleep 15
bin/rbd --cluster site-a snap create data/clone1@snap1
bin/rbd --cluster site-a mirror image enable data/clone1 snapshot
sleep 5
xfs_io -d -c 'pwrite -S 0x11 -b 4M 7M 1M' /dev/nbd0
xfs_io -d -c 'pread -v 4M 512' /dev/nbd0
xfs_io -d -c 'pread -v 7M 512' /dev/nbd0
bin/rbd --cluster site-a snap create data/clone1@snap2
bin/rbd --cluster site-a mirror image snapshot data/clone1
sleep 10
xfs_io -d -c 'pwrite -S 0x22 -b 4M 5M 2M' /dev/nbd0
bin/rbd --cluster site-a snap create data/clone1@snap3
bin/rbd --cluster site-a mirror image snapshot data/clone1

for i in {1..3}; do bin/rbd --cluster site-a map -t nbd data/clone1@snap$i; bin/rbd --cluster site-b map -t nbd data/clone1@snap$i;diff /dev/nbd0  /dev/nbd1; bin/rbd --cluster site-a unmap -t nbd data/clone1@snap$i; bin/rbd --cluster site-b unmap -t nbd data/clone1@snap$i;done

The actual final images match but snap2 and snap3 do not match on the primary and secondary.
I am working on this in parallel but would like it if this PR could be merged without waiting a fix.

trociny

In general the approach looks right.

Please split the commit into two: librbd and rbd_mirror parts.

trociny · 2024-01-25T08:03:54Z

src/tools/rbd_mirror/image_replayer/CreateImageRequest.h

@@ -120,12 +121,18 @@ class CreateImageRequest {
  void open_remote_parent_image();
  void handle_open_remote_parent_image(int r);

+  void open_local_parent_image();
+  void handle_open_local_parent_image(int r);


nit: you also need to update the diagram above to include the new states.

trociny · 2024-01-25T08:15:30Z

src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc

@@ -275,6 +307,46 @@ void CreateImageRequest<I>::clone_image() {
    close_remote_parent_image();
    return;
  }
+  if (m_mirror_image_mode == cls::rbd::MIRROR_IMAGE_MODE_SNAPSHOT) {


I would prefer if you did this check in handle_open_local_parent_image. This way it would be more clear why we needed to open the local parent image.

This would also mean that the checks above should be moved to handle_open_remote_parent_image and member variables m_snap_name and m_snap_namespace introduced to store the result.

Ideally, I think I would like we introduce image_replayer::ValidateSnapshot request (or something like this) that would call image_replayer::snapshot::ValidateSnapshot for the snapshot based mirroring, where this code could be moved. And for the journal based mirroring, it could be noop, or it could also have ``image_replayer::journal::ValidateSnapshot` that would just check the image snapshot exists. But I will not insist on this.

@idryomov and I did discuss having a separate snapshot/journal class implementations but thought this approach would be ok.

I'll look into moving the checks into the open functions.

trociny · 2024-01-25T08:27:47Z

src/librbd/deep_copy/ObjectCopyRequest.cc

+  // This causes the rbd-mirror to ignore the parent data and only sync the new write
+  // to the clone, causing data corruption.
+  // For rbd-mirror scenarios, we trigger a copyup on the object and write the parent
+  // data to the object before writing the new clone image data.


It is not about clarifying. It is about mentioning a specific application (rbd-mirror) in librbd code. Just describe the scenario but don't specify it is "rbd-mirror" scenario. Actually I am not sure we need this comment at all, but I would leave this to Ilya to decide.

trociny · 2024-01-25T08:28:24Z

src/librbd/io/ObjectRequest.cc

+      // object, the older snaps will be updated with the parent data.
+      // When called by snapshot based rbd-mirror, the diff in the older snap is ignored
+      // as it was processed earlier, causing the snapshot_delta to not include
+      // the parent data.


I am not sure we need this comment here.

I think it helps explain what happens - it took me a long time to figure this out.

trociny · 2024-01-30T15:41:15Z

Please split the commit into two: librbd and rbd_mirror parts.

@nbalacha what about this?

nbalacha · 2024-02-09T08:11:59Z

Please split the commit into two: librbd and rbd_mirror parts.

@nbalacha what about this?

Sorry. Missed that earlier. Updated now.

trociny · 2024-02-13T11:27:47Z

@nbalacha jenkins build failed:

In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc:19:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/librbd/io/Utils.h:54:6: error: function 'librbd::io::util::trigger_copyup<librbd::(anonymous namespace)::MockTestImageCtx>' is used but not defined in this translation unit, and cannot be defined in any other translation unit because its type does not have linkage
bool trigger_copyup(ImageCtxT *image_ctx, uint64_t object_no,
     ^
/home/jenkins-build/build/workspace/ceph-pull-requests/src/librbd/deep_copy/ObjectCopyRequest.cc:854:24: note: used here
    bool r = io::util::trigger_copyup(m_dst_image_ctx, m_dst_object_number,
                       ^
1 error generated.

Parent data is not synced to the non-primary with snapshot based rbd-mirroring when the primary clone image is written to. The rbd-mirror daemon uses deep_copy with subsets of snapshots. If a copyup operation on the clone image modifies the already synced older snapshot with the parent data, a snapshot diff will only return the new data that was written to the clone. The fix checks that the parent image snap from which the clone was created has been synced before creating the non-primary clone image. Fixes: https://tracker.ceph.com/issues/61891 Signed-off-by: N Balachandran <nibalach@redhat.com>

Parent data is not synced to the non-primary after a copyup operation on the primary image with snapshot based rbd-mirroring. Writing to a non-existent object in an rbd clone image triggers a copyup operation.The new write data and the data from the parent are combined and written to the new clone object and any existing snapshots on the clone are updated with the parent data. The rbd-mirror daemon uses deep_copy with subsets of snapshots. If a copyup has modified the already synced older snapshot with the parent data, a snapshot diff will only return the new data that was written to the clone. As the deep_copy writes data to the dst object using rados calls instead of rbd apis, a copyup is not performed and the parent data is lost. The fix involves triggering a copyup on the object if the image is a clone. This does not affect other deep_copy users as they copy the image and all the existing snapshots as a whole. Fixes: https://tracker.ceph.com/issues/61891 Signed-off-by: N Balachandran <nibalach@redhat.com>

These tests do not pass at the moment but are required in order to get the build to succeed. Signed-off-by: N Balachandran <nibalach@redhat.com>

nbalacha · 2024-02-15T11:34:51Z

@nbalacha jenkins build failed:

In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/test/librbd/deep_copy/test_mock_ObjectCopyRequest.cc:19:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/librbd/io/Utils.h:54:6: error: function 'librbd::io::util::trigger_copyup<librbd::(anonymous namespace)::MockTestImageCtx>' is used but not defined in this translation unit, and cannot be defined in any other translation unit because its type does not have linkage
bool trigger_copyup(ImageCtxT *image_ctx, uint64_t object_no,
     ^
/home/jenkins-build/build/workspace/ceph-pull-requests/src/librbd/deep_copy/ObjectCopyRequest.cc:854:24: note: used here
    bool r = io::util::trigger_copyup(m_dst_image_ctx, m_dst_object_number,
                       ^
1 error generated.

Done. The tests still fail though. @trociny , any idea as to what might be causing #53672 (comment) ?

trociny · 2024-02-16T08:12:24Z

@nbalacha Why did you close the PR? Was it accidental?

trociny · 2024-02-16T08:13:51Z

The tests still fail though. @trociny , any idea as to what might be causing #53672 (comment) ?

Have you investigated what is exactly different in the snapshots that do not match?

nbalacha · 2024-02-16T14:29:05Z

I renamed the branch yesterday - looks like it has assumed it was deleted.
I shall create a new PR.

nbalacha requested a review from a team as a code owner September 26, 2023 11:34

nbalacha marked this pull request as draft September 26, 2023 11:34

github-actions bot added the rbd label Sep 26, 2023

nbalacha force-pushed the tracker-61891-2 branch from 072df44 to 806284f Compare September 27, 2023 03:53

github-actions bot added the tests label Oct 9, 2023

idryomov reviewed Oct 9, 2023

View reviewed changes

nbalacha force-pushed the tracker-61891-2 branch from e85f453 to 3dd9f10 Compare October 12, 2023 08:28

nbalacha force-pushed the tracker-61891-2 branch 3 times, most recently from c9eaf25 to f17f0a6 Compare October 25, 2023 11:34

nbalacha assigned trociny Nov 2, 2023

nbalacha requested a review from trociny November 2, 2023 13:08

nbalacha unassigned trociny Nov 2, 2023

trociny reviewed Nov 3, 2023

View reviewed changes

github-actions bot added the stale label Jan 22, 2024

idryomov removed the stale label Jan 22, 2024

nbalacha force-pushed the tracker-61891-2 branch from f17f0a6 to ccd3e7d Compare January 24, 2024 10:23

nbalacha marked this pull request as ready for review January 24, 2024 10:24

nbalacha requested review from idryomov and trociny January 25, 2024 03:56

trociny reviewed Jan 25, 2024

View reviewed changes

nbalacha force-pushed the tracker-61891-2 branch from ccd3e7d to 6f72642 Compare January 30, 2024 15:26

nbalacha requested a review from trociny January 30, 2024 15:26

nbalacha force-pushed the tracker-61891-2 branch from 6f72642 to dc5389c Compare February 9, 2024 08:10

nbalacha added 3 commits February 15, 2024 16:12

tests: add mock test for librbd

e4ca4bd

These tests do not pass at the moment but are required in order to get the build to succeed. Signed-off-by: N Balachandran <nibalach@redhat.com>

nbalacha force-pushed the tracker-61891-2 branch from 42c0f9f to e4ca4bd Compare February 15, 2024 10:43

nbalacha closed this Feb 15, 2024

nbalacha deleted the tracker-61891-2 branch February 15, 2024 11:36

librbd: data corruption on mirroring cloned images #53672

librbd: data corruption on mirroring cloned images #53672

Conversation

nbalacha commented Sep 26, 2023 • edited

Contribution Guidelines

Checklist

nbalacha commented Sep 26, 2023 • edited

trociny commented Sep 26, 2023

satoru-takeuchi commented Sep 26, 2023

nbalacha commented Sep 27, 2023

nbalacha commented Sep 27, 2023

nbalacha commented Oct 9, 2023

Choose a reason for hiding this comment

nbalacha commented Nov 2, 2023

trociny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbalacha Jan 26, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Jan 22, 2024

nbalacha commented Jan 24, 2024 • edited

trociny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trociny commented Jan 30, 2024

nbalacha commented Feb 9, 2024

trociny commented Feb 13, 2024

nbalacha commented Feb 15, 2024

trociny commented Feb 16, 2024

trociny commented Feb 16, 2024

nbalacha commented Feb 16, 2024

nbalacha commented Sep 26, 2023 •

edited

nbalacha commented Sep 26, 2023 •

edited

nbalacha Jan 26, 2024 •

edited

nbalacha commented Jan 24, 2024 •

edited