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 6 years ago
- Category changed from 47 to Code Hygiene
- Component(FS) MDS added
#2 Updated by Patrick Donnelly almost 4 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 4 years ago
- Tracker changed from Feature to Cleanup
- Priority changed from High to Normal
#4 Updated by Patrick Donnelly over 3 years ago
- Status changed from 12 to New
- Assignee deleted (
servesha dudhgaonkar)
#5 Updated by Patrick Donnelly about 3 years ago
- Target version deleted (
v15.0.0)
#6 Updated by Xiubo Li about 1 year ago
- Assignee set to Xiubo Li
#8 Updated by Tamar Shacked 6 months 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 6 months 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 6 months 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 6 months 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 6 months ago
- Assignee set to Tamar Shacked
#13 Updated by Tamar Shacked 5 months ago
- Status changed from New to Fix Under Review
- Pull request ID set to 47598