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@xxxxxxxxxxxxx
--
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
