mds: add ref counting to LogSegment by mchangir · Pull Request #62554 · ceph/ceph

mchangir · 2025-03-28T12:48:23Z

Fixes: https://tracker.ceph.com/issues/70723
Signed-off-by: Milind Changire mchangir@redhat.com

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins test classic perf Jenkins Job | Jenkins Job Definition
jenkins test crimson perf Jenkins Job | Jenkins Job Definition
jenkins test signed Jenkins Job | Jenkins Job Definition
jenkins test make check Jenkins Job | Jenkins Job Definition
jenkins test make check arm64 Jenkins Job | Jenkins Job Definition
jenkins test submodules Jenkins Job | Jenkins Job Definition
jenkins test dashboard Jenkins Job | Jenkins Job Definition
jenkins test dashboard cephadm Jenkins Job | Jenkins Job Definition
jenkins test api Jenkins Job | Jenkins Job Definition
jenkins test docs ReadTheDocs | Github Workflow Definition
jenkins test ceph-volume all Jenkins Jobs | Jenkins Jobs Definition
jenkins test windows Jenkins Job | Jenkins Job Definition
jenkins test rook e2e Jenkins Job | Jenkins Job Definition

vshankar · 2025-04-10T05:25:21Z

src/mds/MDLog.h

 class LogSegment;
 class ESubtreeMap;

+using LogSegmentRef = boost::intrusive_ptr<LogSegment>;


Ugh.. where are intrusive_ptr_add_ref and intrusive_ptr_release implemented? o_O

Oh I see, LogSegment is a subclass of RefCountedObject. Interesting.

vshankar · 2025-04-10T06:06:25Z

src/mds/CDir.h

 class MDCache;
+class LogSegment;
+
+using LogSegmentRef = boost::intrusive_ptr<LogSegment>;


Generally, using using keyword is discouraged in header files, especially when its done outside the scope of the class itself (CDir in this case).

I'll move all such instances inside the class definitions.

rebased

moved using LogSegmentRef = inside class definitions

github-actions · 2025-04-22T15:40:42Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

mchangir · 2025-05-07T02:09:41Z

rebased

src/mds/LogSegment.h

mchangir · 2025-06-10T14:47:04Z

Note: PR has been revised completely

batrick · 2025-06-10T17:30:59Z

src/mds/MDCache.cc

  adjust_subtree_auth(mydir, mds->get_nodeid());   

-  LogSegment *ls = mds->mdlog->get_current_segment();
+  std::shared_ptr<LogSegment> &ls = mds->mdlog->get_current_segment();


Suggested change

std::shared_ptr<LogSegment> &ls = mds->mdlog->get_current_segment();

auto&& ls = mds->mdlog->get_current_segment();

is more robust. I'd also like to see:

using LogSegmentRef = std::shared_ptr<LogSegment>;

in the headers.

OK, I had advised @mchangir to not use using keyword in headers. I see now, this is being done via a new header as per #62554 (comment)

So, it looks like using in header is fine iff it's a separate header and this header is only included in cc source, yes?

Following are the headers in which LogSegmentRef.h is included:

src/mds/CDentry.h:#include "LogSegmentRef.h" src/mds/CDir.h:#include "LogSegmentRef.h" src/mds/CInode.h:#include "LogSegmentRef.h" src/mds/LogEvent.h:#include "LogSegmentRef.h" src/mds/MDCache.h:#include "LogSegmentRef.h" src/mds/MDLog.h:#include "LogSegmentRef.h" src/mds/MDSTableClient.h:#include "LogSegmentRef.h" src/mds/Migrator.h:#include "LogSegmentRef.h" src/mds/Mutation.h:#include "LogSegmentRef.h" src/mds/Server.h:#include "LogSegmentRef.h" src/mds/events/EMetaBlob.h:#include "../LogSegmentRef.h"

as class function prototypes also make a reference to LogSegmentRef ... otherwise the compiler will spew errors about undefined symbols.

or, maybe include LogSegmentRef.h in the .cc before any other header included in the .cc makes a reference to LogSegmentRef ?

I think as long as you include LogSegmentRef.h in LogSegment.h, you can avoid explicitly including it in most of those .cc files. You only need the separate header for places where you do not want to include LogSegment.h for circular dependency reasons.

There's not many files that both LogSegment.h and LogSegmentRef.h get included. Here's the list:

$ git grep -E 'LogSegment\.h' src/mds/CDentry.cc:#include "LogSegment.h" src/mds/CDir.cc:#include "LogSegment.h" src/mds/CInode.cc:#include "LogSegment.h" src/mds/MDLog.h:#include "LogSegment.h" src/mds/MDSTableClient.cc:#include "LogSegment.h" src/mds/SegmentBoundary.h:#include "LogSegment.h" src/mds/events/EMetaBlob.h:#include "../LogSegment.h" src/mds/journal.cc:#include "LogSegment.h" $ git grep -E 'LogSegmentRef\.h' src/mds/CDentry.h:#include "LogSegmentRef.h" src/mds/CDir.h:#include "LogSegmentRef.h" src/mds/CInode.h:#include "LogSegmentRef.h" src/mds/LogEvent.h:#include "LogSegmentRef.h" src/mds/MDCache.h:#include "LogSegmentRef.h" src/mds/MDLog.h:#include "LogSegmentRef.h" src/mds/MDSTableClient.h:#include "LogSegmentRef.h" src/mds/Migrator.h:#include "LogSegmentRef.h" src/mds/Mutation.h:#include "LogSegmentRef.h" src/mds/Server.h:#include "LogSegmentRef.h" src/mds/events/EMetaBlob.h:#include "../LogSegmentRef.h" src/mds/journal.cc:#include "LogSegmentRef.h"

so the only common files among two lists are:

src/mds/MDLog.h src/mds/events/EMetaBlob.h src/mds/journal.cc

I've mostly included LogSegmentRef.h in files where there was no reference to LogSegment.h. I haven't tested what happens if I include LogSegmentRef.h via LogSegment.h.

batrick

otherwise LGTM

src/mds/LogEvent.h

src/mds/MDLog.cc

src/mds/MDLog.h

batrick · 2025-06-12T12:57:49Z

src/mds/events/EMetaBlob.h

 *
 */

+using LogSegmentRef = std::shared_ptr<LogSegment>;


Create a new header file mds/LogSegmentRef.h which is just:

<copyright> #pragma once class LogSegment; using LogSegmentRef = std::shared_ptr<LogSegment>;

should this header be included in all the files that has using LogSegmentRef = defined and replace this declaration ?

@mchangir I think you forgot to stage the LogSegmentRef.h header.

thanks for taking a look ... I've now included LogSegmentRef.h in the commit.

Fixes: https://tracker.ceph.com/issues/70723 Signed-off-by: Milind Changire <mchangir@redhat.com>

vshankar · 2025-07-21T15:53:21Z

This PR is under test in https://tracker.ceph.com/issues/72210.

vshankar

https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-vshankar-testing-20250721155223-debug

Nice work @mchangir

github-actions bot added the cephfs Ceph File System label Mar 28, 2025

mchangir force-pushed the mds-add-ref-counting-to-LogSegment branch 3 times, most recently from 59e5177 to 9751c70 Compare March 30, 2025 15:35

mchangir added the needs-review label Apr 7, 2025

vshankar reviewed Apr 10, 2025

View reviewed changes

mchangir force-pushed the mds-add-ref-counting-to-LogSegment branch from 9751c70 to 6f68417 Compare April 16, 2025 09:14

vshankar assigned vshankar, batrick and kotreshhr Apr 22, 2025

github-actions bot added the needs-rebase label Apr 22, 2025

mchangir force-pushed the mds-add-ref-counting-to-LogSegment branch from 6f68417 to a862e7c Compare April 23, 2025 14:17

github-actions bot removed the needs-rebase label Apr 23, 2025

mchangir force-pushed the mds-add-ref-counting-to-LogSegment branch from a862e7c to fffa4d9 Compare May 7, 2025 02:09

batrick requested changes Jun 9, 2025

View reviewed changes

src/mds/LogSegment.h Outdated Show resolved Hide resolved

mchangir force-pushed the mds-add-ref-counting-to-LogSegment branch from fffa4d9 to 4403702 Compare June 10, 2025 14:44

batrick requested changes Jun 10, 2025

View reviewed changes

mchangir force-pushed the mds-add-ref-counting-to-LogSegment branch from 4403702 to 959d2e2 Compare June 11, 2025 17:47

batrick requested changes Jun 11, 2025

View reviewed changes

mchangir force-pushed the mds-add-ref-counting-to-LogSegment branch from 959d2e2 to 3154072 Compare June 12, 2025 08:04

mchangir requested a review from batrick June 12, 2025 08:07

batrick requested changes Jun 12, 2025

View reviewed changes

mchangir force-pushed the mds-add-ref-counting-to-LogSegment branch from 3154072 to a51b170 Compare June 13, 2025 04:05

mchangir requested a review from batrick June 13, 2025 04:07

mds: add ref counting to LogSegment

fe3b415

Fixes: https://tracker.ceph.com/issues/70723 Signed-off-by: Milind Changire <mchangir@redhat.com>

mchangir force-pushed the mds-add-ref-counting-to-LogSegment branch from a51b170 to fe3b415 Compare July 8, 2025 00:52

batrick approved these changes Jul 8, 2025

View reviewed changes

batrick added needs-qa and removed needs-review labels Jul 8, 2025

vshankar approved these changes Jul 21, 2025

View reviewed changes

vshankar added the wip-vshankar-testing4 label Jul 21, 2025

vshankar approved these changes Jul 25, 2025

View reviewed changes

vshankar merged commit 742cb59 into ceph:main Jul 25, 2025
13 checks passed

vshankar removed the wip-vshankar-testing4 label Jul 25, 2025

	std::shared_ptr<LogSegment> &ls = mds->mdlog->get_current_segment();
	auto&& ls = mds->mdlog->get_current_segment();

Conversation

mchangir commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 22, 2025

Uh oh!

mchangir commented May 7, 2025

Uh oh!

Uh oh!

mchangir commented Jun 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vshankar commented Jul 21, 2025

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mchangir commented Mar 28, 2025 •

edited

Loading