Re: [PATCH 13/14] xfs: enable bigtime for quota timers

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: linux-xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH 13/14] xfs: enable bigtime for quota timers
Date: Mon, 1 Jun 2020 17:09:17 -0700	[thread overview]
Message-ID: <20200602000917.GE2162697@magnolia> (raw)
In-Reply-To: <CAOQ4uxgvnuVtfxz41W+FuTxy2GZ5QZwwUhacHgfMzJMKJ_db1g@mail.gmail.com>

On Sun, May 31, 2020 at 08:07:59PM +0300, Amir Goldstein wrote:
> On Wed, Jan 1, 2020 at 3:12 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Enable the bigtime feature for quota timers.  We decrease the accuracy
> > of the timers to ~4s in exchange for being able to set timers up to the
> > bigtime maximum.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_dquot_buf.c  |   72 ++++++++++++++++++++++++++++++++++++++--
> >  fs/xfs/libxfs/xfs_format.h     |   22 ++++++++++++
> >  fs/xfs/libxfs/xfs_quota_defs.h |   11 ++++--
> >  fs/xfs/scrub/quota.c           |    5 +++
> >  fs/xfs/xfs_dquot.c             |   71 +++++++++++++++++++++++++++++++--------
> >  fs/xfs/xfs_ondisk.h            |    6 +++
> >  fs/xfs/xfs_qm.c                |   13 ++++---
> >  fs/xfs/xfs_qm.h                |    8 ++--
> >  fs/xfs/xfs_qm_syscalls.c       |   19 ++++++-----
> >  9 files changed, 186 insertions(+), 41 deletions(-)
> >
> >
> > diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> > index 72e0fcfef580..2b5d51a6d64b 100644
> > --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> > +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> > @@ -40,6 +40,8 @@ xfs_dquot_verify(
> >         xfs_dqid_t              id,
> >         uint                    type)   /* used only during quotacheck */
> >  {
> > +       uint8_t                 dtype;
> > +
> >         /*
> >          * We can encounter an uninitialized dquot buffer for 2 reasons:
> >          * 1. If we crash while deleting the quotainode(s), and those blks got
> > @@ -60,11 +62,22 @@ xfs_dquot_verify(
> >         if (ddq->d_version != XFS_DQUOT_VERSION)
> >                 return __this_address;
> >
> > -       if (type && ddq->d_flags != type)
> > +       dtype = ddq->d_flags & XFS_DQ_ALLTYPES;
> 
> Suggest dtype = XFS_DQ_TYPE(ddq->d_flags);
> 
> [...]
> 
> > @@ -540,13 +551,28 @@ xfs_dquot_from_disk(
> >         dqp->q_res_icount = be64_to_cpu(ddqp->d_icount);
> >         dqp->q_res_rtbcount = be64_to_cpu(ddqp->d_rtbcount);
> >
> > -       xfs_dquot_from_disk_timestamp(&tv, ddqp->d_btimer);
> > +       xfs_dquot_from_disk_timestamp(ddqp, &tv, ddqp->d_btimer);
> >         dqp->q_btimer = tv.tv_sec;
> > -       xfs_dquot_from_disk_timestamp(&tv, ddqp->d_itimer);
> > +       xfs_dquot_from_disk_timestamp(ddqp, &tv, ddqp->d_itimer);
> >         dqp->q_itimer = tv.tv_sec;
> > -       xfs_dquot_from_disk_timestamp(&tv, ddqp->d_rtbtimer);
> > +       xfs_dquot_from_disk_timestamp(ddqp, &tv, ddqp->d_rtbtimer);
> >         dqp->q_rtbtimer = tv.tv_sec;
> >
> > +       /* Upgrade to bigtime if possible. */
> > +       if (xfs_dquot_add_bigtime(dqp->q_mount, iddq)) {
> > +               tv.tv_sec = xfs_dquot_clamp_timer(iddq, dqp->q_btimer);
> > +               xfs_dquot_to_disk_timestamp(iddq, &iddq->d_btimer, &tv);
> > +               dqp->q_btimer = tv.tv_sec;
> > +
> > +               tv.tv_sec = xfs_dquot_clamp_timer(iddq, dqp->q_itimer);
> > +               xfs_dquot_to_disk_timestamp(iddq, &iddq->d_itimer, &tv);
> > +               dqp->q_itimer = tv.tv_sec;
> > +
> > +               tv.tv_sec = xfs_dquot_clamp_timer(iddq, dqp->q_rtbtimer);
> > +               xfs_dquot_to_disk_timestamp(iddq, &iddq->d_rtbtimer, &tv);
> > +               dqp->q_rtbtimer = tv.tv_sec;
> > +       }
> > +
> 
> This is better than the inode timestamp conversion because at
> least the bigtime flag incore is always consistent with the incore values.
> But I think it would be safer if the conversion happened inside the helper.

This code, like the inode timestamp handling code, reads the ondisk
timer value into the incore dquot, and sets the bigtime flag on the
incore dquot.  We then wait until someone dirties the dquot to transform
the ondisk dquot to be in bigtime format with the flag set.

The dquot conversion is /much/ messier than the inode, because the dquot
infrastructure maintains an incore copy of the ondisk dquot as well as
an incore time64_t value, and we have to keep that whole mess
consistent.

TBH someone should probably go fix the incore dquots to have only the
incore values (instead of the embedded xfs_disk_dquot) but I'd rather
that go in as a cleanup after adding this series, because this series is
already 13 patches, and if you combine cleaning up dquots with hch's
incore inode removal now we have Yet Another Friggin 40-patch series.

--D

> Thanks,
> Amir.