From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755285AbcATXHY (ORCPT ); Wed, 20 Jan 2016 18:07:24 -0500 Received: from ipmail04.adl6.internode.on.net ([150.101.137.141]:65174 "EHLO ipmail04.adl6.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751194AbcATXHU (ORCPT ); Wed, 20 Jan 2016 18:07:20 -0500 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: A2DIDACvEqBWPGwtLHldKAECgw+BP4ZfgXieZQEBAQEBAQaLXIlGhgkEAgKBQE0BAQEBAQEHAQEBAUE/hDQBAQEDAScTHCMFCwgDGAklDwUlAwcaE4gTB7w9AQEBBwIeGIVahTyJDAWXBI1WjwmOQYRdKC6HEgEBAQ Date: Thu, 21 Jan 2016 10:06:32 +1100 From: Dave Chinner To: Arnd Bergmann Cc: Deepa Dinamani , Andreas Dilger , "y2038@lists.linaro.org" , "linux-fsdevel@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Message-ID: <20160120230632.GZ6033@dastard> References: <1452144972-15802-1-git-send-email-deepa.kernel@gmail.com> <20160119052713.GA20081@myubuntu.deepaubuntu.g7.internal.cloudapp.net> <20160119204946.GA20456@dastard> <14988333.dbUrT9DQnz@wuerfel> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <14988333.dbUrT9DQnz@wuerfel> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 19, 2016 at 11:25:02PM +0100, Arnd Bergmann wrote: > On Wednesday 20 January 2016 07:49:46 Dave Chinner wrote: > > 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.... > > 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; > } WTF? That's insane and completely unnecessary. It's even worse than the FAT example I've already pointed out was just fucking wrong. The only change this function requires is: > 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; + struct timespec64 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)) { + !timespec64_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)) { + !timespec64_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. IOWs, you're now finally suggesting doing the *simple conversion* I've been suggesting needs to be made *since the start* of this long, frustrating thread, except you *still want to abstract the timestamp unnecessarily*. For the last time: use timespec64 directly, do not abstract it in any way. -Dave. -- Dave Chinner david@fromorbit.com