Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

librbd: cannot copy all image-metas if we have more than 64 key/value pairs #18328

Merged
merged 2 commits into from Oct 27, 2017

Conversation

PCzhangPC
Copy link

Signed-off-by: PCzhangPC pengcheng.zhang@easystack.cn

@PCzhangPC PCzhangPC force-pushed the cplostmeta branch 2 times, most recently from 30c1b98 to f762627 Compare October 16, 2017 14:46
@PCzhangPC
Copy link
Author

@ceph-jenkins retest this please

@PCzhangPC
Copy link
Author

@dillaman what about this fix for the copy command?

Copy link

@dillaman dillaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a simple test case to test_librbd.cc where you verify that librbd::copy actually copies >64 metadata keys?

@dillaman dillaman changed the title rbd:can not copy all image-metas if we have more than 64 key/value pairs librbd: cannot copy all image-metas if we have more than 64 key/value pairs Oct 16, 2017
@dillaman
Copy link

@PCzhangPC can you open a tracker ticket for this issue as well?

@PCzhangPC
Copy link
Author

@dillaman dillaman added bug-fix and removed feature labels Oct 16, 2017
@PCzhangPC PCzhangPC force-pushed the cplostmeta branch 3 times, most recently from 49c4c97 to 1b126b8 Compare October 19, 2017 08:32
@PCzhangPC
Copy link
Author

@dillaman updated

@@ -3906,14 +3906,40 @@ extern "C" int rbd_metadata_list(rbd_image_t image, const char *start, uint64_t
librbd::ImageCtx *ictx = (librbd::ImageCtx *)image;
tracepoint(librbd, metadata_list_enter, ictx);
map<string, bufferlist> pairs;
int r = librbd::metadata_list(ictx, start, max, &pairs);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you fixing here (since the C++ API wasn't updated)?

std::string value;

while (more_results) {
ASSERT_EQ(0, image2.metadata_list(last_key, 0, &pairs));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to list all 70 keys in this test in one invocation (and below)

Copy link
Author

@PCzhangPC PCzhangPC Oct 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we? we are iterating over metadata items when listing. just like
https://github.com/ceph/ceph/blob/master/src/tools/rbd/action/ImageMeta.cc#L48

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed -- there is no max restriction on the metadata_list librbd API methods. If such a restriction were added, it could be added to librbd::metadata_list in internal.cc so that (1) the OSDs cannot be overloaded,(2) the C and C++ APIs are consistent, and (3) the API-based iteration would only need to be implemented once behind the scene.

Copy link
Author

@PCzhangPC PCzhangPC Oct 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fine. so what about i fix the 'metadata lost' bug in 'internal.cc' to get all metadatas and remove iterations in 'ImageMeta.cc' , 'export.cc' and so on in another pr? If so, image.metadata_list() could get all all key/value pairs at one time and there's no need to use iterations in the test of this pr

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, i misunderstood what you mean. will update soon

Signed-off-by: PCzhangPC <pengcheng.zhang@easystack.cn>
@PCzhangPC PCzhangPC force-pushed the cplostmeta branch 2 times, most recently from 6d36921 to be5ad87 Compare October 20, 2017 09:07
Signed-off-by: PCzhangPC <pengcheng.zhang@easystack.cn>
@PCzhangPC
Copy link
Author

@dillaman updated

Copy link

@dillaman dillaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@yuriw
Copy link
Contributor

yuriw commented Oct 26, 2017

@yuriw yuriw merged commit 77ea42a into ceph:master Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants