Re: [PATCH 02/14] xfs: preserve default grace interval during quotacheck

[Eric Sandeen <sandeen@sandeen.net>]

On 12/31/19 7:11 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When quotacheck runs, it zeroes all the timer fields in every dquot.
> Unfortunately, it also does this to the root dquot, which erases any
> preconfigured grace interval that the administrator may have set.  Worse
> yet, the incore copies of those variables remain set.  This cache
> coherence problem manifests itself as the grace interval mysteriously
> being reset back to the defaults at the /next/ mount.

woot that's kind of a theme in xfs quota code :/

Is it my turn to ask for a testcase?

so: "quotacheck" on xfs means "mount with quota accounting enabled" I think,
just for clarity...

> Fix it by resetting the root disk dquot's timer fields to the incore
> values.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_qm.c |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 0ce334c51d73..d4a9765c9502 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -842,6 +842,23 @@ xfs_qm_qino_alloc(
>  	return error;
>  }
>  
> +/* Save the grace period intervals when zeroing dquots for quotacheck. */
> +static inline void
> +xfs_qm_reset_dqintervals(
> +	struct xfs_mount	*mp,
> +	struct xfs_disk_dquot	*ddq)
> +{
> +	struct xfs_quotainfo	*qinf = mp->m_quotainfo;
> +
> +	if (qinf->qi_btimelimit != XFS_QM_BTIMELIMIT)
> +		ddq->d_btimer = cpu_to_be32(qinf->qi_btimelimit);
> +
> +	if (qinf->qi_itimelimit != XFS_QM_ITIMELIMIT)
> +		ddq->d_itimer = cpu_to_be32(qinf->qi_itimelimit);
> +
> +	if (qinf->qi_rtbtimelimit != XFS_QM_RTBTIMELIMIT)
> +		ddq->d_rtbtimer = cpu_to_be32(qinf->qi_rtbtimelimit);

Probably need to handle warning counters here too, but ...

> +}
>  
>  STATIC void
>  xfs_qm_reset_dqcounts(
> @@ -895,6 +912,8 @@ 	(
>  		ddq->d_bwarns = 0;
>  		ddq->d_iwarns = 0;
>  		ddq->d_rtbwarns = 0;

a comment about why !ddq->d_id (i.e. it's the default quota)
would probably be good here.

> +		if (!ddq->d_id)
> +			xfs_qm_reset_dqintervals(mp, ddq);

Isn't it a little weird to clear it for ID 0, then immediately reset it?

Let's see, quotacheck only happens when we do a fresh mount where quota accounting
was not on during the previous mount.

The point of quotacheck is to get all of the block counters in sync with actual
block usage.

The timers (and warnings) for normal users are zero until they exceed soft limits,
then reflect the time at which EDQUOT will appear.

<aside: does quotacheck set timers for users who are already over soft limits
at quotacheck time...?  Yes: see xfs_qm_quotacheck_dqadjust()>

The timers (and warnings) for ID 0 (root/default) are where we store the default
grace times & warning limits, there is no need for quotacheck to change them;
they serve a different purpose.

So quotacheck really should never be touching the default timers or warn limits
on ID 0.  I'd suggest simply skipping them for id 0, as it is treated specially
in several other places as well, i.e.

-               ddq->d_btimer = 0;
-               ddq->d_itimer = 0;
-               ddq->d_rtbtimer = 0;
-               ddq->d_bwarns = 0;
-               ddq->d_iwarns = 0;
-               ddq->d_rtbwarns = 0;
+               /* Don't reset default quota timers & counters in root dquot */
+               if (ddq->d_id) {
+                       ddq->d_btimer = 0;
+                       ddq->d_itimer = 0;
+                       ddq->d_rtbtimer = 0;
+                       ddq->d_bwarns = 0;
+                       ddq->d_iwarns = 0;
+                       ddq->d_rtbwarns = 0;
+               }

>  
>  		if (xfs_sb_version_hascrc(&mp->m_sb)) {
>  			xfs_update_cksum((char *)&dqb[j],
>