Message ID | 157784107520.1364230.49128863919644273.stgit@magnolia (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfs: widen timestamps to deal with y2038 | expand |
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], >
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
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
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
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 --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],