On Tue, Apr 20, 2021 at 09:22:55AM +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@xxxxxx>
> ---
> 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 | 4 -
> fs/xfs/xfs_qm_syscalls.c | 298 ---------------------------------
> fs/xfs/xfs_quotaops.c | 27 ++-
> fs/xfs/xfs_trans_dquot.c | 38 -----
> 9 files changed, 26 insertions(+), 526 deletions(-)
>
...
> diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
> index 88d70c236a5445..775bbee907a4b3 100644
> --- a/fs/xfs/xfs_quotaops.c
> +++ b/fs/xfs/xfs_quotaops.c
...
> @@ -184,7 +185,29 @@ xfs_quota_disable(
> if (!XFS_IS_QUOTA_ON(mp))
> return -EINVAL;
>
> - return xfs_qm_scall_quotaoff(mp, xfs_quota_flags(uflags));
What happened to the xfs_quota_flags() call here? Is it unnecessary?
> + /*
> + * No file system can have quotas enabled on disk but not in core.
> + * Note that quota utilities (like quotaoff) expect -EEXIST here.
> + */
> + if ((mp->m_qflags & flags) == 0)
> + return -EEXIST;
> +
> + /*
> + * We do not support actually turning off quota accounting any more.
> + * Just log a warning and ignored the accounting related flags.
> + */
> + if (flags & XFS_ALL_QUOTA_ACCT)
> + xfs_info(mp, "disabling of quota accounting not supported.");
> +
> + 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);
> +
One thing I notice from the old implementation is that it looks like we
effectively apply XFS_[UGP]QUOTA_ENFD to flags whenever the
corresponding XFS_[UGP]QUOTA_ACCT flag is passed. I don't know if that
is actually how flags are passed by userspace, but it looks like we'd
automatically disable enforcement if only a particular accounting flag
was passed. If that is the case, I wonder if we should preserve that
behavior one way or another..?
Otherwise the rest all looks pretty reasonable to me. Thanks for putting
this together.
Brian
> + /* XXX what to do if error ? Revert back to old vals incore ? */
> + return xfs_sync_sb(mp, false);
> }
>
> STATIC int
> diff --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.1
>
