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
osdc: reduce ObjectCacher's memory fragments #24297
Conversation
|
|
||
| uint64_t total = 0; | ||
| const raw *last = nullptr; | ||
| for (const auto r : raw_vec) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please note, if multiple ptrs are sharing the same raw buffer and they overlap with each other, we will underestimate the wasted space. as the length() takes the overlapped spaces into consideration. probably this does not happen in the usecase of ObjectCacher, it might be worthy to mention as a comment in this method though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be better to run maybe_rebuild_buffer in the split method since it would catch more waste corner cases.
|
you mean ObjectCacher::Object::split? that function split bufferlist, but the new bufferlists share the raw buffer |
|
@ukernel That's true, but the reason the split occurs is because one bufferlist is about to be (1) overwritten, (2) truncated, or (3) discarded. However, it's the piece that's not overwritten but is instead just split that concerns me in terms of wasted space and that piece could be denoted to For example, in RBD, a user can write 4 MiB at offset 0 and then later write 4 MiB at offset 512. The BTW -- you can probably tag http://tracker.ceph.com/issues/20054 to this as well assuming it addresses the case above. |
|
updated |
src/osdc/ObjectCacher.cc
Outdated
| @@ -454,15 +468,18 @@ ObjectCacher::BufferHead *ObjectCacher::Object::map_write(ObjectExtent &ex, | |||
| if (cur + max >= bh->end()) { | |||
| // we want right bit (one splice) | |||
| final = split(bh, cur); // just split it, take right half. | |||
| maybe_rebuild_buffer(bh); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: mismatched indentation in this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. new code use tab. old code uses all whitespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ukernel Can we keep it consistent?
Fixes: http://tracker.ceph.com/issues/36192 Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
@dillaman If you're happy with it then I am. |
|
@dillaman fine by me. |
I'm probably missing something simple, but how does it break the ABI/API? |
|
sorry! no, it does not. i thought it changes an existing method. but apparently, i was wrong. |
|
Didn't see any failures in the cephfs runs that would block merging this. @tchaikov merge when you're satisfied. |
Fixes: http://tracker.ceph.com/issues/36192
Signed-off-by: "Yan, Zheng" zyan@redhat.com