Re: [RFC 02/15] vfs: Change all structures to support 64 bit time

From: Arnd Bergmann <arnd@arndb.de>
To: Dave Chinner <david@fromorbit.com>
Cc: Deepa Dinamani <deepa.kernel@gmail.com>,
	Andreas Dilger <adilger@dilger.ca>,
	"y2038@lists.linaro.org" <y2038@lists.linaro.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC 02/15] vfs: Change all structures to support 64 bit time
Date: Tue, 19 Jan 2016 23:25:02 +0100	[thread overview]
Message-ID: <14988333.dbUrT9DQnz@wuerfel> (raw)
In-Reply-To: <20160119204946.GA20456@dastard>

On Wednesday 20 January 2016 07:49:46 Dave Chinner wrote:
> On Mon, Jan 18, 2016 at 09:27:13PM -0800, Deepa Dinamani wrote:
> > On Mon, Jan 18, 2016 at 5:38 PM, Dave Chinner <david@fromorbit.com> wrote:
> > > On Mon, Jan 18, 2016 at 10:46:07PM +0100, Arnd Bergmann wrote:
> > >> On Tuesday 19 January 2016 08:14:59 Dave Chinner wrote:
> > >> > On Mon, Jan 18, 2016 at 08:53:22PM +0100, Arnd Bergmann wrote:
> > 
> > Let's back out a bit and consider a few changes with the suggested "abstraction":
> > 
> > original code:
> > 
> > extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec *ts,
> > __le16 __time, __le16 __date, u8 time_cs);
> > 
> > fat_time_fat2unix(sbi, &inode->i_mtime, de->time, de->date, 0);
> > 
> > becomes ugly
> > 
> > extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec64 *ts,
> > __le16 __time, __le16 __date, u8 time_cs);
> > 
> > struct timespec64 mtime = vfs_time_to_timespec64(i_mtime, inode);
> > fat_time_fat2unix(sbi, &mtime, de->time, de->date, 0);
> 
> You're doing it wrong. fat_time_fat2unix() still gets passed
> &inode->i_mtime, and the function prototype is changed to a
> timespec64.  *Nothing else needs to change*, because
> fat_time_fat2unix() does it own calculations and then stores the
> time directly into the timespec structure members....

That puts us back at the 'one big patch' problem: We can't change
fat_time_fat2unix() to pass a timespec64 until we also change 
struct inode. The change may be small, but I see roughly 30 file
systems that assign i_?time into or from a local variable or pass it
into by reference into a function that is not from VFS.

see http://pastebin.com/BSnwJa1N for a list (certainly some false
positives and some false negatives in there)

Roughly two thirds of the instances can be handled easily using
vfs_time_to_timespec(), the others could be done much nicer
with additional helpers such as inode_timespec_compare()

> I think you're making a mountain out of a molehill. Most filesystems
> will be unchanged except for s/timespec/timespec64/ as they store
> values directly into timespec members when encoding/decoding. There
> is no need for timestamp conversion in places like this - you're
> simply not looking deep enough and applying the conversion at the
> wrong layer.

Any idea how to improve this somewhat lacking patch?

diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
index b97f1df910ab..7fbb07dcad36 100644
--- a/fs/xfs/xfs_trans_inode.c
+++ b/fs/xfs/xfs_trans_inode.c
@@ -68,22 +68,24 @@ xfs_trans_ichgtime(
 	int			flags)
 {
 	struct inode		*inode = VFS_I(ip);
-	struct timespec		tv;
+	struct timespec		tv, mtime, ctime;
 
 	ASSERT(tp);
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
-	tv = current_fs_time(inode->i_sb);
+	tv = vfs_time_to_timespec(current_fs_time(inode->i_sb));
+	mtime = vfs_time_to_timespec(inode->i_mtime);
+	ctime = vfs_time_to_timespec(inode->i_ctime);
 
 	if ((flags & XFS_ICHGTIME_MOD) &&
-	    !timespec_equal(&inode->i_mtime, &tv)) {
-		inode->i_mtime = tv;
+	    !timespec_equal(&mtime, &tv)) {
+		inode->i_mtime = timespec_to_vfs_time(tv);
 		ip->i_d.di_mtime.t_sec = tv.tv_sec;
 		ip->i_d.di_mtime.t_nsec = tv.tv_nsec;
 	}
 	if ((flags & XFS_ICHGTIME_CHG) &&
-	    !timespec_equal(&inode->i_ctime, &tv)) {
-		inode->i_ctime = tv;
+	    !timespec_equal(&ctime, &tv)) {
+		inode->i_ctime = timespec_to_vfs_time(tv);
 		ip->i_d.di_ctime.t_sec = tv.tv_sec;
 		ip->i_d.di_ctime.t_nsec = tv.tv_nsec;
 	}


The way that Deepa suggests I think would turn out as:

diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
index b97f1df910ab..54fc3c41047a 100644
--- a/fs/xfs/xfs_trans_inode.c
+++ b/fs/xfs/xfs_trans_inode.c
@@ -68,7 +68,7 @@ xfs_trans_ichgtime(
 	int			flags)
 {
 	struct inode		*inode = VFS_I(ip);
-	struct timespec		tv;
+	struct vfs_time		tv;
 
 	ASSERT(tp);
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
@@ -76,13 +76,13 @@ xfs_trans_ichgtime(
 	tv = current_fs_time(inode->i_sb);
 
 	if ((flags & XFS_ICHGTIME_MOD) &&
-	    !timespec_equal(&inode->i_mtime, &tv)) {
+	    !vfs_time_equal(&inode->i_mtime, &tv)) {
 		inode->i_mtime = tv;
 		ip->i_d.di_mtime.t_sec = tv.tv_sec;
 		ip->i_d.di_mtime.t_nsec = tv.tv_nsec;
 	}
 	if ((flags & XFS_ICHGTIME_CHG) &&
-	    !timespec_equal(&inode->i_ctime, &tv)) {
+	    !vfs_time_equal(&inode->i_ctime, &tv)) {
 		inode->i_ctime = tv;
 		ip->i_d.di_ctime.t_sec = tv.tv_sec;
 		ip->i_d.di_ctime.t_nsec = tv.tv_nsec;


which I would much prefer here.

	Arnd
