diff mbox series

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

Message ID 157784109369.1364230.637677553755124721.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: widen timestamps to deal with y2038 | expand

Commit Message

Darrick J. Wong Jan. 1, 2020, 1:11 a.m. UTC 
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>
---
 fs/xfs/libxfs/xfs_format.h |   22 ++++++++++++++++++++++
 fs/xfs/xfs_dquot.c         |   42 ++++++++++++++++++++++++++++++++++++------
 fs/xfs/xfs_ondisk.h        |    2 ++
 3 files changed, 60 insertions(+), 6 deletions(-)

Comments

Eric Sandeen Feb. 12, 2020, 11:57 p.m. UTC | #1 
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" ;)

...

> 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.

>  	/* ag/file structures */
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_acl,			4);
>
Darrick J. Wong Feb. 13, 2020, 1:46 a.m. UTC | #2 
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);
> >
Eric Sandeen Feb. 13, 2020, 3:27 a.m. UTC | #3 
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
Eric Sandeen Feb. 13, 2020, 3:32 a.m. UTC | #4 
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
Darrick J. Wong Feb. 13, 2020, 5:33 a.m. UTC | #5 
On Wed, Feb 12, 2020 at 09:32:33PM -0600, Eric Sandeen wrote:
> 
> 
> 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?

Yes, the grace periods recorded in the timer fields in dquot 0 are
intervals measured in seconds.

However, for dquot != 0, the timer fields store the time of the
expiration, so I settled on timespec64 as the incore structure so that
XFS consistently uses struct timespec64 to represent specific points in
time.

(That and time64_t doesn't exist in userspace.)

--D

> -Eric
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 82b15832ba32..95761b38fe86 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -1180,6 +1180,28 @@  static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev)
 #define XFS_DQUOT_MAGIC		0x4451		/* 'DQ' */
 #define XFS_DQUOT_VERSION	(uint8_t)0x01	/* latest version number */
 
+/*
+ * XFS Quota Timers
+ * ================
+ *
+ * Quota grace period expiration timers are an unsigned 32-bit seconds counter;
+ * time zero is the Unix epoch, Jan  1 00:00:01 UTC 1970.  An expiration value
+ * of zero means that the quota limit has not been reached, and therefore no
+ * expiration has been set.
+ */
+
+/*
+ * Smallest possible quota expiration with traditional timestamps, which is
+ * Jan  1 00:00:01 UTC 1970.
+ */
+#define XFS_DQ_TIMEOUT_MIN	((int64_t)1)
+
+/*
+ * Largest possible quota expiration with traditional timestamps, which is
+ * Feb  7 06:28:15 UTC 2106.
+ */
+#define XFS_DQ_TIMEOUT_MAX	((int64_t)U32_MAX)
+
 /*
  * This is the main portion of the on-disk representation of quota
  * information for a user. This is the q_core of the struct xfs_dquot that
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);
+}
+
 /*
  * 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);
 		}
 	}
 
@@ -167,14 +197,14 @@  xfs_qm_adjust_dqtimers(
 			&d->d_ino_softlimit, &d->d_ino_hardlimit);
 	if (!d->d_itimer) {
 		if (over) {
-			d->d_itimer = cpu_to_be32(get_seconds() +
+			xfs_quota_set_timer(&d->d_itimer,
 					mp->m_quotainfo->qi_itimelimit);
 		} else {
 			d->d_iwarns = 0;
 		}
 	} else {
 		if (!over) {
-			d->d_itimer = 0;
+			xfs_quota_clear_timer(&d->d_itimer);
 		}
 	}
 
@@ -182,14 +212,14 @@  xfs_qm_adjust_dqtimers(
 			&d->d_rtb_softlimit, &d->d_rtb_hardlimit);
 	if (!d->d_rtbtimer) {
 		if (over) {
-			d->d_rtbtimer = cpu_to_be32(get_seconds() +
+			xfs_quota_set_timer(&d->d_rtbtimer,
 					mp->m_quotainfo->qi_rtbtimelimit);
 		} else {
 			d->d_rtbwarns = 0;
 		}
 	} else {
 		if (!over) {
-			d->d_rtbtimer = 0;
+			xfs_quota_clear_timer(&d->d_rtbtimer);
 		}
 	}
 }
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);
 
 	/* ag/file structures */
 	XFS_CHECK_STRUCT_SIZE(struct xfs_acl,			4);