Re: [PATCH 10/11] xfs: enable bigtime for quota timers

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Aug 18, 2020 at 04:58:35PM +0300, Amir Goldstein wrote:
> On Tue, Aug 18, 2020 at 1:57 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>
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> Minor suggestion below...
> 
> 
> > @@ -306,5 +327,24 @@ xfs_dquot_to_disk_timestamp(
> >         __be32                  *dtimer,
> >         time64_t                timer)
> >  {
> > +       /* Zero always means zero, regardless of encoding. */
> > +       if (!timer) {
> > +               *dtimer = cpu_to_be32(0);
> > +               return;
> > +       }
> > +
> > +       if (dqp->q_type & XFS_DQTYPE_BIGTIME) {
> > +               uint64_t        t = timer;
> > +
> > +               /*
> > +                * Round the end of the grace period up to the nearest bigtime
> > +                * interval that we support, to give users the most time to fix
> > +                * the problems.
> > +                */
> > +               t = roundup_64(t, 1U << XFS_DQ_BIGTIME_SHIFT);
> > +               *dtimer = cpu_to_be32(t >> XFS_DQ_BIGTIME_SHIFT);
> > +               return;
> > +       }
> > +
> >         *dtimer = cpu_to_be32(timer);
> >  }
> 
> This suggestion has to do with elegance which is subjective...
> 
> /*
>  * When bigtime is enabled, we trade a few bits of precision to expand the
>  * expiration timeout range to match that of big inode timestamps.  The grace
>  * periods stored in dquot 0 are not shifted, since they record an interval,
>  * not a timestamp.
>  */
> #define XFS_DQ_BIGTIME_SHIFT   (2)
> #define XFS_DQ_BIGTIME_SLACK ((1U << XFS_DQ_BIGTIME_SHIFT)-1)
> 
>                /*
>                 * Round the end of the grace period up to the nearest bigtime
>                 * interval that we support, to give users the most time to fix
>                 * the problems.
>                 */
>                uint64_t        t = timer + XFS_DQ_BIGTIME_SLACK;
>                *dtimer = cpu_to_be32(t >> XFS_DQ_BIGTIME_SHIFT);
> 
> Take it or leave it.

Hm.  Normally I don't really like open-coding a rounding operation, but
it does eliminate an integer division.

(I dunno about "SLACK", but I can't think of a better word for
imprecision...)

--D

> Thanks,
> Amir.