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

[Amir Goldstein <amir73il@gmail.com>]

On Tue, Jun 2, 2020 at 3:09 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> 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.
>

I see. That's what got me confused about how the inode timestamp incore
values are stored.

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

Fare enough, but until someone does go and fix this code, my comment
still stands.

This patch calls xfs_dquot_add_bigtime() in three call sites.

Two of them are correct because the values of timers is 0.
The one above is correct because it shifts the embedded timer
values right after the call.

IMO it would be a better practice to keep this conversion
containers in a helper xfs_dquot_upgrade_bigtime() and
explicitly check for the 0 value case.

Thanks,
Amir.