client: use path supplied in statfs by chrisphoffman · Pull Request #64864 · ceph/ceph

chrisphoffman · 2025-08-06T19:42:15Z

If path provided, use in statfs. Replumb internal statfs
for internal only to allow for use in ll_statfs and statfs

In statfs, get quota_root for inode provided. This will allow for
quotas not set at root to be viewed.

Fixes: https://tracker.ceph.com/issues/72355
Signed-off-by: Christopher Hoffman choffman@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

If path provided, use in statfs. Replumb internal statfs for internal only to allow for use in ll_statfs and statfs Fixes: https://tracker.ceph.com/issues/72355 Signed-off-by: Christopher Hoffman <choffman@redhat.com>

ffilz

A few comments, but overall this looks good. Nice that it ended up being a simple and concise change.

src/client/Client.cc

ffilz · 2025-08-08T15:13:12Z

src/client/Client.cc

  stbuf->f_files = total_files_on_fs;
  stbuf->f_ffree = -1;
  stbuf->f_favail = -1;
-  stbuf->f_fsid = -1;       // ??


So outside a sub-volume f_fsid will be -1? I guess that will work.

I do hope using an inode number is sufficiently unique if multiple filesystems are involved.

It's possible to run into collisions if multiple FSes are used.

Yea, I'm not sure we have to deal with multiple FSes. Is there any way to make more unique?

src/client/Client.cc

ffilz · 2025-08-08T15:23:02Z

src/client/Client.cc

@@ -12973,7 +12974,21 @@ int Client::ll_statfs(Inode *in, struct statvfs *stbuf, const UserPerm& perms)
  /* Since the only thing this does is wrap a call to statfs, and
     statfs takes a lock, it doesn't seem we have a need to split it
     out. */


And those look great, so nice addition of statfs method with path.

Although Ganesha won't use it, you might want to add a path based ll_statfs_path and a ceph_ll_statfs_path.

I see what you mean.

I'm more of a minimalist for adding/changing apis. I'm not aware of a hard requirement for those since the other statfs calls were added in 2013. Perhaps if it's required we add them later?

src/client/Client.cc

batrick · 2025-08-11T13:19:10Z

src/client/Client.cc

-  stbuf->f_fsid = -1;       // ??
+  if (in->snaprealm) {
+    // this is subvolume root ino
+    stbuf->f_fsid = in->snaprealm->ino;


No, this would be an unexpected divergence in behavior for f_fsid. It should remain the same for any CephFS file system. (It also should definitely not be -1.)

I would suggest the proper fix here is to use the last 4 bytes of ceph_fsid (the Ceph cluster UUID) as the most significant 4 bytes of stbuf->f_fsid and the file system ID as the least significant 4 bytes of stbuf->f_fsid.

@ffilz do you really need this to be unique for each subvolume? That's not typical for any quota enabled file system I'm aware of.

On the Linux NFS client, when a user does df, it will do a statfs on each filesystem mounted. The Linux NFS client creates additional mounts whenever it sees a new filesystem (NFS fsid attribute changes). I'm pretty sure statfs will also translate into a statfs on the root inode of such filesystems.

So if we don't show the fsid changing, the Linux NFS client won't understand that there are different statfs attributes (the sub-volume quota for ceph).

So, ideally, the fsid Ganesha gives to the NFS client should be whatever CephFS gives.

I'm concerned about making subvolumes a "file system" abstraction even though they masquerade as such in various ways (e.g. no external links). At the very least, this fsid needs to include parts of the cluster id and CephFS FSID. I'm not sure how to consolidate all of that into a 16 byte field that will work as you intend. The subvolume inode number itself can be 8 bytes and the CEphFS FSID is probably safely considered a 4 byte int.

concatenate the bytes and hash (e.g., xxhash, or even a cryptographic hash)?

The question is if we want the client to understand that a sub-volume is something different. The documented way to clue in the client is to change fsid which in NFSv4 is 2 uint64_t so 16 bytes. In NFSv3 it's 2 uint32_t so only 8 bytes.

Hmm, but it looks like Cephfs allows a quota to be set on any directory. Does that create a sub-volume? Or is quota root something different from sub-volumes (with the no external links and such)?

The question is if we want the client to understand that a sub-volume is something different. The documented way to clue in the client is to change fsid which in NFSv4 is 2 uint64_t so 16 bytes. In NFSv3 it's 2 uint32_t so only 8 bytes.

So I think the better question is: can CephFS provide enough information to Ganesha using libcephfs that it's dealing with a subvolume root? Would NFS-Ganesha be able to communicate that to the NFS client in some standard way that doesn't require encoding it as a 16 byte (or 8 byte for NFSv3) identifier?

Hmm, but it looks like Cephfs allows a quota to be set on any directory. Does that create a sub-volume? Or is quota root something different from sub-volumes (with the no external links and such)?

Setting a quota does not create a subvolume, no. In practice, we expect that clients will not have the necessary CephFS ("mds") capabilities to create/set quotas for subvolumes. Those quotas would be set when the subvolume is created.

So it really sounds like for Ganesha and given the Linux kernel behavior, we need to fake up a per-subvolume FSID to get any sensible behavior.
If Rook/Kubernetes had the same experience, would that break things? Since they're sharing a single mount, but have a bunch of sub volumes? Would diverging between these clients for this API call be a problem?

So, I don't see f_fsid changes in the recent revision. Did we decide not to do that? If yes, can someone update why is that bit is not required?

This was removed as we didn't want to hold up the changes with statfs using path/inode provided. Discussion on f_fsid wasn't finalized or fully agreed on. We broke out this requirement here: https://tracker.ceph.com/issues/72575

src/client/Client.cc

ffilz · 2025-08-11T23:27:20Z

src/client/Client.cc

  stbuf->f_files = total_files_on_fs;
  stbuf->f_ffree = -1;
  stbuf->f_favail = -1;
-  stbuf->f_fsid = -1;       // ??


Yea, I'm not sure we have to deal with multiple FSes. Is there any way to make more unique?

ffilz · 2025-08-11T23:30:58Z

src/client/Client.cc

@@ -12529,24 +12529,25 @@ int Client::getcwd(string& dir, const UserPerm& perms)
 int Client::_statfs(Inode *in, struct statvfs *stbuf,
 		   const UserPerm& perms)
 {


I don't quite understand the changes in this patch, perhaps more description in the commit message?

In statfs, get quota_root for inode provided. Check if a quota is directly applied to inode. If not, reverse tree walk up and maybe find a quota set higher up the tree. Fixes: https://tracker.ceph.com/issues/72355 Signed-off-by: Christopher Hoffman <choffman@redhat.com>

batrick

good work!

batrick · 2025-08-13T13:59:28Z

	
CTest Failure

3 tests failed out of 318

Total Test time (real) = 3579.92 sec

The following tests FAILED:
	209 - unittest_bluefs (SEGFAULT)
	210 - unittest_bluefs_ex (Failed)
	212 - unittest_bdev (SEGFAULT)

batrick · 2025-08-13T13:59:40Z

jenkins test make check

batrick · 2025-08-13T13:59:43Z

jenkins test make check arm64

batrick · 2025-08-13T13:59:46Z

jenkins test windows

batrick · 2025-08-13T14:00:33Z

@chrisphoffman please open a ticket for discussion on f_fsid and summarize the slack discussion.

Signed-off-by: Christopher Hoffman <choffman@redhat.com>

chrisphoffman · 2025-08-13T14:24:48Z

I did one more minor repush.
What changed:
-Removed the fsid test previously included.
-Added more description to a commit message.

ffilz

Looks good now.

chrisphoffman · 2025-08-15T12:50:04Z

Testing patch here: https://pulpito.ceph.com/choffman-2025-08-15_12:05:12-fs-wip-choffman-72355-distro-default-smithi/

chrisphoffman · 2025-08-15T15:20:08Z

More general comprehensive test here: https://pulpito.ceph.com/choffman-2025-08-15_15:02:24-fs-wip-choffman-72355-distro-default-gibba/

During test_df_for_invalid_directory, path_walk is now called. Use a more general error message as more errnos can be returned and this will be a better catch all. Signed-off-by: Christopher Hoffman <choffman@redhat.com>

chrisphoffman · 2025-08-18T13:11:20Z

More general comprehensive test here: https://pulpito.ceph.com/choffman-2025-08-15_15:02:24-fs-wip-choffman-72355-distro-default-gibba/

One error pertaining to the change:https://pulpito.ceph.com/choffman-2025-08-15_15:02:24-fs-wip-choffman-72355-distro-default-gibba/8445011/
I am testing the fix now.

chrisphoffman · 2025-08-18T19:39:43Z

Testing under way here: https://pulpito.ceph.com/choffman-2025-08-18_16:12:52-fs-wip-choffman-72355-distro-default-smithi/

client: use path supplied in statfs

0c6a8ad

If path provided, use in statfs. Replumb internal statfs for internal only to allow for use in ll_statfs and statfs Fixes: https://tracker.ceph.com/issues/72355 Signed-off-by: Christopher Hoffman <choffman@redhat.com>

github-actions bot added cephfs Ceph File System tests labels Aug 6, 2025

chrisphoffman force-pushed the wip-choffman-72355 branch from 76144d7 to ddbeb2f Compare August 7, 2025 19:33

chrisphoffman requested a review from a team August 7, 2025 19:40

ffilz reviewed Aug 8, 2025

View reviewed changes

vshankar assigned batrick Aug 11, 2025

batrick requested changes Aug 11, 2025

View reviewed changes

chrisphoffman force-pushed the wip-choffman-72355 branch from ddbeb2f to 3ffa92f Compare August 11, 2025 14:06

chrisphoffman requested review from batrick and ffilz August 11, 2025 15:06

ffilz reviewed Aug 11, 2025

View reviewed changes

chrisphoffman force-pushed the wip-choffman-72355 branch from b64b164 to 699a1cf Compare August 12, 2025 19:56

batrick approved these changes Aug 13, 2025

View reviewed changes

batrick removed their assignment Aug 13, 2025

batrick added the needs-qa label Aug 13, 2025

chrisphoffman added 3 commits August 13, 2025 14:15

test: Add test for libcephfs statfs

61c13a9

Signed-off-by: Christopher Hoffman <choffman@redhat.com>

client: move mref_reader check in statfs out of helper

9ab1209

Signed-off-by: Christopher Hoffman <choffman@redhat.com>

client: bring client_lock out of statfs helper method

dd7bdd3

Signed-off-by: Christopher Hoffman <choffman@redhat.com>

chrisphoffman force-pushed the wip-choffman-72355 branch from 699a1cf to dd7bdd3 Compare August 13, 2025 14:22

ffilz approved these changes Aug 14, 2025

View reviewed changes

vshankar approved these changes Aug 14, 2025

View reviewed changes

tasks/cephfs: Use different errmsg for invalid dir

45776aa

During test_df_for_invalid_directory, path_walk is now called. Use a more general error message as more errnos can be returned and this will be a better catch all. Signed-off-by: Christopher Hoffman <choffman@redhat.com>

chrisphoffman merged commit 7b5fe62 into ceph:main Aug 19, 2025
13 checks passed

This was referenced Aug 19, 2025

tentacle: client: use path supplied in statfs #65132

Merged

squid: client: use path supplied in statfs #65133

Merged

Conversation

chrisphoffman commented Aug 6, 2025

Contribution Guidelines

Checklist

Uh oh!

ffilz left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

mattbenjamin Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

batrick commented Aug 13, 2025

Uh oh!

batrick commented Aug 13, 2025

Uh oh!

batrick commented Aug 13, 2025

Uh oh!

batrick commented Aug 13, 2025

Uh oh!

batrick commented Aug 13, 2025

Uh oh!

chrisphoffman commented Aug 13, 2025

Uh oh!

ffilz left a comment

Choose a reason for hiding this comment

Uh oh!

chrisphoffman commented Aug 15, 2025

Uh oh!

chrisphoffman commented Aug 15, 2025

Uh oh!

chrisphoffman commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisphoffman commented Aug 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

mattbenjamin Aug 12, 2025 •

edited

Loading

chrisphoffman commented Aug 18, 2025 •

edited

Loading