Delete timespec64_trunc()

PATCH In review Deepa Dinamani 3 participants 15 messages Dec 3, 2019 – Dec 9, 2019

Subsystems (7)

VFS Memory technology devices (MTD) SMB client (CIFS) Kernel SMB3 server (ksmbd) +3 more subsystems

lore

Share & tools

lore

Deepa Dinamani

author

2019-12-03 05:19 UTC #0 lore raw

v2 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 (6):
fs: fat: Eliminate timespec64_trunc() usage
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 |  4 +---
 fs/cifs/inode.c      | 13 +++++++------
 fs/fat/misc.c        | 10 +++++++++-
 fs/inode.c           | 33 +++------------------------------
 fs/ubifs/sb.c        | 11 ++++-------
 include/linux/fs.h   |  1 -
 6 files changed, 24 insertions(+), 48 deletions(-)

Signature · 12 lines

Changes since v1:
* Dropped the atime comparison (patch 2/7) taken through cifs tree.
* Refactored update_time according to review comments.
2.17.1

Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

Deepa Dinamani

author

2019-12-03 05:19 UTC #3 lore raw

PATCH v2 3/6 fs: ceph: Delete timespec64_trunc() usage

Since ceph always uses ns granularity, skip the
truncation which is a no-op.

Signed-off-by Deepa Dinamani <[email protected]>

Cc [email protected]

---
 fs/ceph/mds_client.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

fs/ceph/mds_client.c +1 -3

index 068b029cf073..88687ed65cff 100644
@@ -2069,7 +2069,6 @@ struct ceph_mds_request *
 ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode)
 {
 	struct ceph_mds_request *req = kzalloc(sizeof(*req), GFP_NOFS);
-	struct timespec64 ts;
 
 	if (!req)
 		return ERR_PTR(-ENOMEM);

@@ -2088,8 +2087,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

Deepa Dinamani

author

2019-12-03 05:20 UTC #1 lore raw

PATCH v2 1/6 fs: fat: Eliminate timespec64_trunc() usage

In reply to Deepa Dinamani

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.

Suggested-by Al Viro <[email protected]>

Signed-off-by Deepa Dinamani <[email protected]>

Cc [email protected]

---
 fs/fat/misc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

fs/fat/misc.c +9 -1

index 1e08bd54c5fb..f1b2a1fc2a6a 100644
@@ -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

Deepa Dinamani

author

2019-12-03 05:20 UTC #2 lore raw

PATCH v2 2/6 fs: cifs: Delete usage of timespec64_trunc

In reply to Deepa Dinamani

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.

Signed-off-by Deepa Dinamani <[email protected]>

Cc [email protected]

---
 fs/cifs/inode.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

fs/cifs/inode.c +7 -6

index ca76a9287456..026ed49e8aa4 100644
@@ -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

Deepa Dinamani

author

2019-12-03 05:20 UTC #7 lore raw

PATCH v2 4/6 fs: ubifs: Eliminate timespec64_trunc() usage

In reply to Deepa Dinamani

DEFAULT_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.

Signed-off-by Deepa Dinamani <[email protected]>

Cc [email protected]

---
 fs/ubifs/sb.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

fs/ubifs/sb.c +4 -7

index 2b7c04bf8983..93d550be4c11 100644
@@ -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

Deepa Dinamani

author

2019-12-03 05:20 UTC #8 lore raw

PATCH v2 5/6 fs: Delete timespec64_trunc()

In reply to Deepa Dinamani

There are no more callers to the function remaining.

Signed-off-by Deepa Dinamani <[email protected]>

---
 fs/inode.c         | 24 ------------------------
 include/linux/fs.h |  1 -
 2 files changed, 25 deletions(-)

fs/inode.c +0 ~~-24~~

index fef457a42882..12c9e38529c9 100644
@@ -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

index 98e0349adb52..46dd7e6f6d73 100644
@@ -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

Deepa Dinamani

author

2019-12-03 05:20 UTC #9 lore raw

PATCH v2 6/6 fs: Do not overload update_time

In reply to Deepa Dinamani

update_time() also has an internal function pointer update_time. Even though this works correctly, it is confusing to the readers.

Just use a regular if statement to call the generic function or the function pointer.

Suggested-by Al Viro <[email protected]>

Signed-off-by Deepa Dinamani <[email protected]>

---
 fs/inode.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

fs/inode.c +3 -6

index 12c9e38529c9..aff2b5831168 100644
@@ -1675,12 +1675,9 @@ 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);
-
-	update_time = inode->i_op->update_time ? inode->i_op->update_time :
-		generic_update_time;
-
-	return update_time(inode, time, flags);
+	if (inode->i_op->update_time)
+		return inode->i_op->update_time(inode, time, flags);
+	return generic_update_time(inode, time, flags);
 }
 
 /**

Signature · 1 line

2.17.1

Jeff Layton

2019-12-03 18:55 UTC #4 lore raw

Re: [PATCH v2 3/6] fs: ceph: Delete timespec64_trunc() usage

In reply to Deepa Dinamani

Quoted 23 lines · Deepa Dinamani

Since ceph always uses ns granularity, skip the
truncation which is a no-op.
Signed-off-by Deepa Dinamani <[email protected]>
Cc [email protected]
Cc [email protected]
---
 fs/ceph/mds_client.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
fs/ceph/mds_client.c +0 -1
index 068b029cf073..88687ed65cff 100644
@@ -2069,7 +2069,6 @@ struct ceph_mds_request *
 ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode)
 {
 	struct ceph_mds_request *req = kzalloc(sizeof(*req), GFP_NOFS);
-	struct timespec64 ts;
 
 	if (!req)
 		return ERR_PTR(-ENOMEM);

fs/ceph/mds_client.c ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode) · line 2093

Quoted 9 lines

fs/ceph/mds_client.c +1 -2

@@ -2088,8 +2087,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;

Thanks Deepa. We'll plan to take this one in via the ceph tree.

Cheers,

Signature · 1 line

Jeff Layton <[email protected]>

Deepa Dinamani

author

2019-12-03 19:41 UTC #5 lore raw

Re: [PATCH v2 3/6] fs: ceph: Delete timespec64_trunc() usage

Quoted 1 line

Thanks Deepa. We'll plan to take this one in via the ceph tree.

Actually, deletion of the timespec64_trunc() will depend on this patch. Can we merge the series through a common tree? Otherwise, whoever takes the [PATCH 6/7] ("fs:
Delete timespec64_trunc()") would have to depend on your tree. If you are ok with the change, can you ack it?

Thanks,
Deepa

Jeff Layton

Acked-by

2019-12-03 19:49 UTC #6 lore raw

Re: [PATCH v2 3/6] fs: ceph: Delete timespec64_trunc() usage

Quoted 10 lines · Deepa Dinamani

Quoted 1 line
Thanks Deepa. We'll plan to take this one in via the ceph tree.
Actually, deletion of the timespec64_trunc() will depend on this patch. Can we merge the series through a common tree? Otherwise, whoever takes the [PATCH 6/7] ("fs:
Delete timespec64_trunc()") would have to depend on your tree. If you are ok with the change, can you ack it?
Thanks,
Deepa

Sure, no problem if that works better for you.

Acked-by Jeff Layton <[email protected]>

Deepa Dinamani

author

2019-12-06 02:43 UTC #10 lore raw

In reply to Deepa Dinamani

Quoted 4 lines · Deepa Dinamani

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.

Al/Andrew, can one of you help merge these patches?

Thanks,
-Deepa

Al Viro

maintainer

2019-12-07 06:02 UTC #11 lore raw

Quoted 7 lines · Deepa Dinamani

Quoted 4 lines · Deepa Dinamani
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.
Al/Andrew, can one of you help merge these patches?

Looks sane. Could you check if #misc.timestamp looks sane to you?

One thing that leaves me scratching head is kernfs - surely we are _not_ limited by any external layouts there, so why do we need to bother with truncation?

Deepa Dinamani

author

2019-12-08 02:04 UTC #12 lore raw

Quoted 11 lines · Al Viro

Quoted 7 lines · Deepa Dinamani
Quoted 4 lines · Deepa Dinamani
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.
Al/Andrew, can one of you help merge these patches?
Looks sane. Could you check if #misc.timestamp looks sane to you?

Yes, that looks sane to me.

Quoted 3 lines

One thing that leaves me scratching head is kernfs - surely we are _not_ limited by any external layouts there, so why do we need to bother with truncation?

I think I was more pedantic then, and was explicitly truncating times before assignment to inode timestamps. But, Arnd has since coached me that we should not introduce things to safe guard against all possibilities, but only what is needed currently. So this kernfs truncate is redundant, given the limits and the granularity match vfs timestamp representation limits.

-Deepa

Al Viro

maintainer

2019-12-08 03:04 UTC #13 lore raw

Quoted 19 lines · Deepa Dinamani

Quoted 11 lines · Al Viro
Quoted 7 lines · Deepa Dinamani
Quoted 4 lines · Deepa Dinamani
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.
Al/Andrew, can one of you help merge these patches?
Looks sane. Could you check if #misc.timestamp looks sane to you?
Yes, that looks sane to me.
Quoted 3 lines
One thing that leaves me scratching head is kernfs - surely we are _not_ limited by any external layouts there, so why do we need to bother with truncation?

Quoted 6 lines

I think I was more pedantic then, and was explicitly truncating times before assignment to inode timestamps. But, Arnd has since coached me that we should not introduce things to safe guard against all possibilities, but only what is needed currently. So this kernfs truncate is redundant, given the limits and the granularity match vfs timestamp representation limits.

OK... I've tossed a followup removing the truncation from kernfs; the whole series looks reasonably safe, but I don't think it's urgent enough to even try getting it merged before -rc1. So here's what I'm going to do: immediately after -rc1 it gets renamed[*] to #imm.timestamp, which will be in the never-modified mode, in #for-next from the very begining and safe for other trees to pull. Current shortlog:

Al Viro (1):
kernfs: don't bother with timestamp truncation

Amir Goldstein (1):
utimes: Clamp the timestamps in notify_change()

Diffstat:

 fs/attr.c            | 23 +++++++++++------------
 fs/ceph/mds_client.c |  4 +---
 fs/cifs/inode.c      | 13 +++++++------
 fs/configfs/inode.c  |  9 +++------
 fs/f2fs/file.c       | 18 ++++++------------
 fs/fat/misc.c        | 10 +++++++++-
 fs/inode.c           | 33 +++------------------------------
 fs/kernfs/inode.c    |  6 +++---
 fs/ntfs/inode.c      | 18 ++++++------------
 fs/ubifs/file.c      | 18 ++++++------------
 fs/ubifs/sb.c        | 11 ++++-------
 fs/utimes.c          |  4 ++--
 include/linux/fs.h   |  1 -
 13 files changed, 61 insertions(+), 107 deletions(-)

[*] right now it's based on v5.4; I don't see anything that would warrant rebasing it to -rc1 at the moment, but if anything of that sort shows up tomorrow, s/renamed/rebased to -rc1 and renamed/.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Al Viro

maintainer

2019-12-09 00:48 UTC #14 lore raw

Quoted 6 lines · Al Viro

OK... I've tossed a followup removing the truncation from kernfs; the whole series looks reasonably safe, but I don't think it's urgent enough to even try getting it merged before -rc1. So here's what I'm going to do: immediately after -rc1 it gets renamed[*] to #imm.timestamp, which will be in the never-modified mode, in #for-next from the very begining and safe for other trees to pull.

Rebased to -rc1, pushed out as #imm.timestamp, included into #for-next. Never-modified mode...