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

[Eric Sandeen <sandeen@sandeen.net>]

On 2/12/20 9:27 PM, Eric Sandeen wrote:
> On 2/12/20 7:46 PM, Darrick J. Wong wrote:
>> 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>
> 
> ...
> 
>>>>  	} 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.
> 
> +void
> +xfs_dquot_to_disk_timestamp(
> +	__be32			*dtimer,
> +	const struct timespec64	*tv)
> +{
> +	*dtimer = cpu_to_be32(tv->tv_sec);
> +}
> 
>  static inline void
>  xfs_quota_clear_timer(
> +	time64_t		*itimer,
>  	__be32			*dtimer)
>  {
> -	*dtimer = cpu_to_be32(0);
> +	struct timespec64	tv = { 0 };
> +
> +	*itimer = tv.tv_sec;
> +	xfs_dquot_to_disk_timestamp(dtimer, &tv);
>  } 
> 
> xfs_quota_clear_timer(&dqp->q_btimer, &d->d_btimer);
> 
> That's still a very fancy way of saying:
> 
>         dqp->q_btimer = d->d_btimer = 0;
> 
> I think?  Can't really see what value the timespec64 adds here.
> 
> -Eric
> 

Actually,

 xfs_quota_set_timer(
+	time64_t		*itimer,
 	__be32			*dtimer,
 	time_t			limit)
 {
-	time64_t		new_timeout;
+	struct timespec64	tv = { 0 };
 
-	new_timeout = xfs_dquot_clamp_timer(get_seconds() + limit);
-	*dtimer = cpu_to_be32(new_timeout);
+	tv.tv_sec = xfs_dquot_clamp_timer(ktime_get_real_seconds() + limit);
+	*itimer = tv.tv_sec;
+	xfs_dquot_to_disk_timestamp(dtimer, &tv);
 }

I'm not sure why there's a timespec64 here either.  Isn't everything
we're dealing with on timers in seconds, using only tv_sec, and time64_t would
suffice instead of using a timespec64 just to carry around a seconds value?

-Eric