Cleanup #4744
mds: pass around LogSegments via std::shared_ptr
0%
Description
These really ought to be ref-counted in some way to prevent early expiry.
History
#1 Updated by Greg Farnum over 7 years ago
- Category changed from 47 to Code Hygiene
- Component(FS) MDS added
#2 Updated by Patrick Donnelly almost 5 years ago
- Subject changed from Pass around LogSegments via intrusive_ptr or something to mds: pass around LogSegments via std::shared_ptr
- Status changed from New to 12
- Assignee set to servesha dudhgaonkar
- Priority changed from Normal to High
- Target version set to v15.0.0
- Start date deleted (
04/17/2013) - Labels (FS) task(easy), task(intern) added
#3 Updated by Patrick Donnelly almost 5 years ago
- Tracker changed from Feature to Cleanup
- Priority changed from High to Normal
#4 Updated by Patrick Donnelly almost 5 years ago
- Status changed from 12 to New
- Assignee deleted (
servesha dudhgaonkar)
#5 Updated by Patrick Donnelly about 4 years ago
- Target version deleted (
v15.0.0)
#6 Updated by Xiubo Li over 2 years ago
- Assignee set to Xiubo Li
#7 Updated by Xiubo Li over 1 year ago
- Assignee deleted (
Xiubo Li)
#8 Updated by Tamar Shacked over 1 year ago
The issue suggests spreading LogSegment* as shared_ptr while class MDLog manages those ptrs lifetime (creates/stores/deletes) and the other objects use get_current_segment() / get_segment() for reading it.
So Im not sure what the benefit of sharing LogSegment* ownership via shared_ptr as only one owner responsible to create/delete them.
Am I missing something :-)
#9 Updated by Xiubo Li over 1 year ago
Tamar Shacked wrote:
The issue suggests spreading LogSegment* as shared_ptr while class MDLog manages those ptrs lifetime (creates/stores/deletes) and the other objects use get_current_segment() / get_segment() for reading it.
So Im not sure what the benefit of sharing LogSegment* ownership via shared_ptr as only one owner responsible to create/delete them.
Am I missing something :-)
Yeah, IMO it should be a good habit to use the shared_ptr to avoid potential use-after-free bugs as we hit in client/Client.cc before, which were very hard to debug. And this could simplify the code too.
#10 Updated by Tamar Shacked over 1 year ago
Yeah, IMO it should be a good habit to use the shared_ptr to avoid potential use-after-free bugs as we hit in client/Client.cc before, which were very hard to debug. And this could simplify the code too.
Sure, I thought that a case of the current_segment being deleted while it's getting read isn't applicable, but using shared_ptr is safer and I"ll prepare the PR.
Still want to note that shared_ptr can be expansive:
https://www.modernescpp.com/index.php/memory-and-performance-overhead-of-smart-pointer
Was a memory pool which can be grow and shrink was considered (or maybe it isn't not a good choice as the LogSegments holds a few list<T*>)?
#11 Updated by Greg Farnum over 1 year ago
For those following along, most MDS operations involve something like "mut->ls = get_current_segment()", and the possibility of breaking those references is the concern this ticket is meant to address.
#12 Updated by Greg Farnum over 1 year ago
- Assignee set to Tamar Shacked
#13 Updated by Tamar Shacked over 1 year ago
- Status changed from New to Fix Under Review
- Pull request ID set to 47598
#14 Updated by Venky Shankar 8 months ago
- Assignee deleted (
Tamar Shacked) - Target version set to v19.0.0
- Backport set to reef
#15 Updated by Venky Shankar 8 months ago
- Labels (FS) deleted (
task(easy), task(intern))
#16 Updated by Venky Shankar 8 months ago
- Assignee set to Leonid Usov
#17 Updated by Leonid Usov 7 months ago
- Status changed from Fix Under Review to In Progress
#18 Updated by Leonid Usov 7 months ago
- Pull request ID changed from 47598 to 53086