containers / storage

Container Storage Library
Apache License 2.0
539 stars 234 forks source link

Fix assignment of quota IDs for XFS quotas #1921

Closed mheon closed 1 month ago

mheon commented 1 month ago

It appears that setting the FS_XFLAG_PROJINHERIT flag when assigning a quota to a directory overrides any requested project ID set (even in the same ioctl!) if a parent directory already has a project ID. And our parent directories always do, as we set the top-level directory to the base project ID for all volumes.

The result of this is that the inherit flag forces every subdirectory to share the same project ID, and thus every time we set a quota, it's set for everything using quotas - not just the single directory we wanted to set it on.

Lacking the inherit flag made me a bit worried about subdirectories, but I can confirm that the quotas themselves are still function (creating a subdirectory in a dir with a quota set and creating files in that directory still applies their size towards the quota). There's a chance it would impact doing a separate project ID in a subdirectory of a subdirectory but from my read of the code that isn't supported at all right now.

Fixes RH Jira RHEL-18038

mheon commented 1 month ago

@nalind @rhatdan PTAL

rhatdan commented 1 month ago

LGTM @giuseppe PTAL

rhatdan commented 1 month ago

@giuseppe @mtrmac PTAL

giuseppe commented 1 month ago

we took this file from Moby where the INHERIT flag is still used:

https://github.com/moby/moby/blob/master/quota/projectquota.go#L312

I think we should keep the current behavior for overlay.

Could we add a new function if libpod volumes require the quota without INHERIT set?

Fixes RH Jira RHEL-18038

It seems I've no permissions to access the Jira card

mheon commented 1 month ago

@giuseppe Is there a guide somewhere on how to work with the overlay driver quotas? I suspect from what I'm seeing they may also be broken, so I'd like to check.

giuseppe commented 1 month ago

@giuseppe Is there a guide somewhere on how to work with the overlay driver quotas? I suspect from what I'm seeing they may also be broken, so I'd like to check.

I am aware only of the man page.

Looking into the code more, it seems the quota is applied only to empty directories (through create), so for overlay it should not matter either

mtrmac commented 1 month ago

I can’t find any authoritative-looking documentation of the FS_XFLAG_PROJINHERIT flag.

Looking at the code, at least https://github.com/torvalds/linux/blob/ea5f6ad9ad9645733b72ab53a98e719b460d36a6/fs/xfs/xfs_inode.h#L274 suggests that we do need that flag set on the directories we create.

Also https://man7.org/linux/man-pages/man8/xfs_quota.8.html#DIRECTORY_TREE_QUOTA and https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/tree/quota/project.c#n117 seem to confirm that the expected shape of a project-quota directory tree is that all directories have the flag set.

I know nothing about this problem space, I didn’t try anything in practice, and I’m perfectly happy to stay out of this PR.

If I should get involved, my starting hypothesis would be that we need that flag set, and then either look to invalidate that hypothesis, or to investigate why the code setting the flag + project ID is failing, and how to change it so that it succeeds (Compare https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/tree/quota/project.c#n209 ).

One intriguing note is https://github.com/torvalds/linux/blob/ea5f6ad9ad9645733b72ab53a98e719b460d36a6/fs/ioctl.c#L603-L607 , but if this were the cause we should have seen a failure.

rhatdan commented 1 month ago

Could @haircommander @saschagrunert Take a look, I think CRI-O uses some of this code, most likely we can just merge, but CRI-O should know this change is coming.

openshift-ci[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/containers/storage/blob/main/OWNERS)~~ [saschagrunert] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
mheon commented 1 month ago

After further investigation, I think removal is the only option unless we can find some documentation that offers major clarification to how the flag is supposed to be used. FS_XFLAG_PROJINHERIT when present always uses the project ID of the parent directory, overriding any project ID we set ourselves, even if that project ID is set in a separate ioctl call (before or after the PROJINHERIT flag is set makes no different; once the flag is set, we always use the parent's project ID). Based on this, my assumption is that it was probably meant to be used in a case where the parent does not have a project ID set, only subdirectories, and those subdirectories are sticky to ensure there is no escape from the given project ID? But that assumption is a complete mismatch to how the code is written right now, so I have no idea what's going on?

In short, I still have confidence in this fix, but I'm not sure of how this was originally meant to work.

rhatdan commented 1 month ago

/lgtm