diff mbox series

[01/11] xfs: explicitly define inode timestamp range

Message ID 159797589388.965217.3068074933916806311.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>

Formally define the inode timestamp ranges that existing filesystems
support, and switch the vfs timetamp ranges to use it.

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

Comments

Christoph Hellwig Aug. 22, 2020, 7:12 a.m. UTC | #1
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index acb9b737fe6b..48a64fa49f91 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -15,6 +15,18 @@
>  		"XFS: offsetof(" #structname ", " #member ") is wrong, " \
>  		"expected " #off)
>  
> +#define XFS_CHECK_VALUE(value, expected) \
> +	BUILD_BUG_ON_MSG((value) != (expected), \
> +		"XFS: value of " #value " is wrong, expected " #expected)
> +
> +static inline void __init
> +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);

I have to admit I don't get why you'd define a constant and then
check that it has the value you defined it to.  Seems a little cargo
cult.
Dave Chinner Aug. 23, 2020, 11:54 p.m. UTC | #2
On Thu, Aug 20, 2020 at 07:11:34PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Formally define the inode timestamp ranges that existing filesystems
> support, and switch the vfs timetamp ranges to use it.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_format.h |   19 +++++++++++++++++++
>  fs/xfs/xfs_ondisk.h        |   12 ++++++++++++
>  fs/xfs/xfs_super.c         |    5 +++--
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index be86fa1a5556..b1b8a5c05cea 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -849,11 +849,30 @@ struct xfs_agfl {
>  	    ASSERT(xfs_daddr_to_agno(mp, d) == \
>  		   xfs_daddr_to_agno(mp, (d) + (len) - 1)))
>  
> +/*
> + * XFS Timestamps
> + * ==============
> + *
> + * Inode timestamps consist of signed 32-bit counters for seconds and
> + * nanoseconds; time zero is the Unix epoch, Jan  1 00:00:00 UTC 1970.
> + */
>  typedef struct xfs_timestamp {
>  	__be32		t_sec;		/* timestamp seconds */
>  	__be32		t_nsec;		/* timestamp nanoseconds */
>  } xfs_timestamp_t;
>  
> +/*
> + * Smallest possible timestamp with traditional timestamps, which is
> + * Dec 13 20:45:52 UTC 1901.
> + */
> +#define XFS_INO_TIME_MIN	((int64_t)S32_MIN)
> +
> +/*
> + * Largest possible timestamp with traditional timestamps, which is
> + * Jan 19 03:14:07 UTC 2038.
> + */
> +#define XFS_INO_TIME_MAX	((int64_t)S32_MAX)

These are based on the Unix epoch. Can we call them something like
XFS_INO_UNIX_TIME_{MIN,MAX} to indicate what epoch they reference?

>  /*
>   * On-disk inode structure.
>   *
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index acb9b737fe6b..48a64fa49f91 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -15,6 +15,18 @@
>  		"XFS: offsetof(" #structname ", " #member ") is wrong, " \
>  		"expected " #off)
>  
> +#define XFS_CHECK_VALUE(value, expected) \
> +	BUILD_BUG_ON_MSG((value) != (expected), \
> +		"XFS: value of " #value " is wrong, expected " #expected)
> +
> +static inline void __init
> +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);
> +}

Not sure this really gains us anything? All it does is check that
S32_MIN/S32_MAX haven't changed value....

Cheers,

Dave.
Darrick J. Wong Aug. 24, 2020, 2:34 a.m. UTC | #3
On Mon, Aug 24, 2020 at 09:54:29AM +1000, Dave Chinner wrote:
> On Thu, Aug 20, 2020 at 07:11:34PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Formally define the inode timestamp ranges that existing filesystems
> > support, and switch the vfs timetamp ranges to use it.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_format.h |   19 +++++++++++++++++++
> >  fs/xfs/xfs_ondisk.h        |   12 ++++++++++++
> >  fs/xfs/xfs_super.c         |    5 +++--
> >  3 files changed, 34 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> > index be86fa1a5556..b1b8a5c05cea 100644
> > --- a/fs/xfs/libxfs/xfs_format.h
> > +++ b/fs/xfs/libxfs/xfs_format.h
> > @@ -849,11 +849,30 @@ struct xfs_agfl {
> >  	    ASSERT(xfs_daddr_to_agno(mp, d) == \
> >  		   xfs_daddr_to_agno(mp, (d) + (len) - 1)))
> >  
> > +/*
> > + * XFS Timestamps
> > + * ==============
> > + *
> > + * Inode timestamps consist of signed 32-bit counters for seconds and
> > + * nanoseconds; time zero is the Unix epoch, Jan  1 00:00:00 UTC 1970.
> > + */
> >  typedef struct xfs_timestamp {
> >  	__be32		t_sec;		/* timestamp seconds */
> >  	__be32		t_nsec;		/* timestamp nanoseconds */
> >  } xfs_timestamp_t;
> >  
> > +/*
> > + * Smallest possible timestamp with traditional timestamps, which is
> > + * Dec 13 20:45:52 UTC 1901.
> > + */
> > +#define XFS_INO_TIME_MIN	((int64_t)S32_MIN)
> > +
> > +/*
> > + * Largest possible timestamp with traditional timestamps, which is
> > + * Jan 19 03:14:07 UTC 2038.
> > + */
> > +#define XFS_INO_TIME_MAX	((int64_t)S32_MAX)
> 
> These are based on the Unix epoch. Can we call them something like
> XFS_INO_UNIX_TIME_{MIN,MAX} to indicate what epoch they reference?

I'll update the comment and name to make it clear that this is the Unix
epoch we're talking about.

> >  /*
> >   * On-disk inode structure.
> >   *
> > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> > index acb9b737fe6b..48a64fa49f91 100644
> > --- a/fs/xfs/xfs_ondisk.h
> > +++ b/fs/xfs/xfs_ondisk.h
> > @@ -15,6 +15,18 @@
> >  		"XFS: offsetof(" #structname ", " #member ") is wrong, " \
> >  		"expected " #off)
> >  
> > +#define XFS_CHECK_VALUE(value, expected) \
> > +	BUILD_BUG_ON_MSG((value) != (expected), \
> > +		"XFS: value of " #value " is wrong, expected " #expected)
> > +
> > +static inline void __init
> > +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);
> > +}
> 
> Not sure this really gains us anything? All it does is check that
> S32_MIN/S32_MAX haven't changed value....

Will remove.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
Darrick J. Wong Aug. 24, 2020, 4:29 p.m. UTC | #4
On Sat, Aug 22, 2020 at 08:12:18AM +0100, Christoph Hellwig wrote:
> > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> > index acb9b737fe6b..48a64fa49f91 100644
> > --- a/fs/xfs/xfs_ondisk.h
> > +++ b/fs/xfs/xfs_ondisk.h
> > @@ -15,6 +15,18 @@
> >  		"XFS: offsetof(" #structname ", " #member ") is wrong, " \
> >  		"expected " #off)
> >  
> > +#define XFS_CHECK_VALUE(value, expected) \
> > +	BUILD_BUG_ON_MSG((value) != (expected), \
> > +		"XFS: value of " #value " is wrong, expected " #expected)
> > +
> > +static inline void __init
> > +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);
> 
> I have to admit I don't get why you'd define a constant and then
> check that it has the value you defined it to.  Seems a little cargo
> cult.

Testing the timestamp min/max is more important for bigtime, since the
supported ranges are not conveniently aligned with S32_MIN/MAX.  But it
felt strange to build-check bigtime without doing the same for !bigtime,
so I included it here for completeness.

--D
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index be86fa1a5556..b1b8a5c05cea 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -849,11 +849,30 @@  struct xfs_agfl {
 	    ASSERT(xfs_daddr_to_agno(mp, d) == \
 		   xfs_daddr_to_agno(mp, (d) + (len) - 1)))
 
+/*
+ * XFS Timestamps
+ * ==============
+ *
+ * Inode timestamps consist of signed 32-bit counters for seconds and
+ * nanoseconds; time zero is the Unix epoch, Jan  1 00:00:00 UTC 1970.
+ */
 typedef struct xfs_timestamp {
 	__be32		t_sec;		/* timestamp seconds */
 	__be32		t_nsec;		/* timestamp nanoseconds */
 } xfs_timestamp_t;
 
+/*
+ * Smallest possible timestamp with traditional timestamps, which is
+ * Dec 13 20:45:52 UTC 1901.
+ */
+#define XFS_INO_TIME_MIN	((int64_t)S32_MIN)
+
+/*
+ * Largest possible timestamp with traditional timestamps, which is
+ * Jan 19 03:14:07 UTC 2038.
+ */
+#define XFS_INO_TIME_MAX	((int64_t)S32_MAX)
+
 /*
  * On-disk inode structure.
  *
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index acb9b737fe6b..48a64fa49f91 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -15,6 +15,18 @@ 
 		"XFS: offsetof(" #structname ", " #member ") is wrong, " \
 		"expected " #off)
 
+#define XFS_CHECK_VALUE(value, expected) \
+	BUILD_BUG_ON_MSG((value) != (expected), \
+		"XFS: value of " #value " is wrong, expected " #expected)
+
+static inline void __init
+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);
+}
+
 static inline void __init
 xfs_check_ondisk_structs(void)
 {
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index c7ffcb57b586..375f05a47ba4 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1484,8 +1484,8 @@  xfs_fc_fill_super(
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
 	sb->s_max_links = XFS_MAXLINK;
 	sb->s_time_gran = 1;
-	sb->s_time_min = S32_MIN;
-	sb->s_time_max = S32_MAX;
+	sb->s_time_min = XFS_INO_TIME_MIN;
+	sb->s_time_max = XFS_INO_TIME_MAX;
 	sb->s_iflags |= SB_I_CGROUPWB;
 
 	set_posix_acl_flag(sb);
@@ -2077,6 +2077,7 @@  init_xfs_fs(void)
 {
 	int			error;
 
+	xfs_check_limits();
 	xfs_check_ondisk_structs();
 
 	printk(KERN_INFO XFS_VERSION_STRING " with "