[PATCH] afs: use a consistent interpretation of time values

[PATCH] afs: use a consistent interpretation of time values

[Arnd Bergmann]

afs uses 32-bit timestamps everywhere, but mixes signed and unsigned
usage, which is a bit inconsistent. In particular on 32-bit machines,
it currently uses unsigned timestamps (ranging from 1970 to 2106) for
locally modified files, but signed timestamps (rand 1902 to 2038) when
reading from a remote end. On 64-bit machines, we always interpret
timestamps as unsigned here.

This replaces the deprecated time_t and get_seconds() interfaces with the
modern time64_t and current_time() to locally store 64-bit timestamps,
taking care to use unsigned interpretation of the raw values everywhere,
which avoids the y2038 overflow and is consistent with the previous
usage on 64-bit machines.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/afs/afs.h   | 6 +++---
 fs/afs/inode.c | 4 +---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/afs/afs.h b/fs/afs/afs.h
index b4ff1f7ae4ab..6ca50c293553 100644
--- a/fs/afs/afs.h
+++ b/fs/afs/afs.h
@@ -129,8 +129,8 @@ typedef u32 afs_access_t;
 struct afs_file_status {
 	u64			size;		/* file size */
 	afs_dataversion_t	data_version;	/* current data version */
-	time_t			mtime_client;	/* last time client changed data */
-	time_t			mtime_server;	/* last time server changed data */
+	time64_t		mtime_client;	/* last time client changed data */
+	time64_t		mtime_server;	/* last time server changed data */
 	unsigned		abort_code;	/* Abort if bulk-fetching this failed */
 
 	afs_file_type_t		type;		/* file type */
@@ -158,7 +158,7 @@ struct afs_file_status {
  * AFS volume synchronisation information
  */
 struct afs_volsync {
-	time_t			creation;	/* volume creation time */
+	time64_t		creation;	/* volume creation time */
 };
 
 /*
diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 479b7fdda124..0507e52e3330 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -213,9 +213,7 @@ struct inode *afs_iget_pseudo_dir(struct super_block *sb, bool root)
 	set_nlink(inode, 2);
 	inode->i_uid		= GLOBAL_ROOT_UID;
 	inode->i_gid		= GLOBAL_ROOT_GID;
-	inode->i_ctime.tv_sec	= get_seconds();
-	inode->i_ctime.tv_nsec	= 0;
-	inode->i_atime		= inode->i_mtime = inode->i_ctime;
+	inode->i_ctime = inode->i_atime = inode->i_mtime = current_time(inode);
 	inode->i_blocks		= 0;
 	inode_set_iversion_raw(inode, 0);
 	inode->i_generation	= 0;
-- 
2.9.0

Re: [PATCH] afs: use a consistent interpretation of time values

[David Howells]

Arnd Bergmann <arnd@arndb.de> wrote:

> which avoids the y2038 overflow

No it doesn't.  The AFS protocol is limited.

> +	time64_t		mtime_client;	/* last time client changed data */
> +	time64_t		mtime_server;	/* last time server changed data */
> ...
> -	time_t			creation;	/* volume creation time */
> +	time64_t		creation;	/* volume creation time */

Unless you can change the AFS protocol, this is a waste of memory.  It might
be better to change them to u32 as they are protocol values rather than system
values.

> -	inode->i_ctime.tv_sec	= get_seconds();
> -	inode->i_ctime.tv_nsec	= 0;
> -	inode->i_atime		= inode->i_mtime = inode->i_ctime;
> +	inode->i_ctime = inode->i_atime = inode->i_mtime = current_time(inode);

Surely, the tv_nsec should be zero since anything else cannot be represented
in the AFS protocol.

I will grant, however, I should be consistently using them as unsigned values.

Note that the answers to the above may change if and when I start supporting
the YFS protocol extensions, but for the AFS protocol, this is simply not
there.

David

Re: [PATCH] afs: use a consistent interpretation of time values

[Arnd Bergmann]

On Wed, Jun 20, 2018 at 12:53 PM, David Howells <dhowells@redhat.com> wrote:
> Arnd Bergmann <arnd@arndb.de> wrote:
>
>> which avoids the y2038 overflow
>
> No it doesn't.  The AFS protocol is limited.
>
>> +     time64_t                mtime_client;   /* last time client changed data */
>> +     time64_t                mtime_server;   /* last time server changed data */
>> ...
>> -     time_t                  creation;       /* volume creation time */
>> +     time64_t                creation;       /* volume creation time */
>
> Unless you can change the AFS protocol, this is a waste of memory.  It might
> be better to change them to u32 as they are protocol values rather than system
> values.

AFS uses 'unsigned' seconds, right? What I was trying to say there is
that with the patch, the 32-bit overflow gets moved from 2038 to 2106, so at
least the nearer problem is solved.

On 64-bit machines, we already waste a little memory here, the usual
tradeoff I took was to use time64_t for all time storage when possible for
clarity reasons, but that is easily changed if you prefer.

>> -     inode->i_ctime.tv_sec   = get_seconds();
>> -     inode->i_ctime.tv_nsec  = 0;
>> -     inode->i_atime          = inode->i_mtime = inode->i_ctime;
>> +     inode->i_ctime = inode->i_atime = inode->i_mtime = current_time(inode);
>
> Surely, the tv_nsec should be zero since anything else cannot be represented
> in the AFS protocol.

current_time() truncates the nanoseconds to the granularity of the filesystem.
Since AFS doesn't set s_time_gran, it gets the default 1000000000 value
leads to tv_nsec being zero. Once Deepa's patch to truncate the tv_sec
range lands, it will also ensure that this is within the range (this is less
of a problem for setting the current time than it is for utimensat() which
can set arbitrary future timestamps of course).

> I will grant, however, I should be consistently using them as unsigned values.
>
> Note that the answers to the above may change if and when I start supporting
> the YFS protocol extensions, but for the AFS protocol, this is simply not
> there.

Ok, good to know this exists.

      Arnd