Push clamping timestamps down the call stack into notify_change(), so
in-kernel callers like nfsd and overlayfs will get similar timestamp
set behavior as utimes.
Suggested-by: Miklos Szeredi <[email protected]>
Fixes: 42e729b9ddbb ("utimes: Clamp the timestamps before update")
Cc: [email protected] # v5.4
Cc: Deepa Dinamani <[email protected]>
Cc: Jeff Layton <[email protected]>
Signed-off-by: Amir Goldstein <[email protected]>
---
Arnd,
This fixes xfstest generic/402 when run with -overlay setup.
Note that running the test requires latest xfstests with:
acb2ba78 - overlay: support timestamp range check
I had previously posted a fix specific for overlayfs [1],
but Miklos suggested this more generic fix, which should also
serve nfsd and other in-kernel users.
I tested this change with test generic/402 on ext4/xfs/btrfs
and overlayfs, but not with nfsd.
Jeff, could you ack this change is good for nfsd as well?
Thanks,
Amir.
[1] https://lore.kernel.org/linux-fsdevel/[email protected]/
fs/attr.c | 5 +++++
fs/utimes.c | 4 ++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/attr.c b/fs/attr.c
index df28035aa23e..e8de5e636e66 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -268,8 +268,13 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
attr->ia_ctime = now;
if (!(ia_valid & ATTR_ATIME_SET))
attr->ia_atime = now;
+ else
+ attr->ia_atime = timestamp_truncate(attr->ia_atime, inode);
if (!(ia_valid & ATTR_MTIME_SET))
attr->ia_mtime = now;
+ else
+ attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
+
if (ia_valid & ATTR_KILL_PRIV) {
error = security_inode_need_killpriv(dentry);
if (error < 0)
diff --git a/fs/utimes.c b/fs/utimes.c
index 1ba3f7883870..090739322463 100644
--- a/fs/utimes.c
+++ b/fs/utimes.c
@@ -36,14 +36,14 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
if (times[0].tv_nsec == UTIME_OMIT)
newattrs.ia_valid &= ~ATTR_ATIME;
else if (times[0].tv_nsec != UTIME_NOW) {
- newattrs.ia_atime = timestamp_truncate(times[0], inode);
+ newattrs.ia_atime = times[0];
newattrs.ia_valid |= ATTR_ATIME_SET;
}
if (times[1].tv_nsec == UTIME_OMIT)
newattrs.ia_valid &= ~ATTR_MTIME;
else if (times[1].tv_nsec != UTIME_NOW) {
- newattrs.ia_mtime = timestamp_truncate(times[1], inode);
+ newattrs.ia_mtime = times[1];
newattrs.ia_valid |= ATTR_MTIME_SET;
}
/*
--
2.17.1
On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> Push clamping timestamps down the call stack into notify_change(), so
> in-kernel callers like nfsd and overlayfs will get similar timestamp
> set behavior as utimes.
Makes sense; said that, shouldn't we go through ->setattr() instances and
get rid of that there, now that notify_change() is made to do it?
I mean,
if (ia_valid & ATTR_ATIME)
sd_iattr->ia_atime = timestamp_truncate(iattr->ia_atime,
inode);
in configfs_setattr() looks like it should be reverted to
if (ia_valid & ATTR_ATIME)
sd_iattr->ia_atime = iattr->ia_atime;
with that, etc.
Moreover, does that leave any valid callers of timestamp_truncate()
outside of notify_change() and current_time()? IOW, is there any
point having it exported? Look:
fs/attr.c:187: inode->i_atime = timestamp_truncate(attr->ia_atime,
fs/attr.c:191: inode->i_mtime = timestamp_truncate(attr->ia_mtime,
fs/attr.c:195: inode->i_ctime = timestamp_truncate(attr->ia_ctime,
setattr_copy(), called downstream of your changes.
fs/configfs/inode.c:79: sd_iattr->ia_atime = timestamp_truncate(iattr->ia_atime,
fs/configfs/inode.c:82: sd_iattr->ia_mtime = timestamp_truncate(iattr->ia_mtime,
fs/configfs/inode.c:85: sd_iattr->ia_ctime = timestamp_truncate(iattr->ia_ctime,
configfs_setattr(); ditto.
fs/f2fs/file.c:755: inode->i_atime = timestamp_truncate(attr->ia_atime,
fs/f2fs/file.c:759: inode->i_mtime = timestamp_truncate(attr->ia_mtime,
fs/f2fs/file.c:763: inode->i_ctime = timestamp_truncate(attr->ia_ctime,
__setattr_copy() from f2fs_setattr(); ditto.
fs/inode.c:2224: return timestamp_truncate(now, inode);
current_time()
fs/kernfs/inode.c:163: inode->i_atime = timestamp_truncate(attrs->ia_atime, inode);
fs/kernfs/inode.c:164: inode->i_mtime = timestamp_truncate(attrs->ia_mtime, inode);
fs/kernfs/inode.c:165: inode->i_ctime = timestamp_truncate(attrs->ia_ctime, inode);
->s_time_max and ->s_time_min are left TIME64_MAX and TIME64_MIN resp., so
timestamp_truncate() should be a no-op there.
fs/ntfs/inode.c:2903: vi->i_atime = timestamp_truncate(attr->ia_atime,
fs/ntfs/inode.c:2907: vi->i_mtime = timestamp_truncate(attr->ia_mtime,
fs/ntfs/inode.c:2911: vi->i_ctime = timestamp_truncate(attr->ia_ctime,
ntfs_setattr(); downstream from your changes
fs/ubifs/file.c:1082: inode->i_atime = timestamp_truncate(attr->ia_atime,
fs/ubifs/file.c:1086: inode->i_mtime = timestamp_truncate(attr->ia_mtime,
fs/ubifs/file.c:1090: inode->i_ctime = timestamp_truncate(attr->ia_ctime,
do_attr_changes(), from do_truncation() or do_setattr(), both from ubifs_setattr();
ditto.
fs/utimes.c:39: newattrs.ia_atime = timestamp_truncate(times[0], inode);
fs/utimes.c:46: newattrs.ia_mtime = timestamp_truncate(times[1], inode);
disappears in your patch.
On Sun, Nov 24, 2019 at 9:49 PM Al Viro <[email protected]> wrote:
>
> On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> > Push clamping timestamps down the call stack into notify_change(), so
> > in-kernel callers like nfsd and overlayfs will get similar timestamp
> > set behavior as utimes.
>
> Makes sense; said that, shouldn't we go through ->setattr() instances and
> get rid of that there, now that notify_change() is made to do it?
>
Sounds reasonable. But I'd rather leave this cleanup to Deepa,
who did all this work.
Thanks,
Amir.
On Sun, Nov 24, 2019 at 11:49 AM Al Viro <[email protected]> wrote:
>
> On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> > Push clamping timestamps down the call stack into notify_change(), so
> > in-kernel callers like nfsd and overlayfs will get similar timestamp
> > set behavior as utimes.
>
> Makes sense; said that, shouldn't we go through ->setattr() instances and
> get rid of that there, now that notify_change() is made to do it?
>
> I mean,
> if (ia_valid & ATTR_ATIME)
> sd_iattr->ia_atime = timestamp_truncate(iattr->ia_atime,
> inode);
> in configfs_setattr() looks like it should be reverted to
> if (ia_valid & ATTR_ATIME)
> sd_iattr->ia_atime = iattr->ia_atime;
> with that, etc.
>
> Moreover, does that leave any valid callers of timestamp_truncate()
> outside of notify_change() and current_time()? IOW, is there any
> point having it exported? Look:
> fs/attr.c:187: inode->i_atime = timestamp_truncate(attr->ia_atime,
> fs/attr.c:191: inode->i_mtime = timestamp_truncate(attr->ia_mtime,
> fs/attr.c:195: inode->i_ctime = timestamp_truncate(attr->ia_ctime,
> setattr_copy(), called downstream of your changes.
> fs/configfs/inode.c:79: sd_iattr->ia_atime = timestamp_truncate(iattr->ia_atime,
> fs/configfs/inode.c:82: sd_iattr->ia_mtime = timestamp_truncate(iattr->ia_mtime,
> fs/configfs/inode.c:85: sd_iattr->ia_ctime = timestamp_truncate(iattr->ia_ctime,
> configfs_setattr(); ditto.
> fs/f2fs/file.c:755: inode->i_atime = timestamp_truncate(attr->ia_atime,
> fs/f2fs/file.c:759: inode->i_mtime = timestamp_truncate(attr->ia_mtime,
> fs/f2fs/file.c:763: inode->i_ctime = timestamp_truncate(attr->ia_ctime,
> __setattr_copy() from f2fs_setattr(); ditto.
> fs/inode.c:2224: return timestamp_truncate(now, inode);
> current_time()
> fs/kernfs/inode.c:163: inode->i_atime = timestamp_truncate(attrs->ia_atime, inode);
> fs/kernfs/inode.c:164: inode->i_mtime = timestamp_truncate(attrs->ia_mtime, inode);
> fs/kernfs/inode.c:165: inode->i_ctime = timestamp_truncate(attrs->ia_ctime, inode);
> ->s_time_max and ->s_time_min are left TIME64_MAX and TIME64_MIN resp., so
> timestamp_truncate() should be a no-op there.
> fs/ntfs/inode.c:2903: vi->i_atime = timestamp_truncate(attr->ia_atime,
> fs/ntfs/inode.c:2907: vi->i_mtime = timestamp_truncate(attr->ia_mtime,
> fs/ntfs/inode.c:2911: vi->i_ctime = timestamp_truncate(attr->ia_ctime,
> ntfs_setattr(); downstream from your changes
> fs/ubifs/file.c:1082: inode->i_atime = timestamp_truncate(attr->ia_atime,
> fs/ubifs/file.c:1086: inode->i_mtime = timestamp_truncate(attr->ia_mtime,
> fs/ubifs/file.c:1090: inode->i_ctime = timestamp_truncate(attr->ia_ctime,
> do_attr_changes(), from do_truncation() or do_setattr(), both from ubifs_setattr();
> ditto.
> fs/utimes.c:39: newattrs.ia_atime = timestamp_truncate(times[0], inode);
> fs/utimes.c:46: newattrs.ia_mtime = timestamp_truncate(times[1], inode);
> disappears in your patch.
We also want to replace all uses of timespec64_trunc() with
timestamp_truncate() for all fs cases.
In that case we have a few more:
fs/ceph/mds_client.c: req->r_stamp = timespec64_trunc(ts,
mdsc->fsc->sb->s_time_gran);
fs/cifs/inode.c: fattr->cf_mtime =
timespec64_trunc(fattr->cf_mtime, sb->s_time_gran);
fs/cifs/inode.c: fattr->cf_atime =
timespec64_trunc(fattr->cf_atime, sb->s_time_gran);
fs/fat/misc.c:static inline struct timespec64
fat_timespec64_trunc_2secs(struct timespec64 ts)
fs/fat/misc.c: inode->i_ctime =
timespec64_trunc(*now, 10000000);
fs/fat/misc.c: inode->i_ctime =
fat_timespec64_trunc_2secs(*now);
fs/fat/misc.c: inode->i_mtime = fat_timespec64_trunc_2secs(*now);
fs/ubifs/sb.c: ts = timespec64_trunc(ts, DEFAULT_TIME_GRAN);
These do not follow from notify_change(), so these might still need
timestamp_truncate() exported.
I will post a cleanup series for timespec64_trunc() also, then we can decide.
-Deepa
On Sun, Nov 24, 2019 at 12:50 PM Amir Goldstein <[email protected]> wrote:
>
> On Sun, Nov 24, 2019 at 9:49 PM Al Viro <[email protected]> wrote:
> >
> > On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> > > Push clamping timestamps down the call stack into notify_change(), so
> > > in-kernel callers like nfsd and overlayfs will get similar timestamp
> > > set behavior as utimes.
> >
> > Makes sense; said that, shouldn't we go through ->setattr() instances and
> > get rid of that there, now that notify_change() is made to do it?
> >
>
> Sounds reasonable. But I'd rather leave this cleanup to Deepa,
> who did all this work.
Thanks, I can post a patch for this.
-Deepa
On Sun, Nov 24, 2019 at 01:13:50PM -0800, Deepa Dinamani wrote:
> We also want to replace all uses of timespec64_trunc() with
> timestamp_truncate() for all fs cases.
>
> In that case we have a few more:
>
> fs/ceph/mds_client.c: req->r_stamp = timespec64_trunc(ts,
> mdsc->fsc->sb->s_time_gran);
Umm... That comes from ktime_get_coarse_real_ts64(&ts);
> fs/cifs/inode.c: fattr->cf_mtime =
> timespec64_trunc(fattr->cf_mtime, sb->s_time_gran);
ktime_get_real_ts64(&fattr->cf_mtime) here
> fs/cifs/inode.c: fattr->cf_atime =
> timespec64_trunc(fattr->cf_atime, sb->s_time_gran);
ditto
> fs/fat/misc.c: inode->i_ctime =
> timespec64_trunc(*now, 10000000);
I wonder... some are from setattr, some (with NULL passed to fat_truncate_time())
from current_time()... Wouldn't it make more sense to move the truncation into
the few callers that really need it (if any)? Quite a few of those are *also*
getting the value from current_time(), after all. fat_fill_inode() looks like
the only case that doesn't fall into these classes; does it need truncation?
BTW, could we *please* do something about fs/inode.c:update_time()? I mean,
sure, local variable shadows file-scope function, so it's legitimate C, but
this is not IOCCC and having a function called 'update_time' end with
return update_time(inode, time, flags);
is actively hostile towards casual readers...
> fs/fat/misc.c: inode->i_ctime =
> fat_timespec64_trunc_2secs(*now);
> fs/fat/misc.c: inode->i_mtime = fat_timespec64_trunc_2secs(*now);
> fs/ubifs/sb.c: ts = timespec64_trunc(ts, DEFAULT_TIME_GRAN);
>
> These do not follow from notify_change(), so these might still need
> timestamp_truncate() exported.
> I will post a cleanup series for timespec64_trunc() also, then we can decide.
What I've got right now is
commit 6d13412e2b27970810037f7b1b418febcd7013aa
Author: Amir Goldstein <[email protected]>
Date: Sun Nov 24 21:31:45 2019 +0200
utimes: Clamp the timestamps in notify_change()
Push clamping timestamps into notify_change(), so in-kernel
callers like nfsd and overlayfs will get similar timestamp
set behavior as utimes.
AV: get rid of clamping in ->setattr() instances; we don't need
to bother with that there, with notify_change() doing normalization
in all cases now (it already did for implicit case, since current_time()
clamps).
Suggested-by: Miklos Szeredi <[email protected]>
Fixes: 42e729b9ddbb ("utimes: Clamp the timestamps before update")
Cc: [email protected] # v5.4
Cc: Deepa Dinamani <[email protected]>
Cc: Jeff Layton <[email protected]>
Signed-off-by: Amir Goldstein <[email protected]>
Signed-off-by: Al Viro <[email protected]>
diff --git a/fs/attr.c b/fs/attr.c
index df28035aa23e..b4bbdbd4c8ca 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -183,18 +183,12 @@ void setattr_copy(struct inode *inode, const struct iattr *attr)
inode->i_uid = attr->ia_uid;
if (ia_valid & ATTR_GID)
inode->i_gid = attr->ia_gid;
- if (ia_valid & ATTR_ATIME) {
- inode->i_atime = timestamp_truncate(attr->ia_atime,
- inode);
- }
- if (ia_valid & ATTR_MTIME) {
- inode->i_mtime = timestamp_truncate(attr->ia_mtime,
- inode);
- }
- if (ia_valid & ATTR_CTIME) {
- inode->i_ctime = timestamp_truncate(attr->ia_ctime,
- inode);
- }
+ if (ia_valid & ATTR_ATIME)
+ inode->i_atime = attr->ia_atime;
+ if (ia_valid & ATTR_MTIME)
+ inode->i_mtime = attr->ia_mtime;
+ if (ia_valid & ATTR_CTIME)
+ inode->i_ctime = attr->ia_ctime;
if (ia_valid & ATTR_MODE) {
umode_t mode = attr->ia_mode;
@@ -268,8 +262,13 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
attr->ia_ctime = now;
if (!(ia_valid & ATTR_ATIME_SET))
attr->ia_atime = now;
+ else
+ attr->ia_atime = timestamp_truncate(attr->ia_atime, inode);
if (!(ia_valid & ATTR_MTIME_SET))
attr->ia_mtime = now;
+ else
+ attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
+
if (ia_valid & ATTR_KILL_PRIV) {
error = security_inode_need_killpriv(dentry);
if (error < 0)
diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index 680aba9c00d5..fd0b5dd68f9e 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -76,14 +76,11 @@ int configfs_setattr(struct dentry * dentry, struct iattr * iattr)
if (ia_valid & ATTR_GID)
sd_iattr->ia_gid = iattr->ia_gid;
if (ia_valid & ATTR_ATIME)
- sd_iattr->ia_atime = timestamp_truncate(iattr->ia_atime,
- inode);
+ sd_iattr->ia_atime = iattr->ia_atime;
if (ia_valid & ATTR_MTIME)
- sd_iattr->ia_mtime = timestamp_truncate(iattr->ia_mtime,
- inode);
+ sd_iattr->ia_mtime = iattr->ia_mtime;
if (ia_valid & ATTR_CTIME)
- sd_iattr->ia_ctime = timestamp_truncate(iattr->ia_ctime,
- inode);
+ sd_iattr->ia_ctime = iattr->ia_ctime;
if (ia_valid & ATTR_MODE) {
umode_t mode = iattr->ia_mode;
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 29bc0a542759..a286564ba2e1 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -751,18 +751,12 @@ static void __setattr_copy(struct inode *inode, const struct iattr *attr)
inode->i_uid = attr->ia_uid;
if (ia_valid & ATTR_GID)
inode->i_gid = attr->ia_gid;
- if (ia_valid & ATTR_ATIME) {
- inode->i_atime = timestamp_truncate(attr->ia_atime,
- inode);
- }
- if (ia_valid & ATTR_MTIME) {
- inode->i_mtime = timestamp_truncate(attr->ia_mtime,
- inode);
- }
- if (ia_valid & ATTR_CTIME) {
- inode->i_ctime = timestamp_truncate(attr->ia_ctime,
- inode);
- }
+ if (ia_valid & ATTR_ATIME)
+ inode->i_atime = attr->ia_atime;
+ if (ia_valid & ATTR_MTIME)
+ inode->i_mtime = attr->ia_mtime;
+ if (ia_valid & ATTR_CTIME)
+ inode->i_ctime = attr->ia_ctime;
if (ia_valid & ATTR_MODE) {
umode_t mode = attr->ia_mode;
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 6c7388430ad3..d4359a1df3d5 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -2899,18 +2899,12 @@ int ntfs_setattr(struct dentry *dentry, struct iattr *attr)
ia_valid |= ATTR_MTIME | ATTR_CTIME;
}
}
- if (ia_valid & ATTR_ATIME) {
- vi->i_atime = timestamp_truncate(attr->ia_atime,
- vi);
- }
- if (ia_valid & ATTR_MTIME) {
- vi->i_mtime = timestamp_truncate(attr->ia_mtime,
- vi);
- }
- if (ia_valid & ATTR_CTIME) {
- vi->i_ctime = timestamp_truncate(attr->ia_ctime,
- vi);
- }
+ if (ia_valid & ATTR_ATIME)
+ vi->i_atime = attr->ia_atime;
+ if (ia_valid & ATTR_MTIME)
+ vi->i_mtime = attr->ia_mtime;
+ if (ia_valid & ATTR_CTIME)
+ vi->i_ctime = attr->ia_ctime;
mark_inode_dirty(vi);
out:
return err;
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index cd52585c8f4f..91362079f82a 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1078,18 +1078,12 @@ static void do_attr_changes(struct inode *inode, const struct iattr *attr)
inode->i_uid = attr->ia_uid;
if (attr->ia_valid & ATTR_GID)
inode->i_gid = attr->ia_gid;
- if (attr->ia_valid & ATTR_ATIME) {
- inode->i_atime = timestamp_truncate(attr->ia_atime,
- inode);
- }
- if (attr->ia_valid & ATTR_MTIME) {
- inode->i_mtime = timestamp_truncate(attr->ia_mtime,
- inode);
- }
- if (attr->ia_valid & ATTR_CTIME) {
- inode->i_ctime = timestamp_truncate(attr->ia_ctime,
- inode);
- }
+ if (attr->ia_valid & ATTR_ATIME)
+ inode->i_atime = attr->ia_atime;
+ if (attr->ia_valid & ATTR_MTIME)
+ inode->i_mtime = attr->ia_mtime;
+ if (attr->ia_valid & ATTR_CTIME)
+ inode->i_ctime = attr->ia_ctime;
if (attr->ia_valid & ATTR_MODE) {
umode_t mode = attr->ia_mode;
diff --git a/fs/utimes.c b/fs/utimes.c
index 1ba3f7883870..090739322463 100644
--- a/fs/utimes.c
+++ b/fs/utimes.c
@@ -36,14 +36,14 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
if (times[0].tv_nsec == UTIME_OMIT)
newattrs.ia_valid &= ~ATTR_ATIME;
else if (times[0].tv_nsec != UTIME_NOW) {
- newattrs.ia_atime = timestamp_truncate(times[0], inode);
+ newattrs.ia_atime = times[0];
newattrs.ia_valid |= ATTR_ATIME_SET;
}
if (times[1].tv_nsec == UTIME_OMIT)
newattrs.ia_valid &= ~ATTR_MTIME;
else if (times[1].tv_nsec != UTIME_NOW) {
- newattrs.ia_mtime = timestamp_truncate(times[1], inode);
+ newattrs.ia_mtime = times[1];
newattrs.ia_valid |= ATTR_MTIME_SET;
}
/*
On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> Push clamping timestamps down the call stack into notify_change(), so
> in-kernel callers like nfsd and overlayfs will get similar timestamp
> set behavior as utimes.
So, nfsd has always bypassed timestamp_truncate() and we've never
noticed till now? What are the symptoms? (Do timestamps go backwards
after cache eviction on filesystems with large time granularity?)
Looks like generic/402 has never run in my tests:
generic/402 [not run] no kernel support for y2038 sysfs switch
--b.
>
> Suggested-by: Miklos Szeredi <[email protected]>
> Fixes: 42e729b9ddbb ("utimes: Clamp the timestamps before update")
> Cc: [email protected] # v5.4
> Cc: Deepa Dinamani <[email protected]>
> Cc: Jeff Layton <[email protected]>
> Signed-off-by: Amir Goldstein <[email protected]>
> ---
>
> Arnd,
>
> This fixes xfstest generic/402 when run with -overlay setup.
> Note that running the test requires latest xfstests with:
> acb2ba78 - overlay: support timestamp range check
>
> I had previously posted a fix specific for overlayfs [1],
> but Miklos suggested this more generic fix, which should also
> serve nfsd and other in-kernel users.
>
> I tested this change with test generic/402 on ext4/xfs/btrfs
> and overlayfs, but not with nfsd.
>
> Jeff, could you ack this change is good for nfsd as well?
>
> Thanks,
> Amir.
>
> [1] https://lore.kernel.org/linux-fsdevel/[email protected]/
>
> fs/attr.c | 5 +++++
> fs/utimes.c | 4 ++--
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index df28035aa23e..e8de5e636e66 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -268,8 +268,13 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
> attr->ia_ctime = now;
> if (!(ia_valid & ATTR_ATIME_SET))
> attr->ia_atime = now;
> + else
> + attr->ia_atime = timestamp_truncate(attr->ia_atime, inode);
> if (!(ia_valid & ATTR_MTIME_SET))
> attr->ia_mtime = now;
> + else
> + attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
> +
> if (ia_valid & ATTR_KILL_PRIV) {
> error = security_inode_need_killpriv(dentry);
> if (error < 0)
> diff --git a/fs/utimes.c b/fs/utimes.c
> index 1ba3f7883870..090739322463 100644
> --- a/fs/utimes.c
> +++ b/fs/utimes.c
> @@ -36,14 +36,14 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
> if (times[0].tv_nsec == UTIME_OMIT)
> newattrs.ia_valid &= ~ATTR_ATIME;
> else if (times[0].tv_nsec != UTIME_NOW) {
> - newattrs.ia_atime = timestamp_truncate(times[0], inode);
> + newattrs.ia_atime = times[0];
> newattrs.ia_valid |= ATTR_ATIME_SET;
> }
>
> if (times[1].tv_nsec == UTIME_OMIT)
> newattrs.ia_valid &= ~ATTR_MTIME;
> else if (times[1].tv_nsec != UTIME_NOW) {
> - newattrs.ia_mtime = timestamp_truncate(times[1], inode);
> + newattrs.ia_mtime = times[1];
> newattrs.ia_valid |= ATTR_MTIME_SET;
> }
> /*
> --
> 2.17.1
On Mon, Nov 25, 2019 at 6:46 PM J . Bruce Fields <[email protected]> wrote:
>
> On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> > Push clamping timestamps down the call stack into notify_change(), so
> > in-kernel callers like nfsd and overlayfs will get similar timestamp
> > set behavior as utimes.
>
> So, nfsd has always bypassed timestamp_truncate() and we've never
> noticed till now? What are the symptoms? (Do timestamps go backwards
> after cache eviction on filesystems with large time granularity?)
Clamping seems to be new behavior since v5.4-rc1.
Before that clamping was done implicitly when hitting the disk IIUC,
so it was observed mostly after cache eviction.
>
> Looks like generic/402 has never run in my tests:
>
> generic/402 [not run] no kernel support for y2038 sysfs switch
>
The test in its current form is quite recent as well or at the _require
has changed recently.
See acb2ba78 - overlay: support timestamp range check
You'd probably need something similar for nfs (?)
Thanks,
Amir.
On Mon, Nov 25, 2019 at 8:46 AM J . Bruce Fields <[email protected]> wrote:
>
> On Sun, Nov 24, 2019 at 09:31:45PM +0200, Amir Goldstein wrote:
> > Push clamping timestamps down the call stack into notify_change(), so
> > in-kernel callers like nfsd and overlayfs will get similar timestamp
> > set behavior as utimes.
>
> So, nfsd has always bypassed timestamp_truncate() and we've never
> noticed till now? What are the symptoms? (Do timestamps go backwards
> after cache eviction on filesystems with large time granularity?)
>
> Looks like generic/402 has never run in my tests:
>
> generic/402 [not run] no kernel support for y2038 sysfs switch
You need this in your xfstest:
https://patchwork.kernel.org/patch/11049745/ . The test has been
updated recently.
And, you need a change like for overlayfs as Amir pointed out.
-Deepa
On Sun, Nov 24, 2019 at 1:34 PM Al Viro <[email protected]> wrote:
>
> On Sun, Nov 24, 2019 at 01:13:50PM -0800, Deepa Dinamani wrote:
>
> > We also want to replace all uses of timespec64_trunc() with
> > timestamp_truncate() for all fs cases.
> >
> > In that case we have a few more:
> >
> > fs/ceph/mds_client.c: req->r_stamp = timespec64_trunc(ts,
> > mdsc->fsc->sb->s_time_gran);
>
> Umm... That comes from ktime_get_coarse_real_ts64(&ts);
>
> > fs/cifs/inode.c: fattr->cf_mtime =
> > timespec64_trunc(fattr->cf_mtime, sb->s_time_gran);
> ktime_get_real_ts64(&fattr->cf_mtime) here
>
> > fs/cifs/inode.c: fattr->cf_atime =
> > timespec64_trunc(fattr->cf_atime, sb->s_time_gran);
> ditto
>
> > fs/fat/misc.c: inode->i_ctime =
> > timespec64_trunc(*now, 10000000);
>
> I wonder... some are from setattr, some (with NULL passed to fat_truncate_time())
> from current_time()... Wouldn't it make more sense to move the truncation into
> the few callers that really need it (if any)? Quite a few of those are *also*
> getting the value from current_time(), after all. fat_fill_inode() looks like
> the only case that doesn't fall into these classes; does it need truncation?
I've posted a series at
https://lore.kernel.org/lkml/[email protected]/
I was able to get rid of all instances but it seemed like it would be
easier for cifs to use timestamp_truncate() directly.
If you really don't want it exported, I could find some other way of doing it.
> BTW, could we *please* do something about fs/inode.c:update_time()? I mean,
> sure, local variable shadows file-scope function, so it's legitimate C, but
> this is not IOCCC and having a function called 'update_time' end with
> return update_time(inode, time, flags);
> is actively hostile towards casual readers...
Added this to the series as well.
-Deepa