librbd: fix deep copy a child-image #20099

Songweibin · 2018-01-24T11:25:01Z

root@s222:/ceph-dev/build# rbd snap ls child
SNAPID NAME     SIZE TIMESTAMP                
     5 s1   10240 kB Wed Jan 23 13:47:58 2018 
root@s222:/ceph-dev/build# rbd deep cp child child-cp
2018-01-24 09:54:12.895 7f4953fff700 -1 librbd::deep_copy::SetHeadRequest: 0x7f49935464c0 handle_set_parent: failed to set parent: (22) Invalid argument
2018-01-24 09:54:12.895 7f4953fff700 -1 librbd::deep_copy::SnapshotCreateRequest: 0x7f4993545a10 handle_set_head: failed to set head: (22) Invalid argument
2018-01-24 09:54:12.895 7f4953fff700 -1 librbd::deep_copy::SnapshotCopyRequest: 0x7f4993545f70 handle_snap_create: failed to create snapshot 's1': (22) Invalid argument
2018-01-24 09:54:12.895 7f4953fff700 -1 librbd::DeepCopyRequest: 0x7f4993545cf0 handle_copy_snapshots: failed to copy snapshot metadata: (22) Invalid argument
Image deep copy: 0% complete...failed.
rbd: deep copy failed: (22) Invalid argument

This pull request (1)fix failed to set parent when deep copying a child-image which has snapshot(s) (2)update rbd_children object when deep copying a child-image.

I'm not sure if this is the right way to fix these issues, and the unittest still needs to be fixed.
@dillaman @trociny Please help to have a look, thank you in advance.

dillaman · 2018-01-24T16:09:08Z

@Songweibin Nice catch, thanks. I think librbd::api::Image::deep_copy might need to be tweaked to create a cloned image when the source image is a clone (or at least one of its snapshots is a clone). That would help to consolidate all the child image tracking logic to a single place (especially since I am working on adding a new v2 version of cloning with all new child tracking). Here is an example of how rbd-mirror handles it [1].

Thinking about this some more, I did open a new feature tracker ticket to add optional support to "flatten" an image upon deep-copy, but that would require tweaking the object copy state machine to read from the parent if no snapshots and the object is missing.

[1] https://github.com/ceph/ceph/blob/master/src/tools/rbd_mirror/image_replayer/CreateImageRequest.cc#L317

trociny · 2018-01-25T06:03:07Z

I think librbd::api::Image::deep_copy might need to be tweaked to create a cloned image when the source image is a clone (or at least one of its snapshots is a clone). That would help to consolidate all the child image tracking logic to a single place (especially since I am working on adding a new v2 version of cloning with all new child tracking). Here is an example of how rbd-mirror handles it [1].

In deep_copy case we are allowed to copy from the head. Should we create a (temporal) snapshot on the source in this case before cloning? This might be weird and do not work when copying from the ro pool?

dillaman · 2018-01-25T12:06:11Z

@trociny In this case, the source image is already a clone so we know the parent snapshot already exists and is protected, so we should be OK to chain a second clone off the parent.

Songweibin · 2018-01-26T03:33:07Z

@dillaman Done, please have a look.
Thanks for your nice suggestion.

dillaman · 2018-01-26T15:52:51Z

src/librbd/api/Image.cc

@@ -158,7 +160,50 @@ int Image<I>::deep_copy(I *src, librados::IoCtx& dest_md_ctx,
    return -ENOSYS;
  }

-  int r = create(dest_md_ctx, destname, "", src_size, opts, "", "", false);
+  int r;


You will need to do something like this to determine if the source image is linked to a parent image:

{ RWLock::RLocker snap_locker(src->snap_lock); RWLock::RLocker parent_locker(src->parent_lock); // use oldest snapshot or HEAD for parent spec if (!src->snap_info.empty()) { parent_spec = src->snap_info.begin()->second.parent.spec; } else { parent_spec = src->parent_md.spec; } }

dillaman · 2018-01-26T15:56:48Z

src/librbd/api/Image.cc

+  } else {
+    std::string snap_name;
+    src->parent->snap_lock.get_read();
+    r = src->parent->get_snap_name(src->parent_md.spec.snap_id, &snap_name);


Nit: src->parent might be nullptr so resolve the snap name after opening the parent image below and then just run src_parent_image_ctx->state->snap_set(XYZ). In a perfect world we should be able to internally set the snap by id.

dillaman · 2018-01-26T16:00:11Z

src/librbd/api/Image.cc

@@ -158,7 +160,50 @@ int Image<I>::deep_copy(I *src, librados::IoCtx& dest_md_ctx,
    return -ENOSYS;
  }

-  int r = create(dest_md_ctx, destname, "", src_size, opts, "", "", false);
+  int r;
+  if (src->parent_md.spec.pool_id == -1) {


Nit: use cached parent_spec from code snippet above throughout

dillaman · 2018-01-26T16:01:03Z

src/librbd/api/Image.cc

+    ImageCtx *src_parent_image_ctx =
+      new ImageCtx("", src->parent_md.spec.image_id,
+                   snap_name.c_str(), parent_io_ctx, true);
+    r = src_parent_image_ctx->state->open(false);


Nit: you can pass true here to skip opening the parent of the parent

Songweibin · 2018-01-29T10:06:41Z

@dillaman Updated, thank you very much.
One more thing, if deep cp a resized clone with object-map enabled, we need to rebuild object-map manually.

root@s222:/ceph-dev/build# rbd resize child -s 100
Resizing image: 100% complete...done.
root@s222:/ceph-dev/build# rbd deep cp child child-cp2
Image deep copy: 100% complete...2018-01-29 18:02:03.586 7f71a4ff9700 -1 librbd::object_map::RefreshRequest: object map smaller than current object count: 3 != 25
2018-01-29 18:02:03.586 7f71a4ff9700 -1 librbd::object_map::InvalidateRequest: 0x7f718c013620 invalidating object map in-memory
2018-01-29 18:02:03.586 7f71a4ff9700 -1 librbd::object_map::InvalidateRequest: 0x7f718c013620 invalidating object map on-disk
2018-01-29 18:02:03.598 7f71a4ff9700 -1 librbd::object_map::InvalidateRequest: 0x7f718c013620 should_complete: r=0
Image deep copy: 100% complete...done.

dillaman · 2018-01-29T17:07:25Z

src/librbd/internal.cc

@@ -1989,6 +1989,39 @@ bool compare_by_name(const child_info_t& c1, const child_info_t& c2)
    return 0;
  }

+  int snap_set(ImageCtx *ictx, const cls::rbd::SnapshotNamespace &snap_namespace,


Not needed -- what I was implying was to add a new overload to ImageState::snap_set that just takes the snap_id, but it doesn't need to be part of this PR. In general, I am trying to (slowly) remove code from internal.cc.

Songweibin · 2018-01-30T12:22:55Z

@dillaman Updated and add tests, thanks.

dillaman · 2018-01-30T19:04:23Z

src/librbd/api/Image.cc

+    {
+      RWLock::RLocker parent_snap_locker(src_parent_image_ctx->snap_lock);
+      auto it = src_parent_image_ctx->snap_info.find(parent_spec.snap_id);
+      if (it != src_parent_image_ctx->snap_info.end()) {


Return -ENOENT if the snapshot doesn't exist -- an empty snap_name implies setting the snapshot to the HEAD revision.

@dillaman Done, thank you.

Nit: can you tweak it to the following for clarity:

if (it == src_parent_image_ctx->snap_info.end()) { return -ENOENT; } snap_name = it->second.name;

@dillaman Done, thanks.

dillaman · 2018-01-30T20:42:37Z

qa/workunits/rbd/cli_generic.sh

+    # deep copy clone-image
+    rbd snap rm testimg4@snap1
+    rbd rm testimg4
+    rbd rm testimg5


Nit: the test fails here since testimg5 has a snapshot named snap1

Oh sorry, i mistakenly thought rbd deep copy testimg1 --snap=snap1 testimg5 only copied snapshot just like copy.

* tweak create a cloned image when the source image is a clone (or at least one of its snapshots is a clone). Signed-off-by: songweibin <song.weibin@zte.com.cn>

batrick added bug-fix rbd DNM labels Jan 24, 2018

Songweibin force-pushed the wip-fix-deep-cp branch 2 times, most recently from 046671c to 8987656 Compare January 26, 2018 03:31

dillaman reviewed Jan 26, 2018

View reviewed changes

Songweibin force-pushed the wip-fix-deep-cp branch from 8987656 to 9d1e247 Compare January 29, 2018 06:54

xiexingguo changed the title ~~[Wip] librbd: fix deep copy a child-image~~ librbd: fix deep copy a child-image Jan 29, 2018

xiexingguo removed the DNM label Jan 29, 2018

dillaman reviewed Jan 29, 2018

View reviewed changes

Songweibin force-pushed the wip-fix-deep-cp branch 2 times, most recently from ccf6f59 to 37009a7 Compare January 30, 2018 12:21

dillaman reviewed Jan 30, 2018

View reviewed changes

Songweibin force-pushed the wip-fix-deep-cp branch from 37009a7 to 2161644 Compare January 31, 2018 00:53

librbd: fix deep copy a child-image

c3e119d

* tweak create a cloned image when the source image is a clone (or at least one of its snapshots is a clone). Signed-off-by: songweibin <song.weibin@zte.com.cn>

Songweibin force-pushed the wip-fix-deep-cp branch from 2161644 to c3e119d Compare February 1, 2018 00:21

dillaman approved these changes Feb 1, 2018

View reviewed changes

dillaman added needs-qa wip-jason-testing labels Feb 1, 2018

dillaman merged commit 4b69976 into ceph:master Feb 2, 2018

Songweibin deleted the wip-fix-deep-cp branch February 5, 2018 01:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

librbd: fix deep copy a child-image #20099

librbd: fix deep copy a child-image #20099

Songweibin commented Jan 24, 2018

dillaman commented Jan 24, 2018

trociny commented Jan 25, 2018

dillaman commented Jan 25, 2018

Songweibin commented Jan 26, 2018

dillaman Jan 26, 2018

dillaman Jan 26, 2018

dillaman Jan 26, 2018

dillaman Jan 26, 2018

Songweibin commented Jan 29, 2018

dillaman Jan 29, 2018

Songweibin commented Jan 30, 2018

dillaman Jan 30, 2018

Songweibin Jan 31, 2018

dillaman Jan 31, 2018

Songweibin Feb 1, 2018

dillaman Jan 30, 2018

Songweibin Jan 31, 2018 •

edited

librbd: fix deep copy a child-image #20099

librbd: fix deep copy a child-image #20099

Conversation

Songweibin commented Jan 24, 2018

dillaman commented Jan 24, 2018

trociny commented Jan 25, 2018

dillaman commented Jan 25, 2018

Songweibin commented Jan 26, 2018

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

Songweibin commented Jan 29, 2018

Choose a reason for hiding this comment

Songweibin commented Jan 30, 2018

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

Songweibin Jan 31, 2018 • edited

Choose a reason for hiding this comment

Songweibin Jan 31, 2018 •

edited