Delete timespec64_trunc()
Subsystems (7)
cover Delete timespec64_trunc()
This series aims at deleting timespec64_trunc(). There is a new api: timestamp_truncate() that is the replacement api. The api additionally does a limits check on the filesystem timestamps.
The suggestion to open code some of the truncate logic came from Al Viro. And, this does make the code in some filesystems easy to follow.
The series also does some update_time() cleanup as suggested by Al Viro.
Deepa Dinamani (7):
fs: fat: Eliminate timespec64_trunc() usage
fs: cifs: Fix atime update check vs mtime
fs: cifs: Delete usage of timespec64_trunc
fs: ceph: Delete timespec64_trunc() usage
fs: ubifs: Eliminate timespec64_trunc() usage
fs: Delete timespec64_trunc()
fs: Do not overload update_time
fs/ceph/mds_client.c | 3 +-- fs/cifs/inode.c | 15 ++++++++------- fs/fat/misc.c | 10 +++++++++- fs/inode.c | 30 +++--------------------------- fs/ubifs/sb.c | 11 ++++------- include/linux/fs.h | 1 - 6 files changed, 25 insertions(+), 45 deletions(-)
Signature · 9 lines
2.17.1 Cc: [email protected] Cc: [email protected] Cc: [email protected] Cc: [email protected] Cc: [email protected] Cc: [email protected] Cc: [email protected]
PATCH 1/7 fs: fat: Eliminate timespec64_trunc() usage
timespec64_trunc() is being deleted.
timestamp_truncate() is the replacement api for timespec64_trunc. timestamp_truncate() additionally clamps timestamps to make sure the timestamps lie within the permitted range for the filesystem.
But, fat always truncates the times locally after it obtains the timestamps from current_time().
Implement a local version here along the lines of existing truncate functions.
--- fs/fat/misc.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
fs/fat/misc.c +9 -1
@@ -271,6 +271,14 @@ static inline struct timespec64 fat_timespec64_trunc_2secs(struct timespec64 ts) {return (struct timespec64 ){ ts.tv_sec & ~1ULL ,0 }; } + +static inline struct timespec64 fat_timespec64_trunc_10ms (struct timespec64 ts) +{ +if (ts.tv_nsec) + ts.tv_nsec -= ts.tv_nsec %10000000UL ; +return ts; +} +/* * truncate the various times with appropriate granularity: * root inode:
@@ -308,7 +316,7 @@ int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags) }if (flags &S_CTIME ) {if (sbi->options.isvfat) - inode->i_ctime =timespec64_trunc (*now,10000000 ); + inode->i_ctime =fat_timespec64_trunc_10ms (*now);else inode->i_ctime =fat_timespec64_trunc_2secs (*now); }
Signature · 1 line
2.17.1
PATCH 2/7 fs: cifs: Fix atime update check vs mtime
In reply to Deepa DinamaniAccording to the comment in the code and commit log, some apps expect atime >= mtime; but the introduced code results in atime==mtime. Fix the comparison to guard against atime<mtime.
--- fs/cifs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/cifs/inode.c +1 -1
@@ -163,7 +163,7 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)spin_lock (&inode->i_lock);/* we do not want atime to be less than mtime, it broke some apps */ -if (timespec64_compare (&fattr->cf_atime, &fattr->cf_mtime)) +if (timespec64_compare (&fattr->cf_atime, &fattr->cf_mtime) <0 ) inode->i_atime = fattr->cf_mtime;else inode->i_atime = fattr->cf_atime;
Signature · 1 line
2.17.1
Steve French
AppliedRe: [PATCH 2/7] fs: cifs: Fix atime update check vs mtime
merged into cifs-2.6.git for-next
Quoted 29 lines · Deepa Dinamani
According to the comment in the code and commit log, some apps expect atime >= mtime; but the introduced code results in atime==mtime. Fix the comparison to guard against atime<mtime.
Fixes 9b9c5bea0b96 ("cifs: do not return atime less than mtime")Signed-off-by Deepa Dinamani <[email protected]>--- fs/cifs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/cifs/inode.c +1
-2@@ -163,7 +163,7 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)spin_lock (&inode->i_lock);/* we do not want atime to be less than mtime, it broke some apps */ -if (timespec64_compare (&fattr->cf_atime, &fattr->cf_mtime)) +if (timespec64_compare (&fattr->cf_atime, &fattr->cf_mtime) <0 ) inode->i_atime = fattr->cf_mtime;else inode->i_atime = fattr->cf_atime; --2.17.1
Signature · 3 lines
Thanks, Steve
PATCH 3/7 fs: cifs: Delete usage of timespec64_trunc
In reply to Deepa Dinamanitimestamp_truncate() is the replacement api for timespec64_trunc. timestamp_truncate() additionally clamps timestamps to make sure the timestamps lie within the permitted range for the filesystem.
Truncate the timestamps in the struct cifs_attr at the site of assignment to inode times. This
helps us use the right fs api timestamp_trucate() to perform the truncation.
Also update the ktime_get_* api to match the one used in current_time(). This allows for timestamps to be updated the same way always.
--- fs/cifs/inode.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
fs/cifs/inode.c +7 -6
@@ -113,6 +113,7 @@ cifs_revalidate_cache(struct inode *inode, struct cifs_fattr *fattr) }/* revalidate if mtime or size have changed */ + fattr->cf_mtime =timestamp_truncate (fattr->cf_mtime, inode);if (timespec64_equal (&inode->i_mtime, &fattr->cf_mtime) && cifs_i->server_eof == fattr->cf_eof) { cifs_dbg(FYI, "%s: inode %llu is unchanged\n",
@@ -162,6 +163,9 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)cifs_revalidate_cache (inode, fattr);spin_lock (&inode->i_lock); + fattr->cf_mtime =timestamp_truncate (fattr->cf_mtime, inode); + fattr->cf_atime =timestamp_truncate (fattr->cf_atime, inode); + fattr->cf_ctime =timestamp_truncate (fattr->cf_ctime, inode);/* we do not want atime to be less than mtime, it broke some apps */ if (timespec64_compare (&fattr->cf_atime, &fattr->cf_mtime) <0 ) inode->i_atime = fattr->cf_mtime;
@@ -329,8 +333,7 @@ cifs_create_dfs_fattr(struct cifs_fattr *fattr, struct super_block *sb) fattr->cf_mode =S_IFDIR |S_IXUGO |S_IRWXU ; fattr->cf_uid = cifs_sb->mnt_uid; fattr->cf_gid = cifs_sb->mnt_gid; -ktime_get_real_ts64 (&fattr->cf_mtime); - fattr->cf_mtime =timespec64_trunc (fattr->cf_mtime, sb->s_time_gran); +ktime_get_coarse_real_ts64 (&fattr->cf_mtime); fattr->cf_atime = fattr->cf_ctime = fattr->cf_mtime; fattr->cf_nlink =2 ; fattr->cf_flags =CIFS_FATTR_DFS_REFERRAL ;
@@ -609,10 +612,8 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,if (info->LastAccessTime) fattr->cf_atime =cifs_NTtimeToUnix (info->LastAccessTime); -else { -ktime_get_real_ts64 (&fattr->cf_atime); - fattr->cf_atime =timespec64_trunc (fattr->cf_atime, sb->s_time_gran); - } +else +ktime_get_coarse_real_ts64 (&fattr->cf_atime); fattr->cf_ctime =cifs_NTtimeToUnix (info->ChangeTime); fattr->cf_mtime =cifs_NTtimeToUnix (info->LastWriteTime);
Signature · 1 line
2.17.1
Re: [PATCH 3/7] fs: cifs: Delete usage of timespec64_trunc
Is this intended to merge separately or do you want it merged through the cifs git tree?
Quoted 70 lines · Deepa Dinamani
timestamp_truncate()is the replacement api fortimespec64_trunc.timestamp_truncate()additionally clamps timestamps to make sure the timestamps lie within the permitted range for the filesystem.Truncate the timestamps in the struct
cifs_attrat the site of assignment to inode times. This
helps us use the right fs apitimestamp_trucate()to perform the truncation.Also update the ktime_get_* api to match the one used in
current_time(). This allows for timestamps to be updated the same way always.--- fs/cifs/inode.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)fs/cifs/inode.c +7
-7@@ -113,6 +113,7 @@ cifs_revalidate_cache(struct inode *inode, struct cifs_fattr *fattr) }/* revalidate if mtime or size have changed */ + fattr->cf_mtime =timestamp_truncate (fattr->cf_mtime, inode);if (timespec64_equal (&inode->i_mtime, &fattr->cf_mtime) && cifs_i->server_eof == fattr->cf_eof) { cifs_dbg(FYI, "%s: inode %llu is unchanged\n",@@ -162,6 +163,9 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)cifs_revalidate_cache (inode, fattr);spin_lock (&inode->i_lock); + fattr->cf_mtime =timestamp_truncate (fattr->cf_mtime, inode); + fattr->cf_atime =timestamp_truncate (fattr->cf_atime, inode); + fattr->cf_ctime =timestamp_truncate (fattr->cf_ctime, inode);/* we do not want atime to be less than mtime, it broke some apps */ if (timespec64_compare (&fattr->cf_atime, &fattr->cf_mtime) <0 ) inode->i_atime = fattr->cf_mtime;@@ -329,8 +333,7 @@ cifs_create_dfs_fattr(struct cifs_fattr *fattr, struct super_block *sb) fattr->cf_mode =S_IFDIR |S_IXUGO |S_IRWXU ; fattr->cf_uid = cifs_sb->mnt_uid; fattr->cf_gid = cifs_sb->mnt_gid; -ktime_get_real_ts64 (&fattr->cf_mtime); - fattr->cf_mtime =timespec64_trunc (fattr->cf_mtime, sb->s_time_gran); +ktime_get_coarse_real_ts64 (&fattr->cf_mtime); fattr->cf_atime = fattr->cf_ctime = fattr->cf_mtime; fattr->cf_nlink =2 ; fattr->cf_flags =CIFS_FATTR_DFS_REFERRAL ;@@ -609,10 +612,8 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,if (info->LastAccessTime) fattr->cf_atime =cifs_NTtimeToUnix (info->LastAccessTime); -else { -ktime_get_real_ts64 (&fattr->cf_atime); - fattr->cf_atime =timespec64_trunc (fattr->cf_atime, sb->s_time_gran); - } +else +ktime_get_coarse_real_ts64 (&fattr->cf_atime); fattr->cf_ctime =cifs_NTtimeToUnix (info->ChangeTime); fattr->cf_mtime =cifs_NTtimeToUnix (info->LastWriteTime); --2.17.1
Signature · 3 lines
Thanks, Steve
Re: [PATCH 3/7] fs: cifs: Delete usage of timespec64_trunc
Quoted 3 lines · Steve French
Is this intended to merge separately or do you want it merged through the cifs git tree?
It would be simplest for all uses of timespec64_trunc to be removed through the same tree. Otherwise, whoever takes the [PATCH 6/7] ("fs:
Delete timespec64_trunc()") would have to depend on your tree.
Thanks,
-Deepa
PATCH 4/7 fs: ceph: Delete timespec64_trunc() usage
In reply to Deepa DinamaniSince ceph always uses ns granularity, skip the
truncation which is a no-op.
--- fs/ceph/mds_client.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
fs/ceph/mds_client.c +1 -2
@@ -2088,8 +2088,7 @@ ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode)init_completion (&req->r_safe_completion);INIT_LIST_HEAD (&req->r_unsafe_item); -ktime_get_coarse_real_ts64 (&ts); - req->r_stamp =timespec64_trunc (ts, mdsc->fsc->sb->s_time_gran); +ktime_get_coarse_real_ts64 (&req->r_stamp); req->r_op = op; req->r_direct_mode = mode;
Signature · 1 line
2.17.1
PATCH 5/7 fs: ubifs: Eliminate timespec64_trunc() usage
In reply to Deepa DinamaniDEFAULT_TIME_GRAN is seconds granularity. We can just drop the nsec while creating the default root node. Delete the unneeded call to timespec64_trunc().
Also update the ktime_get_* api to match the one used in current_time(). This allows for the timestamps to be updated by using the same ktime_get_* api always.
--- fs/ubifs/sb.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
fs/ubifs/sb.c +4 -7
@@ -84,7 +84,6 @@ static int create_default_filesystem(struct ubifs_info *c)int idx_node_size;long long tmp64, main_bytes;__le64 tmp_le64; -__le32 tmp_le32;struct timespec64 ts;u8 hash[UBIFS_HASH_ARR_SZ ];u8 hash_lpt[UBIFS_HASH_ARR_SZ ];
@@ -291,16 +290,14 @@ static int create_default_filesystem(struct ubifs_info *c) ino->creat_sqnum =cpu_to_le64 (++c->max_sqnum); ino->nlink =cpu_to_le32 (2 ); -ktime_get_real_ts64 (&ts); - ts =timespec64_trunc (ts,DEFAULT_TIME_GRAN ); +ktime_get_coarse_real_ts64 (&ts); tmp_le64 =cpu_to_le64 (ts.tv_sec); ino->atime_sec = tmp_le64; ino->ctime_sec = tmp_le64; ino->mtime_sec = tmp_le64; - tmp_le32 =cpu_to_le32 (ts.tv_nsec); - ino->atime_nsec = tmp_le32; - ino->ctime_nsec = tmp_le32; - ino->mtime_nsec = tmp_le32; + ino->atime_nsec =0 ; + ino->ctime_nsec =0 ; + ino->mtime_nsec =0 ; ino->mode =cpu_to_le32 (S_IFDIR |S_IRUGO |S_IWUSR |S_IXUGO ); ino->size =cpu_to_le64 (UBIFS_INO_NODE_SZ );
Signature · 1 line
2.17.1
PATCH 6/7 fs: Delete timespec64_trunc()
In reply to Deepa DinamaniThere are no more callers to the function remaining.
--- fs/inode.c | 24 ------------------------ include/linux/fs.h | 1 - 2 files changed, 25 deletions(-)
fs/inode.c +0 -24
@@ -2145,30 +2145,6 @@ void inode_nohighmem(struct inode *inode) }EXPORT_SYMBOL (inode_nohighmem); -/** -* timespec64_trunc - Truncate timespec64 to a granularity -* @t: Timespec64 -* @gran: Granularity in ns. -* -* Truncate a timespec64 to a granularity. Always rounds down. gran must -* not be 0 nor greater than a second (NSEC_PER_SEC, or 10^9 ns). -*/ -struct timespec64 timespec64_trunc (struct timespec64 t,unsigned gran) -{ -/* Avoid division in the common cases 1 ns and 1 s. */ -if (gran ==1 ) { -/* nothing */ - }else if (gran ==NSEC_PER_SEC ) { - t.tv_nsec =0 ; - }else if (gran >1 && gran <NSEC_PER_SEC ) { - t.tv_nsec -= t.tv_nsec % gran; - }else { -WARN (1 ,"illegal file time granularity: %u" , gran); - } -return t; -} -EXPORT_SYMBOL (timespec64_trunc); -/** * timestamp_truncate - Truncate timespec to a granularity * @t: Timespec
include/linux/fs.h +0 -1
@@ -1575,7 +1575,6 @@ static inline void i_gid_write(struct inode *inode, gid_t gid) inode->i_gid =make_kgid (inode->i_sb->s_user_ns, gid); } -extern struct timespec64 timespec64_trunc (struct timespec64 t,unsigned gran);extern struct timespec64 current_time (struct inode *inode);/*
Signature · 1 line
2.17.1
PATCH 7/7 fs: Do not overload update_time
In reply to Deepa Dinamaniupdate_time() also has an internal function pointer update_time. Even though this works correctly, it is confusing to the readers.
Use a different name for the local variable.
--- fs/inode.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
fs/inode.c +3 -3
@@ -1675,12 +1675,12 @@ EXPORT_SYMBOL(generic_update_time);*/ static int update_time (struct inode *inode,struct timespec64 *time,int flags) { -int (*update_time)(struct inode *,struct timespec64 *,int ); +int (*cb)(struct inode *,struct timespec64 *,int ); - update_time = inode->i_op->update_time ? inode->i_op->update_time : + cb = inode->i_op->update_time ? inode->i_op->update_time : generic_update_time; -return update_time (inode, time, flags); +return cb (inode, time, flags); }/**
Signature · 1 line
2.17.1
Re: [PATCH 7/7] fs: Do not overload update_time
Quoted 19 lines · Deepa Dinamani
update_time()also has an internal function pointerupdate_time. Even though this works correctly, it is confusing to the readers.Use a different name for the local variable.
Suggested-by Al Viro <[email protected]>Signed-off-by Deepa Dinamani <[email protected]>--- fs/inode.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)fs/inode.c +0
-0@@ -1675,12 +1675,12 @@ EXPORT_SYMBOL(generic_update_time);*/ static int update_time (struct inode *inode,struct timespec64 *time,int flags)
fs/inode.c EXPORT_SYMBOL(generic_update_time); · line 1683
Quoted 10 lines
fs/inode.c +3
-3{ -int (*update_time)(struct inode *,struct timespec64 *,int ); +int (*cb)(struct inode *,struct timespec64 *,int ); - update_time = inode->i_op->update_time ? inode->i_op->update_time : + cb = inode->i_op->update_time ? inode->i_op->update_time : generic_update_time; -return update_time (inode, time, flags); +return cb (inode, time, flags);
What's wrong with a simple if() like we use everywhere else for this sort of thing?
if (inode->i_op->update_time) return inode->i_op->update_time(inode, time, flags); return generic_update_time(inode, time, flags);
-Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH 7/7] fs: Do not overload update_time
In reply to Deepa Dinamanifs/inode.c EXPORT_SYMBOL(generic_update_time); · line 1683
Quoted 9 lines · Deepa Dinamani
fs/inode.c +3
-3-int (*update_time)(struct inode *,struct timespec64 *,int ); +int (*cb)(struct inode *,struct timespec64 *,int ); - update_time = inode->i_op->update_time ? inode->i_op->update_time : + cb = inode->i_op->update_time ? inode->i_op->update_time : generic_update_time; -return update_time (inode, time, flags); +return cb (inode, time, flags);
Just killing the pointless local variable cleans the function op, and also avoids the expensive indirect call for the common case:
if (inode->i_op->update_time) return inode->i_op->update_time(inode, time, flags); return generic_update_time(inode, time, flags);
Re: [PATCH 7/7] fs: Do not overload update_time
Quoted 9 lines · Christoph Hellwig
Quoted 7 lines · Deepa Dinamani
fs/inode.c +2
-2-int (*update_time)(struct inode *,struct timespec64 *,int ); +int (*cb)(struct inode *,struct timespec64 *,int ); - update_time = inode->i_op->update_time ? inode->i_op->update_time : + cb = inode->i_op->update_time ? inode->i_op->update_time : generic_update_time;
Quoted 5 lines
fs/inode.c EXPORT_SYMBOL(generic_update_time); · line 1683
Quoted 2 lines
fs/inode.c +1
-1-return update_time (inode, time, flags); +return cb (inode, time, flags);Just killing the pointless local variable cleans the function op, and also avoids the expensive indirect call for the common case:
Will update this in v2. Thanks.
-Deepa