Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix assignment of quota IDs for XFS quotas #1921

Merged
merged 1 commit into from
May 21, 2024

Conversation

mheon
Copy link
Member

@mheon mheon commented May 13, 2024

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

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

Signed-off-by: Matt Heon <mheon@redhat.com>
@mheon
Copy link
Member Author

mheon commented May 14, 2024

@nalind @rhatdan PTAL

@rhatdan
Copy link
Member

rhatdan commented May 14, 2024

LGTM
@giuseppe PTAL

@rhatdan
Copy link
Member

rhatdan commented May 14, 2024

@giuseppe @mtrmac PTAL

@giuseppe
Copy link
Member

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
Copy link
Member Author

mheon commented May 15, 2024

@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
Copy link
Member

@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
Copy link
Collaborator

mtrmac commented May 16, 2024

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
Copy link
Member

rhatdan commented May 16, 2024

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.

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

openshift-ci bot commented May 17, 2024

[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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mheon
Copy link
Member Author

mheon commented May 20, 2024

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
Copy link
Member

rhatdan commented May 21, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label May 21, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 554f5ca into containers:main May 21, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants