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@xxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
