diff mbox series

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

Message ID 157784107520.1364230.49128863919644273.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: widen timestamps to deal with y2038 | expand

Commit Message

Darrick J. Wong Jan. 1, 2020, 1:11 a.m. UTC 
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.

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(+)

Comments

Eric Sandeen Feb. 12, 2020, 11:35 p.m. UTC | #1 
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],
>
Eric Sandeen Feb. 19, 2020, 4:55 a.m. UTC | #2 
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.
> 
> Fix it by resetting the root disk dquot's timer fields to the incore
> values.

Uh, so, even with this, it seems that we don't properly set up default time
limits on the first mount.  Looking into it...

I think we need something like this but I need to look more closely.

Otherwise, xfs_qm_dqget_uncached fails if run before quotacheck has initialized
things, and so we fail to set up default quotas or timers.

(also, not sure if there's any point to initializing timers if that quota
is not enabled...)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index f94f6c34ee35..f4ac69fd946e 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -663,17 +671,6 @@ xfs_qm_init_quotainfo(
 
 	mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
 
-	xfs_qm_init_timelimits(mp, XFS_DQ_USER);
-	xfs_qm_init_timelimits(mp, XFS_DQ_GROUP);
-	xfs_qm_init_timelimits(mp, XFS_DQ_PROJ);
-
-	if (XFS_IS_UQUOTA_RUNNING(mp))
-		xfs_qm_set_defquota(mp, XFS_DQ_USER);
-	if (XFS_IS_GQUOTA_RUNNING(mp))
-		xfs_qm_set_defquota(mp, XFS_DQ_GROUP);
-	if (XFS_IS_PQUOTA_RUNNING(mp))
-		xfs_qm_set_defquota(mp, XFS_DQ_PROJ);
-
 	qinf->qi_shrinker.count_objects = xfs_qm_shrink_count;
 	qinf->qi_shrinker.scan_objects = xfs_qm_shrink_scan;
 	qinf->qi_shrinker.seeks = DEFAULT_SEEKS;
@@ -1423,6 +1420,18 @@ xfs_qm_mount_quotas(
 			return;
 		}
 	}
+
+	xfs_qm_init_timelimits(mp, XFS_DQ_USER);
+	xfs_qm_init_timelimits(mp, XFS_DQ_GROUP);
+	xfs_qm_init_timelimits(mp, XFS_DQ_PROJ);
+
+	if (XFS_IS_UQUOTA_RUNNING(mp))
+		xfs_qm_set_defquota(mp, XFS_DQ_USER);
+	if (XFS_IS_GQUOTA_RUNNING(mp))
+		xfs_qm_set_defquota(mp, XFS_DQ_GROUP);
+	if (XFS_IS_PQUOTA_RUNNING(mp))
+		xfs_qm_set_defquota(mp, XFS_DQ_PROJ);
+
 	/*
 	 * If one type of quotas is off, then it will lose its
 	 * quotachecked status, since we won't be doing accounting for
Eric Sandeen March 3, 2020, 3:03 a.m. UTC | #3 
On 2/18/20 8:55 PM, Eric Sandeen wrote:
> 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.
>>
>> Fix it by resetting the root disk dquot's timer fields to the incore
>> values.
> 
> Uh, so, even with this, it seems that we don't properly set up default time
> limits on the first mount.  Looking into it...

Sorry.  This was actually a regression from my timer-per-type series.  :(

so ignore this critique, it's my fault. ;)

-Eric
Darrick J. Wong March 3, 2020, 3:48 p.m. UTC | #4 
On Mon, Mar 02, 2020 at 09:03:37PM -0600, Eric Sandeen wrote:
> On 2/18/20 8:55 PM, Eric Sandeen wrote:
> > 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.
> >>
> >> Fix it by resetting the root disk dquot's timer fields to the incore
> >> values.
> > 
> > Uh, so, even with this, it seems that we don't properly set up default time
> > limits on the first mount.  Looking into it...
> 
> Sorry.  This was actually a regression from my timer-per-type series.  :(
> 
> so ignore this critique, it's my fault. ;)

But will you (or anyone really) please review this fix?

--D

> -Eric
Eric Sandeen March 3, 2020, 3:52 p.m. UTC | #5 
On 3/3/20 7:48 AM, Darrick J. Wong wrote:
> On Mon, Mar 02, 2020 at 09:03:37PM -0600, Eric Sandeen wrote:
>> On 2/18/20 8:55 PM, Eric Sandeen wrote:
>>> 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.
>>>>
>>>> Fix it by resetting the root disk dquot's timer fields to the incore
>>>> values.
>>>
>>> Uh, so, even with this, it seems that we don't properly set up default time
>>> limits on the first mount.  Looking into it...
>>
>> Sorry.  This was actually a regression from my timer-per-type series.  :(
>>
>> so ignore this critique, it's my fault. ;)
> 
> But will you (or anyone really) please review this fix?

I did; I have other comments on it up-thread.

-Eric
diff mbox series

Patch

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);
+}
 
 STATIC void
 xfs_qm_reset_dqcounts(
@@ -895,6 +912,8 @@  xfs_qm_reset_dqcounts(
 		ddq->d_bwarns = 0;
 		ddq->d_iwarns = 0;
 		ddq->d_rtbwarns = 0;
+		if (!ddq->d_id)
+			xfs_qm_reset_dqintervals(mp, ddq);
 
 		if (xfs_sb_version_hascrc(&mp->m_sb)) {
 			xfs_update_cksum((char *)&dqb[j],