Re: [PATCH 05/14] xfs: refactor quota expiration timer modification

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

On Wed, Feb 12, 2020 at 05:57:24PM -0600, Eric Sandeen wrote:
> On 12/31/19 7:11 PM, Darrick J. Wong 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>
> 
> ...
> 
> > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > index ae7bb6361a99..44bae5f16b55 100644
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> > @@ -113,6 +113,36 @@ xfs_quota_exceeded(
> >  	return *hardlimit && count > be64_to_cpup(hardlimit);
> >  }
> >  
> > +/*
> > + * Clamp a quota grace period expiration timer to the range that we support.
> > + */
> > +static inline time64_t
> > +xfs_dquot_clamp_timer(
> > +	time64_t			timer)
> > +{
> > +	return clamp_t(time64_t, timer, XFS_DQ_TIMEOUT_MIN, XFS_DQ_TIMEOUT_MAX);
> > +}
> > +
> > +/* Set a quota grace period expiration timer. */
> > +static inline void
> > +xfs_quota_set_timer(
> > +	__be32			*dtimer,
> > +	time_t			limit)
> > +{
> > +	time64_t		new_timeout;
> > +
> > +	new_timeout = xfs_dquot_clamp_timer(get_seconds() + limit);
> > +	*dtimer = cpu_to_be32(new_timeout);
> > +}
> > +
> > +/* Clear a quota grace period expiration timer. */
> > +static inline void
> > +xfs_quota_clear_timer(
> > +	__be32			*dtimer)
> > +{
> > +	*dtimer = cpu_to_be32(0);
> 
> do we need to endian convert 0 to make sparse happy?  I don't see us doing
> that anywhere else.  TBH not really sure I see the reason for the function
> at all unless you really, really like the symmetry.
> 
> > +}
> > +
> >  /*
> >   * Check the limits and timers of a dquot and start or reset timers
> >   * if necessary.
> > @@ -152,14 +182,14 @@ xfs_qm_adjust_dqtimers(
> >  			&d->d_blk_softlimit, &d->d_blk_hardlimit);
> >  	if (!d->d_btimer) {
> >  		if (over) {
> > -			d->d_btimer = cpu_to_be32(get_seconds() +
> > +			xfs_quota_set_timer(&d->d_btimer,
> >  					mp->m_quotainfo->qi_btimelimit);
> >  		} else {
> >  			d->d_bwarns = 0;
> >  		}
> >  	} else {
> >  		if (!over) {
> > -			d->d_btimer = 0;
> > +			xfs_quota_clear_timer(&d->d_btimer);
> 
> yeah that's a very fancy way to say "= 0" ;)
> 
> ...

Yes, that's a fancy way to assign zero.  However, consider that for
bigtime support, I had to add an incore quota timer so that timers more
or less fire when they're supposed to, and now there's a function to
convert the incore timespec64 value to whatever is ondisk:

/* Clear a quota grace period expiration timer. */
static inline void
xfs_quota_clear_timer(
	struct xfs_disk_dquot	*ddq,
	time64_t		*itimer,
	__be32			*dtimer)
{
	struct timespec64	tv = { 0 };

	*itimer = tv.tv_sec;
	xfs_dquot_to_disk_timestamp(ddq, dtimer, &tv);
}

It was at *that* point in the patchset that it seemed easier to call a
small function three times than to open-code this three times.

> > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> > index f67f3645efcd..52dc5326b7bf 100644
> > --- a/fs/xfs/xfs_ondisk.h
> > +++ b/fs/xfs/xfs_ondisk.h
> > @@ -25,6 +25,8 @@ xfs_check_ondisk_structs(void)
> >  	/* make sure timestamp limits are correct */
> >  	XFS_CHECK_VALUE(XFS_INO_TIME_MIN, 			-2147483648LL);
> >  	XFS_CHECK_VALUE(XFS_INO_TIME_MAX,			2147483647LL);
> > +	XFS_CHECK_VALUE(XFS_DQ_TIMEOUT_MIN,			1LL);
> > +	XFS_CHECK_VALUE(XFS_DQ_TIMEOUT_MAX,			4294967295LL);
> 
> again grumble grumble really not checking an ondisk structure.

Same answer as before. :)

--D

> >  	/* ag/file structures */
> >  	XFS_CHECK_STRUCT_SIZE(struct xfs_acl,			4);
> >