* [PATCH 1/7] fs: fat: Eliminate timespec64_trunc() usage
2019-11-30 5:30 [PATCH 0/7] Delete timespec64_trunc() Deepa Dinamani
@ 2019-11-30 5:30 ` Deepa Dinamani
2019-11-30 5:30 ` [PATCH 2/7] fs: cifs: Fix atime update check vs mtime Deepa Dinamani
` (5 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Deepa Dinamani @ 2019-11-30 5:30 UTC (permalink / raw)
To: viro, linux-kernel; +Cc: linux-fsdevel, arnd, hirofumi
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 <viro@zeniv.linux.org.uk>
Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
Cc: hirofumi@mail.parknet.co.jp
---
fs/fat/misc.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index 1e08bd54c5fb..f1b2a1fc2a6a 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -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);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/7] fs: cifs: Fix atime update check vs mtime
2019-11-30 5:30 [PATCH 0/7] Delete timespec64_trunc() Deepa Dinamani
2019-11-30 5:30 ` [PATCH 1/7] fs: fat: Eliminate timespec64_trunc() usage Deepa Dinamani
@ 2019-11-30 5:30 ` Deepa Dinamani
2019-12-01 18:23 ` Steve French
2019-11-30 5:30 ` [PATCH 3/7] fs: cifs: Delete usage of timespec64_trunc Deepa Dinamani
` (4 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Deepa Dinamani @ 2019-11-30 5:30 UTC (permalink / raw)
To: viro, linux-kernel; +Cc: linux-fsdevel, arnd, stfrench, linux-cifs
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 <deepa.kernel@gmail.com>
Cc: stfrench@microsoft.com
Cc: linux-cifs@vger.kernel.org
---
fs/cifs/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 8a76195e8a69..ca76a9287456 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/7] fs: cifs: Fix atime update check vs mtime
2019-11-30 5:30 ` [PATCH 2/7] fs: cifs: Fix atime update check vs mtime Deepa Dinamani
@ 2019-12-01 18:23 ` Steve French
0 siblings, 0 replies; 14+ messages in thread
From: Steve French @ 2019-12-01 18:23 UTC (permalink / raw)
To: Deepa Dinamani
Cc: Al Viro, LKML, linux-fsdevel, Arnd Bergmann, Steve French, CIFS
merged into cifs-2.6.git for-next
On Fri, Nov 29, 2019 at 11:34 PM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
>
> 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 <deepa.kernel@gmail.com>
> Cc: stfrench@microsoft.com
> Cc: linux-cifs@vger.kernel.org
> ---
> fs/cifs/inode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 8a76195e8a69..ca76a9287456 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -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
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/7] fs: cifs: Delete usage of timespec64_trunc
2019-11-30 5:30 [PATCH 0/7] Delete timespec64_trunc() Deepa Dinamani
2019-11-30 5:30 ` [PATCH 1/7] fs: fat: Eliminate timespec64_trunc() usage Deepa Dinamani
2019-11-30 5:30 ` [PATCH 2/7] fs: cifs: Fix atime update check vs mtime Deepa Dinamani
@ 2019-11-30 5:30 ` Deepa Dinamani
2019-11-30 21:25 ` Steve French
2019-11-30 5:30 ` [PATCH 4/7] fs: ceph: Delete timespec64_trunc() usage Deepa Dinamani
` (3 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Deepa Dinamani @ 2019-11-30 5:30 UTC (permalink / raw)
To: viro, linux-kernel; +Cc: linux-fsdevel, arnd, stfrench, linux-cifs
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.
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 <deepa.kernel@gmail.com>
Cc: stfrench@microsoft.com
Cc: linux-cifs@vger.kernel.org
---
fs/cifs/inode.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index ca76a9287456..026ed49e8aa4 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/7] fs: cifs: Delete usage of timespec64_trunc
2019-11-30 5:30 ` [PATCH 3/7] fs: cifs: Delete usage of timespec64_trunc Deepa Dinamani
@ 2019-11-30 21:25 ` Steve French
2019-12-01 5:35 ` Deepa Dinamani
0 siblings, 1 reply; 14+ messages in thread
From: Steve French @ 2019-11-30 21:25 UTC (permalink / raw)
To: Deepa Dinamani
Cc: Al Viro, LKML, linux-fsdevel, Arnd Bergmann, Steve French, CIFS
Is this intended to merge separately or do you want it merged through
the cifs git tree?
On Fri, Nov 29, 2019 at 11:33 PM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
>
> 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.
>
> 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 <deepa.kernel@gmail.com>
> Cc: stfrench@microsoft.com
> Cc: linux-cifs@vger.kernel.org
> ---
> fs/cifs/inode.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index ca76a9287456..026ed49e8aa4 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -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
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/7] fs: cifs: Delete usage of timespec64_trunc
2019-11-30 21:25 ` Steve French
@ 2019-12-01 5:35 ` Deepa Dinamani
0 siblings, 0 replies; 14+ messages in thread
From: Deepa Dinamani @ 2019-12-01 5:35 UTC (permalink / raw)
To: Steve French
Cc: Al Viro, LKML, linux-fsdevel, Arnd Bergmann, Steve French, CIFS
On Sat, Nov 30, 2019 at 1:25 PM Steve French <smfrench@gmail.com> wrote:
>
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/7] fs: ceph: Delete timespec64_trunc() usage
2019-11-30 5:30 [PATCH 0/7] Delete timespec64_trunc() Deepa Dinamani
` (2 preceding siblings ...)
2019-11-30 5:30 ` [PATCH 3/7] fs: cifs: Delete usage of timespec64_trunc Deepa Dinamani
@ 2019-11-30 5:30 ` Deepa Dinamani
2019-11-30 5:30 ` [PATCH 5/7] fs: ubifs: Eliminate " Deepa Dinamani
` (2 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Deepa Dinamani @ 2019-11-30 5:30 UTC (permalink / raw)
To: viro, linux-kernel; +Cc: linux-fsdevel, arnd, jlayton, ceph-devel
Since ceph always uses ns granularity, skip the
truncation which is a no-op.
Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
Cc: jlayton@kernel.org
Cc: ceph-devel@vger.kernel.org
---
fs/ceph/mds_client.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 068b029cf073..c2aa290f6c3e 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -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;
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/7] fs: ubifs: Eliminate timespec64_trunc() usage
2019-11-30 5:30 [PATCH 0/7] Delete timespec64_trunc() Deepa Dinamani
` (3 preceding siblings ...)
2019-11-30 5:30 ` [PATCH 4/7] fs: ceph: Delete timespec64_trunc() usage Deepa Dinamani
@ 2019-11-30 5:30 ` Deepa Dinamani
2019-11-30 5:30 ` [PATCH 6/7] fs: Delete timespec64_trunc() Deepa Dinamani
2019-11-30 5:30 ` [PATCH 7/7] fs: Do not overload update_time Deepa Dinamani
6 siblings, 0 replies; 14+ messages in thread
From: Deepa Dinamani @ 2019-11-30 5:30 UTC (permalink / raw)
To: viro, linux-kernel; +Cc: linux-fsdevel, arnd, richard, linux-mtd
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 <deepa.kernel@gmail.com>
Cc: richard@nod.at
Cc: linux-mtd@lists.infradead.org
---
fs/ubifs/sb.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index 2b7c04bf8983..93d550be4c11 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -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);
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 6/7] fs: Delete timespec64_trunc()
2019-11-30 5:30 [PATCH 0/7] Delete timespec64_trunc() Deepa Dinamani
` (4 preceding siblings ...)
2019-11-30 5:30 ` [PATCH 5/7] fs: ubifs: Eliminate " Deepa Dinamani
@ 2019-11-30 5:30 ` Deepa Dinamani
2019-11-30 5:30 ` [PATCH 7/7] fs: Do not overload update_time Deepa Dinamani
6 siblings, 0 replies; 14+ messages in thread
From: Deepa Dinamani @ 2019-11-30 5:30 UTC (permalink / raw)
To: viro, linux-kernel; +Cc: linux-fsdevel, arnd
There are no more callers to the function remaining.
Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
---
fs/inode.c | 24 ------------------------
include/linux/fs.h | 1 -
2 files changed, 25 deletions(-)
diff --git a/fs/inode.c b/fs/inode.c
index fef457a42882..12c9e38529c9 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -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
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98e0349adb52..46dd7e6f6d73 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -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);
/*
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 7/7] fs: Do not overload update_time
2019-11-30 5:30 [PATCH 0/7] Delete timespec64_trunc() Deepa Dinamani
` (5 preceding siblings ...)
2019-11-30 5:30 ` [PATCH 6/7] fs: Delete timespec64_trunc() Deepa Dinamani
@ 2019-11-30 5:30 ` Deepa Dinamani
2019-12-01 20:51 ` Dave Chinner
2019-12-02 17:48 ` Christoph Hellwig
6 siblings, 2 replies; 14+ messages in thread
From: Deepa Dinamani @ 2019-11-30 5:30 UTC (permalink / raw)
To: viro, linux-kernel; +Cc: linux-fsdevel, arnd
update_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.
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
---
fs/inode.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/inode.c b/fs/inode.c
index 12c9e38529c9..0be58a680457 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -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);
}
/**
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 7/7] fs: Do not overload update_time
2019-11-30 5:30 ` [PATCH 7/7] fs: Do not overload update_time Deepa Dinamani
@ 2019-12-01 20:51 ` Dave Chinner
2019-12-02 17:48 ` Christoph Hellwig
1 sibling, 0 replies; 14+ messages in thread
From: Dave Chinner @ 2019-12-01 20:51 UTC (permalink / raw)
To: Deepa Dinamani; +Cc: viro, linux-kernel, linux-fsdevel, arnd
On Fri, Nov 29, 2019 at 09:30:30PM -0800, Deepa Dinamani wrote:
> update_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.
>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
> ---
> fs/inode.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 12c9e38529c9..0be58a680457 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -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);
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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 7/7] fs: Do not overload update_time
2019-11-30 5:30 ` [PATCH 7/7] fs: Do not overload update_time Deepa Dinamani
2019-12-01 20:51 ` Dave Chinner
@ 2019-12-02 17:48 ` Christoph Hellwig
2019-12-02 23:13 ` Deepa Dinamani
1 sibling, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2019-12-02 17:48 UTC (permalink / raw)
To: Deepa Dinamani; +Cc: viro, linux-kernel, linux-fsdevel, arnd
On Fri, Nov 29, 2019 at 09:30:30PM -0800, Deepa Dinamani wrote:
> - 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);
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 7/7] fs: Do not overload update_time
2019-12-02 17:48 ` Christoph Hellwig
@ 2019-12-02 23:13 ` Deepa Dinamani
0 siblings, 0 replies; 14+ messages in thread
From: Deepa Dinamani @ 2019-12-02 23:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alexander Viro, Linux Kernel Mailing List,
Linux FS-devel Mailing List, Arnd Bergmann
On Mon, Dec 2, 2019 at 9:48 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Nov 29, 2019 at 09:30:30PM -0800, Deepa Dinamani wrote:
> > - 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:
Will update this in v2. Thanks.
-Deepa
^ permalink raw reply [flat|nested] 14+ messages in thread