Re: [PATCH 1/3] xfs: remove support for disabling quota accounting on a mounted file system
From: "Darrick J. Wong" <djwong@kernel.org>
Date: 2021-07-15 17:15:50
On Mon, Jul 12, 2021 at 01:14:24PM +0200, Christoph Hellwig wrote:
Disabling quota accounting is hairy, racy code with all kinds of pitfalls. And it has a very strange mind set, as quota accounting (unlike enforcement) really is a propery of the on-disk format. There is no good use case for supporting this. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_trans_resv.c | 30 ---- fs/xfs/libxfs/xfs_trans_resv.h | 2 - fs/xfs/xfs_dquot_item.c | 134 ------------------ fs/xfs/xfs_dquot_item.h | 17 --- fs/xfs/xfs_qm.c | 2 +- fs/xfs/xfs_qm.h | 3 - fs/xfs/xfs_qm_syscalls.c | 241 ++------------------------------- fs/xfs/xfs_trans_dquot.c | 38 ------ 8 files changed, 13 insertions(+), 454 deletions(-)
<snip> since Christoph didn't find the reply the first time...
quoted hunk ↗ jump to hunk
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c index 13a56e1ea15ce1..e00067a142a094 100644 --- a/fs/xfs/xfs_qm_syscalls.c +++ b/fs/xfs/xfs_qm_syscalls.c
<snip>
quoted hunk ↗ jump to hunk
@@ -111,160 +31,23 @@ xfs_qm_scall_quotaoff( */ if ((mp->m_qflags & flags) == 0) return -EEXIST; - error = 0; - - flags &= (XFS_ALL_QUOTA_ACCT | XFS_ALL_QUOTA_ENFD); /* - * We don't want to deal with two quotaoffs messing up each other, - * so we're going to serialize it. quotaoff isn't exactly a performance - * critical thing. - * If quotaoff, then we must be dealing with the root filesystem. + * We do not support actually turning off quota accounting any more. + * Just log a warning and ignored the accounting related flags.
s/ignored/ignore/
*/ - ASSERT(q); - mutex_lock(&q->qi_quotaofflock); + if (flags & XFS_ALL_QUOTA_ACCT) + xfs_info(mp, "disabling of quota accounting not supported.");
Why not return EOPNOTSUPP here? We're not going to turn off accounting, so we're not doing what the admin asked. --D
quoted hunk ↗ jump to hunk
- /* - * If we're just turning off quota enforcement, change mp and go. - */ - if ((flags & XFS_ALL_QUOTA_ACCT) == 0) { - mp->m_qflags &= ~(flags); - - spin_lock(&mp->m_sb_lock); - mp->m_sb.sb_qflags = mp->m_qflags; - spin_unlock(&mp->m_sb_lock); - mutex_unlock(&q->qi_quotaofflock); - - /* XXX what to do if error ? Revert back to old vals incore ? */ - return xfs_sync_sb(mp, false); - } - - dqtype = 0; - inactivate_flags = 0; - /* - * If accounting is off, we must turn enforcement off, clear the - * quota 'CHKD' certificate to make it known that we have to - * do a quotacheck the next time this quota is turned on. - */ - if (flags & XFS_UQUOTA_ACCT) { - dqtype |= XFS_QMOPT_UQUOTA; - flags |= (XFS_UQUOTA_CHKD | XFS_UQUOTA_ENFD); - inactivate_flags |= XFS_UQUOTA_ACTIVE; - } - if (flags & XFS_GQUOTA_ACCT) { - dqtype |= XFS_QMOPT_GQUOTA; - flags |= (XFS_GQUOTA_CHKD | XFS_GQUOTA_ENFD); - inactivate_flags |= XFS_GQUOTA_ACTIVE; - } - if (flags & XFS_PQUOTA_ACCT) { - dqtype |= XFS_QMOPT_PQUOTA; - flags |= (XFS_PQUOTA_CHKD | XFS_PQUOTA_ENFD); - inactivate_flags |= XFS_PQUOTA_ACTIVE; - } - - /* - * Nothing to do? Don't complain. This happens when we're just - * turning off quota enforcement. - */ - if ((mp->m_qflags & flags) == 0) - goto out_unlock; - - /* - * Write the LI_QUOTAOFF log record, and do SB changes atomically, - * and synchronously. If we fail to write, we should abort the - * operation as it cannot be recovered safely if we crash. - */ - error = xfs_qm_log_quotaoff(mp, &qoffstart, flags); - if (error) - goto out_unlock; - - /* - * Next we clear the XFS_MOUNT_*DQ_ACTIVE bit(s) in the mount struct - * to take care of the race between dqget and quotaoff. We don't take - * any special locks to reset these bits. All processes need to check - * these bits *after* taking inode lock(s) to see if the particular - * quota type is in the process of being turned off. If *ACTIVE, it is - * guaranteed that all dquot structures and all quotainode ptrs will all - * stay valid as long as that inode is kept locked. - * - * There is no turning back after this. - */ - mp->m_qflags &= ~inactivate_flags; - - /* - * Give back all the dquot reference(s) held by inodes. - * Here we go thru every single incore inode in this file system, and - * do a dqrele on the i_udquot/i_gdquot that it may have. - * Essentially, as long as somebody has an inode locked, this guarantees - * that quotas will not be turned off. This is handy because in a - * transaction once we lock the inode(s) and check for quotaon, we can - * depend on the quota inodes (and other things) being valid as long as - * we keep the lock(s). - */ - error = xfs_dqrele_all_inodes(mp, flags); - ASSERT(!error); - - /* - * Next we make the changes in the quota flag in the mount struct. - * This isn't protected by a particular lock directly, because we - * don't want to take a mrlock every time we depend on quotas being on. - */ - mp->m_qflags &= ~flags; - - /* - * Go through all the dquots of this file system and purge them, - * according to what was turned off. - */ - xfs_qm_dqpurge_all(mp, dqtype); - - /* - * Transactions that had started before ACTIVE state bit was cleared - * could have logged many dquots, so they'd have higher LSNs than - * the first QUOTAOFF log record does. If we happen to crash when - * the tail of the log has gone past the QUOTAOFF record, but - * before the last dquot modification, those dquots __will__ - * recover, and that's not good. - * - * So, we have QUOTAOFF start and end logitems; the start - * logitem won't get overwritten until the end logitem appears... - */ - error = xfs_qm_log_quotaoff_end(mp, &qoffstart, flags); - if (error) { - /* We're screwed now. Shutdown is the only option. */ - xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE); - goto out_unlock; - } - - /* - * If all quotas are completely turned off, close shop. - */ - if (mp->m_qflags == 0) { - mutex_unlock(&q->qi_quotaofflock); - xfs_qm_destroy_quotainfo(mp); - return 0; - } - - /* - * Release our quotainode references if we don't need them anymore. - */ - if ((dqtype & XFS_QMOPT_UQUOTA) && q->qi_uquotaip) { - xfs_irele(q->qi_uquotaip); - q->qi_uquotaip = NULL; - } - if ((dqtype & XFS_QMOPT_GQUOTA) && q->qi_gquotaip) { - xfs_irele(q->qi_gquotaip); - q->qi_gquotaip = NULL; - } - if ((dqtype & XFS_QMOPT_PQUOTA) && q->qi_pquotaip) { - xfs_irele(q->qi_pquotaip); - q->qi_pquotaip = NULL; - } + mutex_lock(&mp->m_quotainfo->qi_quotaofflock); + mp->m_qflags &= ~(flags & XFS_ALL_QUOTA_ENFD); + spin_lock(&mp->m_sb_lock); + mp->m_sb.sb_qflags = mp->m_qflags; + spin_unlock(&mp->m_sb_lock); + mutex_unlock(&mp->m_quotainfo->qi_quotaofflock); -out_unlock: - if (error && qoffstart) - xfs_qm_qoff_logitem_relse(qoffstart); - mutex_unlock(&q->qi_quotaofflock); - return error; + /* XXX what to do if error ? Revert back to old vals incore ? */ + return xfs_sync_sb(mp, false); } STATIC intdiff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c index 48e09ea30ee539..b7e4b05a559bdb 100644 --- a/fs/xfs/xfs_trans_dquot.c +++ b/fs/xfs/xfs_trans_dquot.c@@ -843,44 +843,6 @@ xfs_trans_reserve_quota_icreate( dblocks, 1, XFS_QMOPT_RES_REGBLKS); } -/* - * This routine is called to allocate a quotaoff log item. - */ -struct xfs_qoff_logitem * -xfs_trans_get_qoff_item( - struct xfs_trans *tp, - struct xfs_qoff_logitem *startqoff, - uint flags) -{ - struct xfs_qoff_logitem *q; - - ASSERT(tp != NULL); - - q = xfs_qm_qoff_logitem_init(tp->t_mountp, startqoff, flags); - ASSERT(q != NULL); - - /* - * Get a log_item_desc to point at the new item. - */ - xfs_trans_add_item(tp, &q->qql_item); - return q; -} - - -/* - * This is called to mark the quotaoff logitem as needing - * to be logged when the transaction is committed. The logitem must - * already be associated with the given transaction. - */ -void -xfs_trans_log_quotaoff_item( - struct xfs_trans *tp, - struct xfs_qoff_logitem *qlp) -{ - tp->t_flags |= XFS_TRANS_DIRTY; - set_bit(XFS_LI_DIRTY, &qlp->qql_item.li_flags); -} - STATIC void xfs_trans_alloc_dqinfo( xfs_trans_t *tp)-- 2.30.2