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

[jah]

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.

[jah]

sys/kern/vfs_default.c

1364

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.

[kib]

sys/kern/vfs_default.c

1364

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]

OK from manpages.

[jah]

Allow the implementation to direct busy state changes

[jah]

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

110

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?

[kib]

share/man/man9/VFS_QUOTACTL.9

63

New sentence should start on the new line.

sys/kern/vfs_default.c

66

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?

[jah]

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

[markj]

sys/fs/nullfs/null_vfsops.c

305

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

[jah]

sys/fs/nullfs/null_vfsops.c

305

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]

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

110

Yes, please and thank you.

[kib]

sys/fs/nullfs/null_vfsops.c

305

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?

[jah]

sys/fs/nullfs/null_vfsops.c

305

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.

[kib]

sys/fs/nullfs/null_vfsops.c

305

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).

[jah]

sys/fs/nullfs/null_vfsops.c

305

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.

[jah]

Add basic fixes for nullfs and unionfs, conditionalize ZFS changes

[jah]

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.

[jah]

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

110

https://github.com/openzfs/zfs/pull/12052

[kib]

sys/fs/nullfs/null_vfsops.c

310

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

315

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.

[jah]

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

[markj]

sys/fs/nullfs/null_vfsops.c

315

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

[jah]

Add comments around unbusying requirements, chase __FreeBSD_version changes