Re: [PATCH 1/2] xfs: remove support for disabling quota accounting on a mounted file system

[Brian Foster <bfoster@redhat.com>]

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@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                |   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
>