mds: Don't use global snaprealm's seq number for subvolume snapshots by kotreshhr · Pull Request #62682 · ceph/ceph

kotreshhr · 2025-04-04T13:31:47Z

mds: Don't use global snaprealm seq for for subvolumes

Don't use global snaprealm seq number while doing cow
on old inodes which belongs to subvolume or under it i.e.,
for directories marked with 'ceph.dir.subvolume' vxattr. This is
safe because hardlinks and renames are not allowed beyond
these subvolume directories i.e., all the hardlink/renames
are contained within the same subvolume snaprealm and doesn't
cross the subvolume snaprealms

Fixes: https://tracker.ceph.com/issues/70794
Signed-off-by: Kotresh HR khiremat@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

gregsfortytwo

I thought we'd talked about having a config option to turn this behavior on or off? Looks like it's just making an unconditional change to all filesystems (including existing ones!) with the current code?

kotreshhr · 2025-04-04T15:46:57Z

I thought we'd talked about having a config option to turn this behavior on or off? Looks like it's just making an unconditional change to all filesystems (including existing ones!) with the current code?

Yes, that's pending. I will add it.

kotreshhr · 2025-04-05T07:17:10Z

@vshankar the commit e9285e0 (marking 'ceph.dir.subvolume' only if the directory is empty) breaks the subvolume operations mentioned as below.

The subvolume create - sets the 'ceph.dir.subvolume' after creating .meta I think.
- This can be fixed by marking it before the creation of '.meta' file.
Idempotency of subvolume operations like create - the subvolume is not empty on second create
- This is taken care as the 'non-empty-dir' check is after the check of 'if subvolume is alreay marked'
Marks all the subvolumes with 'ceph.dir.subvoulme' on subvolume open to address upgrades.
- This needs little more thought on how to proceed further.

vshankar · 2025-04-05T09:37:11Z

Thanks for the quick turnaround @kotreshhr. I will have a look at the change.

kotreshhr · 2025-04-05T19:43:33Z

Added config option to use global_snaprealm's seq number
Mark subvolume vxattr before creating uuid or config files

vshankar

Mostly looks fine. @kotreshhr We need to do targetted test for this change. What options do we have?

src/common/options/mds.yaml.in

kotreshhr · 2025-04-07T06:39:31Z

Mostly looks fine. @kotreshhr We need to do targetted test for this change. What options do we have?

I will explore that today.

But I didn't find a good solution for the 3rd point mentioned in the comment #62682 (comment) @vshankar You have any thoughts ?

vshankar · 2025-04-07T06:51:47Z

Marks all the subvolumes with 'ceph.dir.subvoulme' on subvolume open to address upgrades.

This needs little more thought on how to proceed further.

How would this be done since the constraint introduced in this change disallows marking a dir as subvolume if it's not empty? Also, this needs to be done for all existing subvolumes which involves scanning the volumes directory??

EDIT: fixed typos

kotreshhr · 2025-04-07T08:47:53Z

Marks all the subvolumes with 'ceph.dir.subvoulme' on subvolume open to address upgrades.

This needs little more thought on how to proceed further.

How would this be done since the constraint introduced in this change disallows marking a dir as subvolume if it's not empty?

Exactly the problem, we have introduced the contradictory fix with this PR. The question is how do we handle upgrades of subvolumes (legacy, v1) which don't have this xattr set ?

Also, this needs to be done for all existing subvolumes which involves scanning the volumes directory??

I didn't quite get this question. This is required for existing subvolumes which didn't had xattr set.
The xattr set currently is done during subvolume open. If there were legacy, v1, v2 subvolume prior to the introduction of this xattr, those subvolumes would get marked with this xattr during open.

EDIT: Answered second question. Had pressed comment button midway

kotreshhr · 2025-04-07T10:28:43Z

Marks all the subvolumes with 'ceph.dir.subvoulme' on subvolume open to address upgrades.

This needs little more thought on how to proceed further.

How would this be done since the constraint introduced in this change disallows marking a dir as subvolume if it's not empty?

Exactly the problem, we have introduced the contradictory fix with this PR. The question is how do we handle upgrades of subvolumes (legacy, v1) which don't have this xattr set ?

Also, this needs to be done for all existing subvolumes which involves scanning the volumes directory??

I didn't quite get this question. This is required for existing subvolumes which didn't had xattr set. The xattr set currently is done during subvolume open. If there were legacy, v1, v2 subvolume prior to the introduction of this xattr, those subvolumes would get marked with this xattr during open.

EDIT: Answered second question. Had pressed comment button midway

@vshankar and me discussed about this in a meeting. Please find the discussion here.

The enforcement of setting the 'ceph.dir.subvolume' only on non-empty directory (e9285e0) is logically correct but it's not useful in the world of subvolumes. Each pvc is backed by a subvolume and it's not possible to create hardlink/rename outside it.
To make the legacy/v1/v2 subvolumes created before the introduction of the 'ceph.dir.subvolume' xattr to get marked with the xattr possible, it's decided that we skip the fix of enforcement of setting xattr on non-empty directory for now.

kotreshhr · 2025-04-07T13:18:39Z

So I did the preliminary targetted testing with the fix.

Since the current fix uses subvolume realm's seq number for subvolume inode and inodes under it, it's preventing blowing up of cowing old inodes only for those. But for '/volumes/' and '/volumes/<group_dir>/', since the fix is using global snaprealm's seq number, the number of cowed old inodes explodes. So we are not fully solving the issue for subvolume use case as well.

@vshankar and me discussed this in the meeting and decided to use the corresponding realms sequence number fully and make the fix optional.

I will refresh the PR with the following.

Remove enforcing of 'ceph.dir.subvolume' only on empty directory.
Use the corresponding realm's seq number over all not restricting only to subvolume directory and below.

kotreshhr · 2025-04-07T13:46:19Z

So I did the preliminary targetted testing with the fix.

Since the current fix uses subvolume realm's seq number for subvolume inode and inodes under it, it's preventing blowing up of cowing old inodes only for those. But for '/volumes/' and '/volumes/<group_dir>/', since the fix is using global snaprealm's seq number, the number of cowed old inodes explodes. So we are not fully solving the issue for subvolume use case as well.

@vshankar and me discussed this in the meeting and decided to use the corresponding realms sequence number fully and make the fix optional.

I will refresh the PR with the following.

Remove enforcing of 'ceph.dir.subvolume' only on empty directory.

Use the corresponding realm's seq number over all not restricting only to subvolume directory and below.

Done. Yet to test this. I will test and update.

kotreshhr · 2025-04-07T19:49:26Z

So I did the preliminary targetted testing with the fix.
Since the current fix uses subvolume realm's seq number for subvolume inode and inodes under it, it's preventing blowing up of cowing old inodes only for those. But for '/volumes/' and '/volumes/<group_dir>/', since the fix is using global snaprealm's seq number, the number of cowed old inodes explodes. So we are not fully solving the issue for subvolume use case as well.
@vshankar and me discussed this in the meeting and decided to use the corresponding realms sequence number fully and make the fix optional.
I will refresh the PR with the following.

Remove enforcing of 'ceph.dir.subvolume' only on empty directory.

Use the corresponding realm's seq number over all not restricting only to subvolume directory and below.

Done. Yet to test this. I will test and update.

Tested the fix locally and it works fine. We need to highlight the restriction of this fix. The fix is controlled by the config mds_use_global_snaprealm_seq_for_subvol and needs to be disabled only on cephfs volumes which are dedicated to subvolumes and not used for any other purposes

I have done this in the option description here c85a6fa#diff-2c5822fc1f3196e9d93cbfe46ccdc0796a449323d075cc562332fc3b51e974eaR1572

kotreshhr · 2025-04-08T06:16:39Z

jenkins test make check arm64

src/common/options/mds.yaml.in

kotreshhr · 2025-04-10T11:38:47Z

jenkins test make check

kotreshhr · 2025-04-10T14:18:45Z

jenkins test make check arm64

kotreshhr · 2025-04-12T17:27:22Z

The snaps tests and yaml fragment added looks fine.

https://pulpito.ceph.com/khiremat-2025-04-12_15:56:58-fs:snaps-wip-khiremat-62682-cow-old-inodes-Apr102025-2-debug-testing-default-smithi/
https://pulpito.ceph.com/khiremat-2025-04-12_16:06:06-fs:volumes-wip-khiremat-62682-cow-old-inodes-Apr102025-2-debug-testing-default-smithi/

src/mds/CInode.cc

qa/suites/fs/workload/tasks/7-mds-use-global-snaprealm-seq-config

Removing vxattr 'ceph.dir.subvolume' on a directory without it being set causes the mds to crash. This is because the snaprealm would be null for the directory and the null check is missing. Setting the vxattr, creates the snaprealm for the directory as part of it. Hence, mds doesn't crash when the vxattr is set and then removed. This patch fixes the same. Reproducer: $mkdir /mnt/dir1 $setfattr -x "ceph.dir.subvolume" /mnt/dir1 Traceback: ------- Core was generated by `./ceph/build/bin/ceph-mds -i a -c ./ceph/build/ceph.conf'. Program terminated with signal SIGSEGV, Segmentation fault. (gdb) bt #0 0x00007f33f1aa8034 in __pthread_kill_implementation () from /lib64/libc.so.6 #1 0x00007f33f1a4ef1e in raise () from /lib64/libc.so.6 #2 0x0000562b148a6fd0 in reraise_fatal (signum=signum@entry=11) at /ceph/src/global/signal_handler.cc:88 #3 0x0000562b148a83d9 in handle_oneshot_fatal_signal (signum=11) at /ceph/src/global/signal_handler.cc:367 #4 <signal handler called> #5 Server::handle_client_setvxattr (this=0x562b4ee3f800, mdr=..., cur=0x562b4ef9cc00) at /ceph/src/mds/Server.cc:6406 #6 0x0000562b145fadc2 in Server::handle_client_removexattr (this=0x562b4ee3f800, mdr=...) at /ceph/src/mds/Server.cc:7022 #7 0x0000562b145fbff0 in Server::dispatch_client_request (this=0x562b4ee3f800, mdr=...) at /ceph/src/mds/Server.cc:2825 #8 0x0000562b145fcfa2 in Server::handle_client_request (this=0x562b4ee3f800, req=...) at /ceph/src/mds/Server.cc:2676 #9 0x0000562b1460063c in Server::dispatch (this=0x562b4ee3f800, m=...) at /ceph/src/mds/Server.cc:382 #10 0x0000562b1450eb22 in MDSRank::handle_message (this=this@entry=0x562b4ef42008, m=...) at /ceph/src/mds/MDSRank.cc:1222 #11 0x0000562b14510c93 in MDSRank::_dispatch (this=this@entry=0x562b4ef42008, m=..., new_msg=new_msg@entry=true) at /ceph/src/mds/MDSRank.cc:1045 #12 0x0000562b14511620 in MDSRankDispatcher::ms_dispatch (this=this@entry=0x562b4ef42000, m=...) at /ceph/src/mds/MDSRank.cc:1019 #13 0x0000562b144ff117 in MDSDaemon::ms_dispatch2 (this=0x562b4ee64000, m=...) at /ceph/src/common/RefCountedObj.h:56 #14 0x00007f33f2f4974a in Messenger::ms_deliver_dispatch (this=0x562b4ee70000, m=...) at /ceph/src/msg/Messenger.h:746 #15 0x00007f33f2f467e2 in DispatchQueue::entry (this=0x562b4ee703b8) at /ceph/src/msg/DispatchQueue.cc:202 #16 0x00007f33f2ff61cb in DispatchQueue::DispatchThread::entry (this=<optimized out>) at /ceph/src/msg/DispatchQueue.h:101 #17 0x00007f33f2df4b5d in Thread::entry_wrapper (this=0x562b4ee70518) at /ceph/src/common/Thread.cc:87 #18 0x00007f33f2df4b6f in Thread::_entry_func (arg=<optimized out>) at /ceph/src/common/Thread.cc:74 #19 0x00007f33f1aa6088 in start_thread () from /lib64/libc.so.6 #20 0x00007f33f1b29f8c in clone3 () from /lib64/libc.so.6 --------- Fixes: https://tracker.ceph.com/issues/70794 Signed-off-by: Kotresh HR <khiremat@redhat.com>

Don't use global snaprealm seq number while doing cow on old inodes for subvolume inode and inodes under it i.e., for directories marked with 'ceph.dir.subvolume' vxattr. This is safe because all the hardlink/renames are contained within the same subvolume snaprealm and doesn't cross the subvolume snaprealms For the directories between / and subvolume snapshot directory, use the global snaprealm seq to cow the old inodes only if there is atleast one snapshot taken. The above behavior is made optional with the mds config 'mds_use_global_snaprealm_seq_for_subvol'. The option is enabled by default which means the above behaviour is disabled by default. The option is suggested to be disabled only on cephfs volumes used for pure subvolume usecase. Fixes: https://tracker.ceph.com/issues/70794 Signed-off-by: Kotresh HR <khiremat@redhat.com>

kotreshhr · 2025-05-09T09:13:12Z

Rebased

kotreshhr · 2025-05-13T07:29:33Z

jenkins test api

kotreshhr · 2025-05-14T09:30:26Z

@vshankar @batrick Tried reproducing the original issue - https://tracker.ceph.com/issues/67102 locally by creating and deleting thousands of subvolume snapshots. The old inodes on the root and /volumes directory does increase but come down to zero after journal flush. The tracker information isn't helping much to reproduce. I will sync up with QE on this to reproduce this separately. I think the optimization done as part of this PR can go in while I work at the original issue separately.

vshankar · 2025-05-19T04:40:38Z

@vshankar @batrick Tried reproducing the original issue - https://tracker.ceph.com/issues/67102 locally by creating and deleting thousands of subvolume snapshots. The old inodes on the root and /volumes directory does increase but come down to zero after journal flush. The tracker information isn't helping much to reproduce. I will sync up with QE on this to reproduce this separately. I think the optimization done as part of this PR can go in while I work at the original issue separately.

That makes sense. I will have a look again and put the change to test.

vshankar · 2025-05-25T12:23:23Z

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

kotreshhr · 2025-05-29T05:48:25Z

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

@vshankar Fixed the snapshot clean up failure discussed in the above QA Tracker - https://tracker.ceph.com/issues/71431?issue_count=61&issue_position=3&next_issue_id=71428&prev_issue_id=71441#note-3

Fixes: https://tracker.ceph.com/issues/70794 Signed-off-by: Kotresh HR <khiremat@redhat.com>

The snapshot data on any part of the subtree should be correct irrespective enabling/disabling 'mds_use_global_snaprealm_seq_for_subvol' even with a directory marked as subvolume. Fixes: https://tracker.ceph.com/issues/70794 Signed-off-by: Kotresh HR <khiremat@redhat.com>

kotreshhr · 2025-05-29T10:58:07Z

jenkins test make check

kotreshhr · 2025-05-29T17:26:56Z

jenkins test make check

vshankar · 2025-06-02T08:46:14Z

LGTM: https://tracker.ceph.com/issues/71431#note-10

vshankar

Nice work @kotreshhr

kotreshhr · 2025-07-10T13:46:40Z

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

kotreshhr · 2025-07-14T08:15:17Z

I thought we'd talked about having a config option to turn this behavior on or off? Looks like it's just making an unconditional change to all filesystems (including existing ones!) with the current code?

@gregsfortytwo This is made configurable. Re-requesting review

kotreshhr · 2025-07-15T06:31:20Z

@batrick ptal

My concern about configuration was addressed!

batrick

@batrick @vshankar I added the tests with journal flush. The results are little surprising and it's not always a fixed number of old_inodes for /volumes or /volumes/group when root_snaps are involved. Please check the note here 8b60e67#diff-d3a7e3f3f24fff510b4d2a562b2093257b20b3908748c3432d14e460c449186bR513 where I tried explaining.

Okay, so this is purely an optimization at this point. I'm fine with it.

vshankar

Well done @kotreshhr

github-actions bot added cephfs Ceph File System tests labels Apr 4, 2025

kotreshhr requested review from a team and vshankar April 4, 2025 13:32

gregsfortytwo previously requested changes Apr 4, 2025

View reviewed changes

kotreshhr force-pushed the wip-cow-old-inodes branch from 418691f to bb6573c Compare April 5, 2025 19:41

github-actions bot added common pybind labels Apr 5, 2025

vshankar requested changes Apr 7, 2025

View reviewed changes

src/common/options/mds.yaml.in Outdated Show resolved Hide resolved

kotreshhr force-pushed the wip-cow-old-inodes branch from bb6573c to 1cbce89 Compare April 7, 2025 13:44

kotreshhr force-pushed the wip-cow-old-inodes branch from 1cbce89 to 8186ad0 Compare April 7, 2025 19:40

kotreshhr commented Apr 8, 2025

View reviewed changes

src/common/options/mds.yaml.in Outdated Show resolved Hide resolved

kotreshhr force-pushed the wip-cow-old-inodes branch from 8186ad0 to 99efb90 Compare April 9, 2025 10:37

vshankar reviewed Apr 14, 2025

View reviewed changes

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

vshankar reviewed Apr 14, 2025

View reviewed changes

qa/suites/fs/workload/tasks/7-mds-use-global-snaprealm-seq-config Outdated Show resolved Hide resolved

kotreshhr added 2 commits May 9, 2025 14:42

kotreshhr force-pushed the wip-cow-old-inodes branch from 91913da to d83b589 Compare May 9, 2025 09:12

vshankar mentioned this pull request May 19, 2025

mds: revert referent inode work #62351

Open

14 tasks

kotreshhr force-pushed the wip-cow-old-inodes branch from d83b589 to cddbc55 Compare May 29, 2025 05:47

kotreshhr added 3 commits May 29, 2025 14:03

qa: Add test for mds_use_global_snaprealm_seq_for_subvol config

cc09354

Fixes: https://tracker.ceph.com/issues/70794 Signed-off-by: Kotresh HR <khiremat@redhat.com>

qa: Add yes/no yaml for mds_use_global_snaprealm_seq_for_subvol conf

806da64

Fixes: https://tracker.ceph.com/issues/70794 Signed-off-by: Kotresh HR <khiremat@redhat.com>

kotreshhr force-pushed the wip-cow-old-inodes branch from cddbc55 to 57e1408 Compare May 29, 2025 08:33

vshankar approved these changes Jul 10, 2025

View reviewed changes

vshankar requested review from batrick, gregsfortytwo and vshankar July 10, 2025 14:53

vshankar removed the wip-vshankar-testing3 label Jul 22, 2025

batrick approved these changes Jul 23, 2025

View reviewed changes

vshankar approved these changes Jul 24, 2025

View reviewed changes

vshankar merged commit 82603e9 into ceph:main Jul 24, 2025
12 checks passed

Conversation

kotreshhr commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

gregsfortytwo left a comment

Choose a reason for hiding this comment

Uh oh!

kotreshhr commented Apr 4, 2025

Uh oh!

kotreshhr commented Apr 5, 2025

Uh oh!

vshankar commented Apr 5, 2025

Uh oh!

kotreshhr commented Apr 5, 2025

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kotreshhr commented Apr 7, 2025

Uh oh!

vshankar commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kotreshhr commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kotreshhr commented Apr 7, 2025

Uh oh!

kotreshhr commented Apr 7, 2025

Uh oh!

kotreshhr commented Apr 7, 2025

Uh oh!

kotreshhr commented Apr 7, 2025

Uh oh!

kotreshhr commented Apr 8, 2025

Uh oh!

Uh oh!

kotreshhr commented Apr 10, 2025

Uh oh!

kotreshhr commented Apr 10, 2025

Uh oh!

kotreshhr commented Apr 12, 2025

Uh oh!

Uh oh!

Uh oh!

kotreshhr commented May 9, 2025

Uh oh!

kotreshhr commented May 13, 2025

Uh oh!

kotreshhr commented May 14, 2025

Uh oh!

vshankar commented May 19, 2025

Uh oh!

vshankar commented May 25, 2025

Uh oh!

kotreshhr commented May 29, 2025

Uh oh!

kotreshhr commented May 29, 2025

Uh oh!

kotreshhr commented May 29, 2025

Uh oh!

vshankar commented Jun 2, 2025

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

kotreshhr commented Jul 10, 2025

Uh oh!

kotreshhr commented Jul 14, 2025

Uh oh!

kotreshhr commented Jul 15, 2025

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

kotreshhr commented Apr 4, 2025 •

edited

Loading

vshankar commented Apr 7, 2025 •

edited

Loading

kotreshhr commented Apr 7, 2025 •

edited

Loading