Page MenuHomeFreeBSD

VFS_QUOTACTL(9): allow implementation to indicate busy state changes
ClosedPublic

Authored by jah on May 11 2021, 7:03 PM.

Details

Summary

Instead of requiring all implementations of vfs_quotactl to unbusy
the mount for Q_QUOTAON and Q_QUOTAOFF, add an "mp_busy" in/out param
to VFS_QUOTACTL(9). The implementation may then indicate to the caller
whether it needed to unbusy the mount.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 39345
Build 36234: arc lint + arc unit

Event Timeline

jah requested review of this revision.May 11 2021, 7:03 PM
sys/kern/vfs_default.c
1335

Instead of doubling down on the existing approach, would it be worthwhile to change the interface to VFS_QUOTACTL() so that it takes a "bool *mp_busy" param that allows the implementation to indicate whether the mount busy state was changed? That's not pretty, but it seems a bit more robust than what we have now.

sys/kern/vfs_default.c
1335

bool **mp_busy perhaps?
Yes it sounds much better than the current code. In fact, current code is targeted for single implementation pf VFS_QUOTACTL() in UFS.

bcr added a subscriber: bcr.

OK from manpages.

Allow the implementation to direct busy state changes

sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vfsops.c
109

I believe that I really need to make these ZFS changes conditional on the new __FreeBSD_version and open a PR against the upstream OpenZFS github, is that correct?

jah retitled this revision from Follow mount point unbusying requirements in vfs_stdquotactl() to VFS_QUOTACTL(9): allow implementation to indicate busy state changes.May 13 2021, 2:10 AM
jah edited the summary of this revision. (Show Details)
jah added a reviewer: ZFS.
share/man/man9/VFS_QUOTACTL.9
62

New sentence should start on the new line.

sys/kern/vfs_default.c
65

Why is this needed?

sys/ufs/ufs/ufs_quota.c
509–511

This chunk is strange. Shouldn't you return error there, instead of unbusying?

Fix manpage formatting, remove leftover include, fix incorrect deleted line due to fingerslip

sys/fs/nullfs/null_vfsops.c
304

Doesn't mp_busy describe the state of the nullfs mount rather than the target mount? Ditto for unionfs.

sys/fs/nullfs/null_vfsops.c
304

Yes, that's right. For unionfs this should already be handled by D30152. It looks like nullfs will need something similar to the ref+busy helpers in D30152, as I don't see that it does anything to ensure the underlying mount sticks around.

freqlabs added inline comments.
sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vfsops.c
109

Yes, please and thank you.

sys/fs/nullfs/null_vfsops.c
304

May be, the right thing to do for stacked filesystems is to disable quota handling from upper at all. For instance, you cannot change async option from upper, why any other administrative setup of lower should be affected?

sys/fs/nullfs/null_vfsops.c
304

Maybe?

From my naive perspective as a VFS newbie, this seems like a difference between something that's an attribute of the mount vs. an attribute of the filesystem. Since the async flag pertains to a specific mount, it makes sense to allow it to be changed only by directly operating on that mount. To me quotas seem more like an attribute of the filesystem. Since stacked filesystems are intended to subsume the attributes of their lower filesystems in order to provide additional functionality, it at least doesn't seem wrong to allow them to pass quota operations to the lower filesystems. In a hypothetical world where quotactl was more generic and less UFS-specific, I could imagine a stacked filesystem wanting to do its own quota management before passing the request to the lower FS.

sys/fs/nullfs/null_vfsops.c
304

I have somewhat different perspective there.

I would think that administrative configuration of the lower filesystem should be only done on lower mp. Upper mp should provide a view into lower, but otherwise it generally should be not allowed to configure it.

This is why I provided the async option as an example of administratively controlled consistency configuration, and I think that quota controls are same. If anything, quota control on mount should control quotas on upper, which would be then strict union of lower and upper limits. But so far nobody asked for this (quotas are rarely used anyway).

sys/fs/nullfs/null_vfsops.c
304

I think we can always do that in a later change, right?

It seems that nullfs needs some fixes to the way it handles the lower mount regardless of quotactl, so I plan to do that as a separate change to be committed at the same time as this and D30152.

Add basic fixes for nullfs and unionfs, conditionalize ZFS changes

In D30218#680984, @jah wrote:

Add basic fixes for nullfs and unionfs, conditionalize ZFS changes

After talking with kib, I've decided to replace D30152 and D30263 with a simpler approach, which will be done in a separate change.
Accordingly I've made the basic quotactl fixes for unionfs and nullfs in this branch, to keep it self contained and avoid the need to be submitted in lockstep with any other changes (with the possible exception of the upstream OpenZFS change)
It's still worth considering whether quotactl should be supported at all for stacked upper filesystems, but I think any change there can be made later.

sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vfsops.c
109
sys/fs/nullfs/null_vfsops.c
309

All these empty lines are not needed/not allowed by style.

314

It is not safe to call into lower QUOTACTL while still having the upper mount busy due to namei()/open().

If you ref/unbusy upper, then you can stop using MBF_NOWAIT for busying lower.

unbusy the unionfs/nullfs mount before calling QUOTACTL on the lower FS

This revision is now accepted and ready to land.May 22 2021, 8:45 AM
markj added inline comments.
sys/fs/nullfs/null_vfsops.c
314

I would suggest adding a comment explaining why it's not ok to hold both mount points busy.

Add comments around unbusying requirements, chase __FreeBSD_version changes

This revision now requires review to proceed.May 25 2021, 6:26 PM
This revision was not accepted when it landed; it landed in state Needs Review.May 29 2021, 9:05 PM
This revision was automatically updated to reflect the committed changes.