diff mbox series

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

Message ID 159797590052.965217.10856208107922013686.stgit@magnolia
State Superseded
Headers show
Series xfs: widen timestamps to deal with y2038 | expand

Commit Message

Darrick J. Wong Aug. 21, 2020, 2: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.  Note that we'll deal with the
grace period timer separately.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/libxfs/xfs_format.h |   22 ++++++++++++++++++++++
 fs/xfs/xfs_dquot.c         |   13 ++++++++++++-
 fs/xfs/xfs_dquot.h         |    2 ++
 fs/xfs/xfs_ondisk.h        |    2 ++
 fs/xfs/xfs_qm_syscalls.c   |    9 +++++++--
 5 files changed, 45 insertions(+), 3 deletions(-)

Comments

Christoph Hellwig Aug. 22, 2020, 7:14 a.m. UTC | #1
On Thu, Aug 20, 2020 at 07:11:40PM -0700, 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.  Note that we'll deal with the
> grace period timer separately.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_format.h |   22 ++++++++++++++++++++++
>  fs/xfs/xfs_dquot.c         |   13 ++++++++++++-
>  fs/xfs/xfs_dquot.h         |    2 ++
>  fs/xfs/xfs_ondisk.h        |    2 ++
>  fs/xfs/xfs_qm_syscalls.c   |    9 +++++++--
>  5 files changed, 45 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index b1b8a5c05cea..ef36978239ac 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -1197,6 +1197,28 @@ static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev)
>  
>  #define XFS_DQTYPE_ANY		(XFS_DQTYPE_REC_MASK)
>  
> +/*
> + * 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.  We pad this with some more expansion room to construct the on
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index bcd73b9c2994..2425b1c30d11 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -98,6 +98,16 @@ xfs_qm_adjust_dqlimits(
>  		xfs_dquot_set_prealloc_limits(dq);
>  }
>  
> +/* Set the expiration time of a quota's grace period. */
> +void
> +xfs_dquot_set_timeout(
> +	time64_t		*timer,
> +	time64_t		value)
> +{
> +	*timer = clamp_t(time64_t, value, XFS_DQ_TIMEOUT_MIN,
> +					  XFS_DQ_TIMEOUT_MAX);
> +}

Why doesn't this just return the value?  That would seem like
a much more natural calling convention to me.
Dave Chinner Aug. 23, 2020, 11:57 p.m. UTC | #2
On Thu, Aug 20, 2020 at 07:11:40PM -0700, 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.  Note that we'll deal with the
> grace period timer separately.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_format.h |   22 ++++++++++++++++++++++
>  fs/xfs/xfs_dquot.c         |   13 ++++++++++++-
>  fs/xfs/xfs_dquot.h         |    2 ++
>  fs/xfs/xfs_ondisk.h        |    2 ++
>  fs/xfs/xfs_qm_syscalls.c   |    9 +++++++--
>  5 files changed, 45 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index b1b8a5c05cea..ef36978239ac 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -1197,6 +1197,28 @@ static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev)
>  
>  #define XFS_DQTYPE_ANY		(XFS_DQTYPE_REC_MASK)
>  
> +/*
> + * 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)

Same as last patch - these reference the unix epoch, so perhaps
they should be named that way....

>   * This is the main portion of the on-disk representation of quota information
>   * for a user.  We pad this with some more expansion room to construct the on
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index bcd73b9c2994..2425b1c30d11 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -98,6 +98,16 @@ xfs_qm_adjust_dqlimits(
>  		xfs_dquot_set_prealloc_limits(dq);
>  }
>  
> +/* Set the expiration time of a quota's grace period. */
> +void
> +xfs_dquot_set_timeout(
> +	time64_t		*timer,
> +	time64_t		value)
> +{
> +	*timer = clamp_t(time64_t, value, XFS_DQ_TIMEOUT_MIN,
> +					  XFS_DQ_TIMEOUT_MAX);
> +}
> +

Not sure I see any benefit in passing *timer as a parameter over
just returning a time64_t value...

Cheers,

Dave.
Darrick J. Wong Aug. 24, 2020, 2:34 a.m. UTC | #3
On Mon, Aug 24, 2020 at 09:57:37AM +1000, Dave Chinner wrote:
> On Thu, Aug 20, 2020 at 07:11:40PM -0700, 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.  Note that we'll deal with the
> > grace period timer separately.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_format.h |   22 ++++++++++++++++++++++
> >  fs/xfs/xfs_dquot.c         |   13 ++++++++++++-
> >  fs/xfs/xfs_dquot.h         |    2 ++
> >  fs/xfs/xfs_ondisk.h        |    2 ++
> >  fs/xfs/xfs_qm_syscalls.c   |    9 +++++++--
> >  5 files changed, 45 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> > index b1b8a5c05cea..ef36978239ac 100644
> > --- a/fs/xfs/libxfs/xfs_format.h
> > +++ b/fs/xfs/libxfs/xfs_format.h
> > @@ -1197,6 +1197,28 @@ static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev)
> >  
> >  #define XFS_DQTYPE_ANY		(XFS_DQTYPE_REC_MASK)
> >  
> > +/*
> > + * 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)
> 
> Same as last patch - these reference the unix epoch, so perhaps
> they should be named that way....
> 
> >   * This is the main portion of the on-disk representation of quota information
> >   * for a user.  We pad this with some more expansion room to construct the on
> > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > index bcd73b9c2994..2425b1c30d11 100644
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> > @@ -98,6 +98,16 @@ xfs_qm_adjust_dqlimits(
> >  		xfs_dquot_set_prealloc_limits(dq);
> >  }
> >  
> > +/* Set the expiration time of a quota's grace period. */
> > +void
> > +xfs_dquot_set_timeout(
> > +	time64_t		*timer,
> > +	time64_t		value)
> > +{
> > +	*timer = clamp_t(time64_t, value, XFS_DQ_TIMEOUT_MIN,
> > +					  XFS_DQ_TIMEOUT_MAX);
> > +}
> > +
> 
> Not sure I see any benefit in passing *timer as a parameter over
> just returning a time64_t value...

I'll look into changing them, both for this and the next patch.
I /think/ it should be fairly straightforward.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index b1b8a5c05cea..ef36978239ac 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -1197,6 +1197,28 @@  static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev)
 
 #define XFS_DQTYPE_ANY		(XFS_DQTYPE_REC_MASK)
 
+/*
+ * 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.  We pad this with some more expansion room to construct the on
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index bcd73b9c2994..2425b1c30d11 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -98,6 +98,16 @@  xfs_qm_adjust_dqlimits(
 		xfs_dquot_set_prealloc_limits(dq);
 }
 
+/* Set the expiration time of a quota's grace period. */
+void
+xfs_dquot_set_timeout(
+	time64_t		*timer,
+	time64_t		value)
+{
+	*timer = clamp_t(time64_t, value, XFS_DQ_TIMEOUT_MIN,
+					  XFS_DQ_TIMEOUT_MAX);
+}
+
 /*
  * Determine if this quota counter is over either limit and set the quota
  * timers as appropriate.
@@ -112,7 +122,8 @@  xfs_qm_adjust_res_timer(
 	if ((res->softlimit && res->count > res->softlimit) ||
 	    (res->hardlimit && res->count > res->hardlimit)) {
 		if (res->timer == 0)
-			res->timer = ktime_get_real_seconds() + qlim->time;
+			xfs_dquot_set_timeout(&res->timer,
+					ktime_get_real_seconds() + qlim->time);
 	} else {
 		if (res->timer == 0)
 			res->warnings = 0;
diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h
index 282a65da93c7..11bd0ee9b0fa 100644
--- a/fs/xfs/xfs_dquot.h
+++ b/fs/xfs/xfs_dquot.h
@@ -237,4 +237,6 @@  typedef int (*xfs_qm_dqiterate_fn)(struct xfs_dquot *dq,
 int xfs_qm_dqiterate(struct xfs_mount *mp, xfs_dqtype_t type,
 		xfs_qm_dqiterate_fn iter_fn, void *priv);
 
+void xfs_dquot_set_timeout(time64_t *timer, time64_t limit);
+
 #endif /* __XFS_DQUOT_H__ */
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index 48a64fa49f91..38ccffcf3336 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -25,6 +25,8 @@  xfs_check_limits(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);
 }
 
 static inline void __init
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);
+	}
 }
 
 /*