Project

General

Profile

Cleanup #4744

mds: pass around LogSegments via std::shared_ptr

Added by Greg Farnum almost 10 years ago. Updated 5 months ago.

Status:
Fix Under Review
Priority:
Normal
Assignee:
Category:
Code Hygiene
Target version:
-
% Done:

0%

Tags:
Backport:
Reviewed:
Affected Versions:
Component(FS):
MDS
Labels (FS):
task(easy), task(intern)
Pull request ID:

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

#7 Updated by Xiubo Li 7 months ago

  • Assignee deleted (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

Also available in: Atom PDF