Project

General

Profile

Cleanup #4744

mds: pass around LogSegments via std::shared_ptr

Added by Greg Farnum almost 11 years ago. Updated 7 months ago.

Status:
In Progress
Priority:
Normal
Assignee:
Category:
Code Hygiene
Target version:
% Done:

0%

Tags:
Backport:
reef
Reviewed:
Affected Versions:
Component(FS):
MDS
Labels (FS):
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 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

Also available in: Atom PDF