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

os/bluestore: refactor dirty blob tracking along with some related fixes #10215

Merged
merged 4 commits into from Aug 11, 2016

Conversation

ifed01
Copy link
Contributor

@ifed01 ifed01 commented Jul 8, 2016

This is an intermediate patch before introducing refcounting for Blob instances.

Signed-off-by: Igor Fedotov ifedotov@mirantis.com

@ifed01 ifed01 changed the title os/bluestore: fix no Blob instance release os/bluestore: fix no Blob instances release Jul 8, 2016
@ifed01 ifed01 force-pushed the wip-bluestore-blob-release-fix branch from 2d29f9a to 0e2390e Compare July 11, 2016 13:36
@ifed01 ifed01 changed the title os/bluestore: fix no Blob instances release os/bluestore: refactor dirty blob tracking along with some related fixes Jul 11, 2016
@ifed01 ifed01 force-pushed the wip-bluestore-blob-release-fix branch from 0e2390e to 75348da Compare July 18, 2016 16:24
@markhpc
Copy link
Member

markhpc commented Jul 21, 2016

retest this please

@markhpc
Copy link
Member

markhpc commented Jul 21, 2016

Note: The above comment was to get jenkins to retest the build.

@markhpc
Copy link
Member

markhpc commented Jul 28, 2016

Hi Igor, this branch has conflicts. Can you please fix? Thanks!

@ifed01
Copy link
Contributor Author

ifed01 commented Jul 28, 2016

@markhpc - rebased

@markhpc
Copy link
Member

markhpc commented Aug 9, 2016

retest this please

int64_t id = get_new_id();
Blob *b = new Blob(id, c);
b->get();
Copy link
Member

Choose a reason for hiding this comment

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

If BlobRef is an intrusive_ptr there shouldn't be any explicit get/put calls anywhere (except the intrustive ptr hooks)...

Copy link
Contributor Author

@ifed01 ifed01 Aug 10, 2016

Choose a reason for hiding this comment

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

@liewegas. Additional ref_count increment is needed since Blobs are stored in intrusive::set blob_map. Blob has to persist while it's stored in this blob_map even if no corresponding BlobRefs are present.
Other options are either using std::set or intrusive::set but IMHO they both have some performance/memory footprint drawbacks. Have I missed something and there is better approach?

Copy link
Member

Choose a reason for hiding this comment

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

@liewegas
Copy link
Member

maybe squash c8a765c and 6d6d940 since the latter replaces all of the former's code anyway?

@liewegas
Copy link
Member

otherwise lgtm!

Igor Fedotov added 4 commits August 11, 2016 14:28
Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>
Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>
Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>
… onode::blob_map enumeration

Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>
@ifed01 ifed01 force-pushed the wip-bluestore-blob-release-fix branch from e7162b8 to 6beade7 Compare August 11, 2016 13:47
@ifed01
Copy link
Contributor Author

ifed01 commented Aug 11, 2016

@liewegas squashed and rebased.

@liewegas liewegas merged commit 3111eb6 into ceph:master Aug 11, 2016
@ifed01 ifed01 deleted the wip-bluestore-blob-release-fix branch August 11, 2016 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants