pybind/cephfs, mgr/volumes: introduce non-recursive rmtree(), refactor purge() to use it and add MDS optimizations by rishabh-d-dave · Pull Request #63917 · ceph/ceph

rishabh-d-dave · 2025-06-13T07:31:57Z

Method purge() in trash.py calls rmtree() which is recursive method. To
avoid Python's recurision limit, switch to non-recursive approach.

Fixes: https://tracker.ceph.com/issues/71648

Also fixes https://tracker.ceph.com/issues/72779 and https://tracker.ceph.com/issues/72993.

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

src/pybind/mgr/volumes/fs/operations/trash.py

rishabh-d-dave · 2025-06-16T09:21:54Z

jenkins test make check

rishabh-d-dave · 2025-06-16T11:00:15Z

jenkins test make check arm64

src/pybind/mgr/volumes/fs/operations/trash.py

batrick

Add more error handling and refactor the code a bit.

src/pybind/mgr/volumes/fs/operations/trash.py

rishabh-d-dave · 2025-06-20T15:09:31Z

@batrick I had some confusion regarding in the code suggested in last review cycle but I think I've got it sorted. I've also made some improvements along the way. PTAL, I'll mark previous review threads resolved based on next set of reviews.

The current PR branch has been tested for deletion of subvolumes of several kinds: ones with reg files, symlinks, dirs, retained snapshots, etc. All tests ran fine.

rishabh-d-dave · 2025-06-23T08:58:53Z

jenkins test make check

rishabh-d-dave · 2025-06-23T08:58:57Z

jenkins test make check arm64

rishabh-d-dave · 2025-06-23T12:42:30Z

jenkins test make check

rishabh-d-dave · 2025-06-23T12:42:34Z

jenkins test make check arm64

batrick

Please add code for tracking what kinds of errors were encountered so we can get useful diagnostics when debugging. This needs to be carried up the stack to the root when the operation completes (either successfully or not).

src/pybind/mgr/volumes/fs/operations/trash.py

rishabh-d-dave · 2025-09-16T14:56:38Z

jenkins test make check

rishabh-d-dave · 2025-09-16T14:56:44Z

jenkins test api

rishabh-d-dave · 2025-09-16T15:29:10Z

@batrick ping

src/mds/CInode.cc

src/include/filepath.cc

batrick

nearly there!

src/client/Client.cc

src/mds/ScrubStack.cc

src/mds/SessionMap.cc

src/mds/Server.cc

src/mds/CDir.cc

Generating full absolute path for inodes for printing in MDS logs slows down the FS to a great extent especially when the path is very long (imagine a path with 2000 components). Also printing such long paths in MDS logs is not only pointless but also greatly reduces the readability of the MDS logs. Therefore, generate only 10 final components of inode paths for logging. Fixes: https://tracker.ceph.com/issues/72779 Signed-off-by: Rishabh Dave <ridave@redhat.com>

Generating full absolute path for dentries for printing in MDS logs slows the down the FS to a great extent especially when the path is very long (imagine a path with 2000 components). Printing such long paths in MDS logs is not only pointless but also greatly reduces the readability of MDS logs. Therefore, generate only 10 final components of the dentry paths for logging. Fixes: https://tracker.ceph.com/issues/72779 Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave · 2025-09-26T18:33:15Z

.

rishabh-d-dave · 2025-10-01T01:59:31Z

jenkins test make check

batrick

Otherwise, LGTM. Remaining nits can be addressed with filepath refactor.

src/mds/Server.cc

Signed-off-by: Rishabh Dave <ridave@redhat.com>

Path can be virtually infinitely long and logging a long long path (imagine around 2000 path components) is un-useful as well as lowers readability of the log. Therefore, trim before logging. Fixes: https://tracker.ceph.com/issues/72993 Signed-off-by: Rishabh Dave <ridave@redhat.com>

Signed-off-by: Rishabh Dave <ridave@redhat.com>

batrick

Again, please have @vshankar also take a look.

rishabh-d-dave · 2025-10-03T15:56:10Z

@vshankar mentioned that he's too occupied with other and therefore can't get to this anytime soon. Checked with @batrick regarding this, it's fine to initiate QA.

rishabh-d-dave · 2025-10-03T15:58:22Z

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

rishabh-d-dave · 2025-10-03T16:10:28Z

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

rishabh-d-dave

QA run was successful - https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-rishabh-testing-20251003161005-debug

rishabh-d-dave added cephfs Ceph File System pybind needs-review labels Jun 13, 2025

rishabh-d-dave requested review from a team and vshankar June 13, 2025 07:31

batrick requested changes Jun 13, 2025

View reviewed changes

src/pybind/mgr/volumes/fs/operations/trash.py Outdated Show resolved Hide resolved

rishabh-d-dave force-pushed the vols-purge-trash-entry branch from c0c7904 to a658ebf Compare June 16, 2025 07:24

batrick requested changes Jun 17, 2025

View reviewed changes

rishabh-d-dave force-pushed the vols-purge-trash-entry branch 2 times, most recently from a1fe4a9 to 442c53b Compare June 17, 2025 08:29

batrick requested changes Jun 17, 2025

View reviewed changes

rishabh-d-dave force-pushed the vols-purge-trash-entry branch from 442c53b to b448c9a Compare June 17, 2025 16:59

batrick reviewed Jun 17, 2025

View reviewed changes

src/pybind/mgr/volumes/fs/operations/trash.py Outdated Show resolved Hide resolved

rishabh-d-dave force-pushed the vols-purge-trash-entry branch 2 times, most recently from 8a63efd to cbcf76e Compare June 18, 2025 16:00

batrick requested changes Jun 18, 2025

View reviewed changes

rishabh-d-dave force-pushed the vols-purge-trash-entry branch from 703b97e to 5284010 Compare June 20, 2025 15:07

rishabh-d-dave force-pushed the vols-purge-trash-entry branch 3 times, most recently from 8f9769c to c027653 Compare June 21, 2025 10:06

batrick requested changes Jun 23, 2025

View reviewed changes

rishabh-d-dave force-pushed the vols-purge-trash-entry branch 2 times, most recently from ed47679 to 7fc7853 Compare June 24, 2025 11:19

rishabh-d-dave force-pushed the vols-purge-trash-entry branch from d972d0c to 6da05f1 Compare September 17, 2025 05:22

batrick requested changes Sep 18, 2025

View reviewed changes

src/mds/CInode.cc Outdated Show resolved Hide resolved

src/include/filepath.cc Show resolved Hide resolved

rishabh-d-dave force-pushed the vols-purge-trash-entry branch 5 times, most recently from bb34476 to e697d47 Compare September 23, 2025 13:14

batrick requested changes Sep 24, 2025

View reviewed changes

src/client/Client.cc Outdated Show resolved Hide resolved

src/client/Client.cc Outdated Show resolved Hide resolved

src/client/Client.cc Outdated Show resolved Hide resolved

batrick requested changes Sep 26, 2025

View reviewed changes

src/mds/ScrubStack.cc Outdated Show resolved Hide resolved

src/mds/SessionMap.cc Outdated Show resolved Hide resolved

src/mds/Server.cc Outdated Show resolved Hide resolved

src/mds/Server.cc Outdated Show resolved Hide resolved

src/mds/CDir.cc Outdated Show resolved Hide resolved

rishabh-d-dave added 2 commits September 26, 2025 23:19

batrick approved these changes Oct 1, 2025

View reviewed changes

src/mds/Server.cc Outdated Show resolved Hide resolved

rishabh-d-dave added 5 commits October 2, 2025 01:41

mds: log trimmed path wherever generating full path is necessary

11de1e5

Signed-off-by: Rishabh Dave <ridave@redhat.com>

mds: fix few log entries

e4c301b

Signed-off-by: Rishabh Dave <ridave@redhat.com>

mds/ScrubStack: avoid generating inode path since it is unused

10e4ccb

Signed-off-by: Rishabh Dave <ridave@redhat.com>

src/common: add helper to prepend "..." to trimmed paths

c38a913

Signed-off-by: Rishabh Dave <ridave@redhat.com>

batrick approved these changes Oct 1, 2025

View reviewed changes

rishabh-d-dave commented Oct 7, 2025

View reviewed changes

Conversation

rishabh-d-dave commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

rishabh-d-dave commented Jun 16, 2025

Uh oh!

rishabh-d-dave commented Jun 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

rishabh-d-dave commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rishabh-d-dave commented Jun 23, 2025

Uh oh!

rishabh-d-dave commented Jun 23, 2025

Uh oh!

rishabh-d-dave commented Jun 23, 2025

Uh oh!

rishabh-d-dave commented Jun 23, 2025

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!

rishabh-d-dave commented Sep 16, 2025

Uh oh!

rishabh-d-dave commented Sep 16, 2025

Uh oh!

rishabh-d-dave commented Sep 16, 2025

Uh oh!

Uh oh!

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!

rishabh-d-dave commented Sep 26, 2025

Uh oh!

rishabh-d-dave commented Oct 1, 2025

Uh oh!

batrick left a comment

rishabh-d-dave commented Jun 13, 2025 •

edited

Loading

rishabh-d-dave commented Jun 20, 2025 •

edited

Loading