| Message ID | 172437089432.61495.8117184114353548540.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | [1/6] xfs: refactor xfs_qm_destroy_quotainos | expand |
On Thu, Aug 22, 2024 at 05:28:59PM -0700, Darrick J. Wong wrote: > Starting with metadir, let's change the behavior so that if the user > does not specify any quota-related mount options at all, the ondisk > quota flags will be used to bring up quota. In other words, the > filesystem will mount in the same state and with the same functionality > as it had during the last mount. Finally! Are you going to send some tests that test this behavior? Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, Aug 22, 2024 at 10:54:51PM -0700, Christoph Hellwig wrote: > On Thu, Aug 22, 2024 at 05:28:59PM -0700, Darrick J. Wong wrote: > > Starting with metadir, let's change the behavior so that if the user > > does not specify any quota-related mount options at all, the ondisk > > quota flags will be used to bring up quota. In other words, the > > filesystem will mount in the same state and with the same functionality > > as it had during the last mount. > > Finally! > > Are you going to send some tests that test this behavior? > > Reviewed-by: Christoph Hellwig <hch@lst.de> Yes. --D xfs: test persistent quota flags Test the persistent quota flags that come with the metadir feature. Signed-off-by: Darrick J. Wong <djwong@kernel.org> --- tests/xfs/1891 | 128 +++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/1891.out | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 282 insertions(+), 1 deletion(-) create mode 100755 tests/xfs/1891 create mode 100644 tests/xfs/1891.out diff --git a/tests/xfs/1891 b/tests/xfs/1891 new file mode 100755 index 0000000000..53009571a9 --- /dev/null +++ b/tests/xfs/1891 @@ -0,0 +1,128 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2024 Oracle. All Rights Reserved. +# +# FS QA Test 1891 +# +# Functionality test for persistent quota accounting and enforcement flags in +# XFS when metadata directories are enabled. +# +. ./common/preamble +_begin_fstest auto quick quota + +. ./common/filter +. ./common/quota + +$MKFS_XFS_PROG 2>&1 | grep -q 'uquota' || \ + _notrun "mkfs does not support uquota option" + +_require_scratch +_require_xfs_quota + +filter_quota_state() { + sed -e 's/Inode: #[0-9]\+/Inode #XXX/g' \ + -e '/max warnings:/d' \ + -e '/Blocks grace time:/d' \ + -e '/Inodes grace time:/d' \ + | _filter_scratch +} + +qerase_mkfs_options() { + echo "$MKFS_OPTIONS" | sed \ + -e 's/uquota//g' \ + -e 's/gquota//g' \ + -e 's/pquota//g' \ + -e 's/uqnoenforce//g' \ + -e 's/gqnoenforce//g' \ + -e 's/pqnoenforce//g' \ + -e 's/,,*/,/g' +} + +confirm() { + echo "$MOUNT_OPTIONS" | grep -E -q '(qnoenforce|quota)' && \ + echo "saw quota mount options" + _scratch_mount + $XFS_QUOTA_PROG -x -c "state -ugp" $SCRATCH_MNT | filter_quota_state + _check_xfs_scratch_fs + _scratch_unmount +} + +ORIG_MOUNT_OPTIONS="$MOUNT_OPTIONS" +MKFS_OPTIONS="$(qerase_mkfs_options)" + +echo "Test 0: formatting a subset" +_scratch_mkfs -m uquota,gqnoenforce &>> $seqres.full +MOUNT_OPTIONS="$ORIG_MOUNT_OPTIONS" +_qmount_option # blank out quota options +confirm + +echo "Test 1: formatting" +_scratch_mkfs -m uquota,gquota,pquota &>> $seqres.full +MOUNT_OPTIONS="$ORIG_MOUNT_OPTIONS" +_qmount_option # blank out quota options +confirm + +echo "Test 2: only grpquota" +MOUNT_OPTIONS="$ORIG_MOUNT_OPTIONS" +_qmount_option grpquota +confirm + +echo "Test 3: repair" +_scratch_xfs_repair &>> $seqres.full || echo "repair failed?" +MOUNT_OPTIONS="$ORIG_MOUNT_OPTIONS" +_qmount_option # blank out quota options +confirm + +echo "Test 4: weird options" +MOUNT_OPTIONS="$ORIG_MOUNT_OPTIONS" +_qmount_option pqnoenforce,uquota +confirm + +echo "Test 5: simple recovery" +_scratch_mkfs -m uquota,gquota,pquota &>> $seqres.full +MOUNT_OPTIONS="$ORIG_MOUNT_OPTIONS" +_qmount_option # blank out quota options +echo "$MOUNT_OPTIONS" | grep -E -q '(qnoenforce|quota)' && \ + echo "saw quota mount options" +_scratch_mount +$XFS_QUOTA_PROG -x -c "state -ugp" $SCRATCH_MNT | filter_quota_state +touch $SCRATCH_MNT/a +_scratch_shutdown -v -f >> $seqres.full +echo shutdown +_scratch_unmount +confirm + +echo "Test 6: simple recovery with mount options" +_scratch_mkfs -m uquota,gquota,pquota &>> $seqres.full +MOUNT_OPTIONS="$ORIG_MOUNT_OPTIONS" +_qmount_option # blank out quota options +echo "$MOUNT_OPTIONS" | grep -E -q '(qnoenforce|quota)' && \ + echo "saw quota mount options" +_scratch_mount +$XFS_QUOTA_PROG -x -c "state -ugp" $SCRATCH_MNT | filter_quota_state +touch $SCRATCH_MNT/a +_scratch_shutdown -v -f >> $seqres.full +echo shutdown +_scratch_unmount +MOUNT_OPTIONS="$ORIG_MOUNT_OPTIONS" +_qmount_option gqnoenforce +confirm + +echo "Test 7: user quotaoff recovery" +_scratch_mkfs -m uquota,gquota,pquota &>> $seqres.full +MOUNT_OPTIONS="$ORIG_MOUNT_OPTIONS" +_qmount_option # blank out quota options +echo "$MOUNT_OPTIONS" | grep -E -q '(qnoenforce|quota)' && \ + echo "saw quota mount options" +_scratch_mount +$XFS_QUOTA_PROG -x -c "state -ugp" $SCRATCH_MNT | filter_quota_state +touch $SCRATCH_MNT/a +$XFS_QUOTA_PROG -x -c 'off -u' $SCRATCH_MNT +_scratch_shutdown -v -f >> $seqres.full +echo shutdown +_scratch_unmount +confirm + +# success, all done +status=0 +exit diff --git a/tests/xfs/1891.out b/tests/xfs/1891.out new file mode 100644 index 0000000000..7e88940880 --- /dev/null +++ b/tests/xfs/1891.out @@ -0,0 +1,147 @@ +QA output created by 1891 +Test 0: formatting a subset +User quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Group quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: OFF + Inode #XXX (1 blocks, 1 extents) +Project quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: OFF + Enforcement: OFF + Inode: N/A +Test 1: formatting +User quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Group quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Project quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Test 2: only grpquota +saw quota mount options +User quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: OFF + Enforcement: OFF + Inode #XXX (1 blocks, 1 extents) +Group quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Project quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: OFF + Enforcement: OFF + Inode #XXX (1 blocks, 1 extents) +Test 3: repair +User quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: OFF + Enforcement: OFF + Inode #XXX (1 blocks, 1 extents) +Group quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Project quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: OFF + Enforcement: OFF + Inode #XXX (1 blocks, 1 extents) +Test 4: weird options +saw quota mount options +User quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Group quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: OFF + Enforcement: OFF + Inode #XXX (1 blocks, 1 extents) +Project quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: OFF + Inode #XXX (1 blocks, 1 extents) +Test 5: simple recovery +User quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Group quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Project quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +shutdown +User quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Group quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Project quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Test 6: simple recovery with mount options +User quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Group quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Project quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +shutdown +saw quota mount options +User quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: OFF + Enforcement: OFF + Inode #XXX (1 blocks, 1 extents) +Group quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: OFF + Inode #XXX (1 blocks, 1 extents) +Project quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: OFF + Enforcement: OFF + Inode #XXX (1 blocks, 1 extents) +Test 7: user quotaoff recovery +User quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Group quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Project quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +shutdown +User quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: OFF + Inode #XXX (1 blocks, 1 extents) +Group quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents) +Project quota state on SCRATCH_MNT (SCRATCH_DEV) + Accounting: ON + Enforcement: ON + Inode #XXX (1 blocks, 1 extents)
On Thu, Aug 22, 2024 at 05:28:59PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > It's annoying that one has to keep reminding XFS about what quota > options it should mount with, since the quota flags recording the > previous state are sitting right there in the primary superblock. Even > more strangely, there exists a noquota option to disable quotas > completely, so it's odder still that providing no options is the same as > noquota. > > Starting with metadir, let's change the behavior so that if the user > does not specify any quota-related mount options at all, the ondisk > quota flags will be used to bring up quota. In other words, the > filesystem will mount in the same state and with the same functionality > as it had during the last mount. This means the only way to switch quota off completely with this functionality is to explicitly unmount the filesystem and then mount it again with the "-o noquota" option instead of mounting it again without any quota options. If so, this will need clear documentation in various man pages because users will not expect this change of quota admin behaviour caused by enabling some other unrelated functionality (like rtgroups)..... -Dave.
On Mon, Aug 26, 2024 at 07:42:48PM +1000, Dave Chinner wrote: > On Thu, Aug 22, 2024 at 05:28:59PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > It's annoying that one has to keep reminding XFS about what quota > > options it should mount with, since the quota flags recording the > > previous state are sitting right there in the primary superblock. Even > > more strangely, there exists a noquota option to disable quotas > > completely, so it's odder still that providing no options is the same as > > noquota. > > > > Starting with metadir, let's change the behavior so that if the user > > does not specify any quota-related mount options at all, the ondisk > > quota flags will be used to bring up quota. In other words, the > > filesystem will mount in the same state and with the same functionality > > as it had during the last mount. > > This means the only way to switch quota off completely with this > functionality is to explicitly unmount the filesystem and then mount > it again with the "-o noquota" option instead of mounting it again > without any quota options. > > If so, this will need clear documentation in various man pages > because users will not expect this change of quota admin behaviour > caused by enabling some other unrelated functionality (like > rtgroups)..... Yeah, manpage updates are in progress. --D > -Dave. > -- > Dave Chinner > david@fromorbit.com >
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 5726ea597f5a2..cbf47354561c1 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -850,6 +850,13 @@ xfs_mountfs( if (error) goto out_fail_wait; + /* + * If we're resuming quota status, pick up the preliminary qflags from + * the ondisk superblock so that we know if we should recover dquots. + */ + if (xfs_is_resuming_quotaon(mp)) + xfs_qm_resume_quotaon(mp); + /* * Log's mount-time initialization. The first part of recovery can place * some items on the AIL, to be handled when recovery is finished or @@ -863,6 +870,14 @@ xfs_mountfs( goto out_inodegc_shrinker; } + /* + * If we're resuming quota status and recovered the log, re-sample the + * qflags from the ondisk superblock now that we've recovered it, just + * in case someone shut down enforcement just before a crash. + */ + if (xfs_clear_resuming_quotaon(mp) && xlog_recovery_needed(mp->m_log)) + xfs_qm_resume_quotaon(mp); + /* * If logged xattrs are still enabled after log recovery finishes, then * they'll be available until unmount. Otherwise, turn them off. diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h index 7e68812db1be7..ba9af63aec143 100644 --- a/fs/xfs/xfs_mount.h +++ b/fs/xfs/xfs_mount.h @@ -459,6 +459,8 @@ __XFS_HAS_FEAT(nouuid, NOUUID) #define XFS_OPSTATE_UNSET_LOG_INCOMPAT 11 /* Filesystem can use logged extended attributes */ #define XFS_OPSTATE_USE_LARP 12 +/* Filesystem should use qflags to determine quotaon status */ +#define XFS_OPSTATE_RESUMING_QUOTAON 13 #define __XFS_IS_OPSTATE(name, NAME) \ static inline bool xfs_is_ ## name (struct xfs_mount *mp) \ @@ -483,8 +485,12 @@ __XFS_IS_OPSTATE(inodegc_enabled, INODEGC_ENABLED) __XFS_IS_OPSTATE(blockgc_enabled, BLOCKGC_ENABLED) #ifdef CONFIG_XFS_QUOTA __XFS_IS_OPSTATE(quotacheck_running, QUOTACHECK_RUNNING) +__XFS_IS_OPSTATE(resuming_quotaon, RESUMING_QUOTAON) #else # define xfs_is_quotacheck_running(mp) (false) +# define xfs_is_resuming_quotaon(mp) (false) +# define xfs_set_resuming_quotaon(mp) (false) +# define xfs_clear_resuming_quotaon(mp) (false) #endif __XFS_IS_OPSTATE(done_with_log_incompat, UNSET_LOG_INCOMPAT) __XFS_IS_OPSTATE(using_logged_xattrs, USE_LARP) diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c index a11436579877d..79a96558f739e 100644 --- a/fs/xfs/xfs_qm_bhv.c +++ b/fs/xfs/xfs_qm_bhv.c @@ -135,3 +135,21 @@ xfs_qm_newmount( return 0; } + +/* + * If the sysadmin didn't provide any quota mount options, restore the quota + * accounting and enforcement state from the ondisk superblock. Only do this + * for metadir filesystems because this is a behavior change. + */ +void +xfs_qm_resume_quotaon( + struct xfs_mount *mp) +{ + if (!xfs_has_metadir(mp)) + return; + if (xfs_has_norecovery(mp)) + return; + + mp->m_qflags = mp->m_sb.sb_qflags & (XFS_ALL_QUOTA_ACCT | + XFS_ALL_QUOTA_ENFD); +} diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h index 645761997bf2d..2d36d967380e7 100644 --- a/fs/xfs/xfs_quota.h +++ b/fs/xfs/xfs_quota.h @@ -125,6 +125,7 @@ extern void xfs_qm_dqdetach(struct xfs_inode *); extern void xfs_qm_dqrele(struct xfs_dquot *); extern void xfs_qm_statvfs(struct xfs_inode *, struct kstatfs *); extern int xfs_qm_newmount(struct xfs_mount *, uint *, uint *); +void xfs_qm_resume_quotaon(struct xfs_mount *mp); extern void xfs_qm_mount_quotas(struct xfs_mount *); extern void xfs_qm_unmount(struct xfs_mount *); extern void xfs_qm_unmount_quotas(struct xfs_mount *); @@ -202,6 +203,7 @@ xfs_trans_reserve_quota_icreate(struct xfs_trans *tp, struct xfs_dquot *udqp, #define xfs_qm_dqrele(d) do { (d) = (d); } while(0) #define xfs_qm_statvfs(ip, s) do { } while(0) #define xfs_qm_newmount(mp, a, b) (0) +#define xfs_qm_resume_quotaon(mp) ((void)0) #define xfs_qm_mount_quotas(mp) #define xfs_qm_unmount(mp) #define xfs_qm_unmount_quotas(mp) diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 835886c322a83..d02bfe9ddfe58 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -67,6 +67,9 @@ enum xfs_dax_mode { XFS_DAX_NEVER = 2, }; +/* Were quota mount options provided? Must use the upper 16 bits of qflags. */ +#define XFS_QFLAGS_MNTOPTS (1U << 31) + static void xfs_mount_set_dax_mode( struct xfs_mount *mp, @@ -1264,6 +1267,8 @@ xfs_fs_parse_param( int size = 0; int opt; + BUILD_BUG_ON(XFS_QFLAGS_MNTOPTS & XFS_MOUNT_QUOTA_ALL); + opt = fs_parse(fc, xfs_fs_parameters, param, &result); if (opt < 0) return opt; @@ -1341,32 +1346,39 @@ xfs_fs_parse_param( case Opt_noquota: parsing_mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT; parsing_mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD; + parsing_mp->m_qflags |= XFS_QFLAGS_MNTOPTS; return 0; case Opt_quota: case Opt_uquota: case Opt_usrquota: parsing_mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ENFD); + parsing_mp->m_qflags |= XFS_QFLAGS_MNTOPTS; return 0; case Opt_qnoenforce: case Opt_uqnoenforce: parsing_mp->m_qflags |= XFS_UQUOTA_ACCT; parsing_mp->m_qflags &= ~XFS_UQUOTA_ENFD; + parsing_mp->m_qflags |= XFS_QFLAGS_MNTOPTS; return 0; case Opt_pquota: case Opt_prjquota: parsing_mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ENFD); + parsing_mp->m_qflags |= XFS_QFLAGS_MNTOPTS; return 0; case Opt_pqnoenforce: parsing_mp->m_qflags |= XFS_PQUOTA_ACCT; parsing_mp->m_qflags &= ~XFS_PQUOTA_ENFD; + parsing_mp->m_qflags |= XFS_QFLAGS_MNTOPTS; return 0; case Opt_gquota: case Opt_grpquota: parsing_mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ENFD); + parsing_mp->m_qflags |= XFS_QFLAGS_MNTOPTS; return 0; case Opt_gqnoenforce: parsing_mp->m_qflags |= XFS_GQUOTA_ACCT; parsing_mp->m_qflags &= ~XFS_GQUOTA_ENFD; + parsing_mp->m_qflags |= XFS_QFLAGS_MNTOPTS; return 0; case Opt_discard: parsing_mp->m_features |= XFS_FEAT_DISCARD; @@ -1761,6 +1773,14 @@ xfs_fs_fill_super( xfs_warn(mp, "EXPERIMENTAL parent pointer feature enabled. Use at your own risk!"); + /* + * If no quota mount options were provided, maybe we'll try to pick + * up the quota accounting and enforcement flags from the ondisk sb. + */ + if (!(mp->m_qflags & XFS_QFLAGS_MNTOPTS)) + xfs_set_resuming_quotaon(mp); + mp->m_qflags &= ~XFS_QFLAGS_MNTOPTS; + error = xfs_mountfs(mp); if (error) goto out_filestream_unmount; @@ -1947,6 +1967,8 @@ xfs_fs_reconfigure( int flags = fc->sb_flags; int error; + new_mp->m_qflags &= ~XFS_QFLAGS_MNTOPTS; + /* version 5 superblocks always support version counters. */ if (xfs_has_crc(mp)) fc->sb_flags |= SB_I_VERSION;