Re: [PATCH v2 21/24] xfs: quota: move to time64_t interfaces

[Arnd Bergmann]

On Fri, Dec 13, 2019 at 10:17 PM Darrick J. Wong
<darrick.wong@xxxxxxxxxx> wrote:
>
> On Fri, Dec 13, 2019 at 09:53:49PM +0100, Arnd Bergmann wrote:
> > As a preparation for removing the 32-bit time_t type and
> > all associated interfaces, change xfs to use time64_t and
> > ktime_get_real_seconds() for the quota housekeeping.
> >
> > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
>
> Looks mostly reasonable to me...
>
> The bigtime series refactors the triplicated timer handling and whatnot,
> but I don't think it would be difficult to rebase that series assuming
> this lands first (which it probably will, I expect a new incompat ondisk
> feature to take a /long/ time to get through review.)

Could you just merge my three patches into your tree then once
you are happy with all the changes?

> > @@ -183,7 +183,7 @@ xfs_qm_adjust_dqtimers(
> >                   (d->d_rtb_hardlimit &&
> >                    (be64_to_cpu(d->d_rtbcount) >
> >                     be64_to_cpu(d->d_rtb_hardlimit)))) {
> > -                     d->d_rtbtimer = cpu_to_be32(get_seconds() +
> > +                     d->d_rtbtimer = cpu_to_be32(ktime_get_real_seconds() +
> >                                       mp->m_quotainfo->qi_rtbtimelimit);
>
> Hmm, so one thing that I clean up on the way to bigtime is the total
> lack of clamping here.  If (for example) it's September 2105 and
> rtbtimelimit is set to 1 year, this will cause an integer overflow.  The
> quota timer will be set to 1970 and expire immediately, rather than what
> I'd consider the best effort of February 2106.

I don't think clamping would be good here, that just replaces
one bug with another at the overflow time. If you would like to
have something better before this gets extended, I could try to
come up with a version that converts it to the nearest 64-bit
timestamp, similar to the way that time_before32() in the kernel
or the NTP protocol work.

If you think it can get extended properly soon, I'd just leave the
patch as it is today in order to remove the get_seconds()
interface for v5.6.

> (I'll grant you the current code also behaves like this...)
>
> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>

Thanks,

       Arnd