| Message ID | 20191124193145.22945-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | utimes: Clamp the timestamps in notify_change() | expand |
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 <viro@zeniv.linux.org.uk> 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 <viro@zeniv.linux.org.uk> 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 <amir73il@gmail.com> wrote: > > On Sun, Nov 24, 2019 at 9:49 PM Al Viro <viro@zeniv.linux.org.uk> 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 <amir73il@gmail.com> 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 <mszeredi@redhat.com> Fixes: 42e729b9ddbb ("utimes: Clamp the timestamps before update") Cc: stable@vger.kernel.org # v5.4 Cc: Deepa Dinamani <deepa.kernel@gmail.com> Cc: Jeff Layton <jlayton@kernel.org> Signed-off-by: Amir Goldstein <amir73il@gmail.com> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> 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 <mszeredi@redhat.com> > Fixes: 42e729b9ddbb ("utimes: Clamp the timestamps before update") > Cc: stable@vger.kernel.org # v5.4 > Cc: Deepa Dinamani <deepa.kernel@gmail.com> > Cc: Jeff Layton <jlayton@kernel.org> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > 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/20191111073000.2957-1-amir73il@gmail.com/ > > 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 <bfields@fieldses.org> 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 <bfields@fieldses.org> 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 <viro@zeniv.linux.org.uk> 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/20191130053030.7868-1-deepa.kernel@gmail.com/ 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
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; } /*
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 <mszeredi@redhat.com> Fixes: 42e729b9ddbb ("utimes: Clamp the timestamps before update") Cc: stable@vger.kernel.org # v5.4 Cc: Deepa Dinamani <deepa.kernel@gmail.com> Cc: Jeff Layton <jlayton@kernel.org> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- 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/20191111073000.2957-1-amir73il@gmail.com/ fs/attr.c | 5 +++++ fs/utimes.c | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-)