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: After write try merge bh. #11545
Conversation
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 can see how merging these buffers is a good thing, but how does that fix the bug? It seems like the code should behave even if buffers aren't merged (merging is really just an optimization, not a correctness thing).
@@ -182,6 +182,7 @@ void ObjectCacher::Object::merge_left(BufferHead *left, BufferHead *right) | |||
|
|||
// hose right | |||
delete right; | |||
right = nullptr; //see bh_write_commit |
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.
Isn't this a local variable?
while (!hit.empty()) { | ||
BufferHead *bh = hit.front(); | ||
hit.pop_front(); | ||
if (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.
bh should always be non-null?
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.
#11262 . In hit, the first may merged w/ the second. So i need know which be merged(delete). Yes as you said, set the local var to nullptr after delete don't indicate the related var in hit. But change the merge_left right to BufferHead **right. I think it's not a goog method.
a0970cb
to
c80492d
Compare
@liewegas . I think this version can work. |
|
||
while (!hit.empty()) { | ||
pair<loff_t, BufferHead*> p = hit.front(); | ||
hit.erase(hit.begin()); |
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.
It would be more efficient to just iterate over the vector (for (auto &p : hit) { ... }) without erasing the front element (and shifting everything to the left).
Can you remove the Fixes line from the commit? It make me think this fixes a bug but it doesn't--it just optimizes in a non-buggy way :) Thanks! |
c80492d
to
8aaa854
Compare
Signed-off-by: Jianpeng Ma <jianpeng.ma@intel.com>
Fixes:http://tracker.ceph.com/issues/17270
Signed-off-by: Jianpeng Ma jianpeng.ma@intel.com