Re: [PATCH 02/11] xfs: refactor quota expiration timer modification

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: linux-xfs <linux-xfs@vger.kernel.org>,
	Eric Sandeen <sandeen@sandeen.net>
Subject: Re: [PATCH 02/11] xfs: refactor quota expiration timer modification
Date: Tue, 18 Aug 2020 08:40:02 -0700	[thread overview]
Message-ID: <20200818154002.GS6096@magnolia> (raw)
In-Reply-To: <CAOQ4uxiekcHPfyjvJJMBhW-7uSDZ3Gb=Zyu5ZpyKhxz1QmMcnw@mail.gmail.com>

On Tue, Aug 18, 2020 at 09:48:14AM +0300, Amir Goldstein wrote:
> On Tue, Aug 18, 2020 at 1:59 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Define explicit limits on the range of quota grace period expiration
> > timeouts and refactor the code that modifies the timeouts into helpers
> > that clamp the values appropriately.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> Question below...
> 
> ...
> 
> > diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> > index 1c542b4a5220..b16d533a6feb 100644
> > --- a/fs/xfs/xfs_qm_syscalls.c
> > +++ b/fs/xfs/xfs_qm_syscalls.c
> > @@ -483,9 +483,14 @@ xfs_setqlim_timer(
> >         struct xfs_quota_limits *qlim,
> >         s64                     timer)
> >  {
> > -       res->timer = timer;
> > -       if (qlim)
> > +       if (qlim) {
> > +               /* Set the length of the default grace period. */
> > +               res->timer = timer;
> >                 qlim->time = timer;
> > +       } else {
> > +               /* Set the grace period expiration on a quota. */
> > +               xfs_dquot_set_timeout(&res->timer, timer);
> > +       }
> >  }
> 
> I understand why you did this. This is mid series craft, but it looks very odd
> to your average reviewer.
> 
> Maybe leave a TODO comment above res->timer = timer which will be
> removed later in the series?
> 
> Not critical.

<shrug> The grace period case gets changed in the very next patch, but I
suppose it wouldn't hurt to mention that in the commit log.

--D

> 
> Thanks,
> Amir.