| Message ID | 20230907-ctime-fixes-v1-1-3b74c970d934@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs: fixes for multigrain ctime code | expand |
On Thu 07-09-23 12:33:47, Jeff Layton wrote: > With the advent of multigrain timestamps, we use inode_set_ctime_current > to set the ctime, which can skip updating if the existing ctime appears > to be in the future. Because we don't initialize this field at > allocation time, that could prevent the ctime from being initialized > properly when the inode is instantiated. > > Always initialize the ctime field to the epoch so that the filesystem > can set the timestamps properly later. > > Reported-by: kernel test robot <oliver.sang@intel.com> > Closes: https://lore.kernel.org/oe-lkp/202309071017.a64aca5e-oliver.sang@intel.com > Signed-off-by: Jeff Layton <jlayton@kernel.org> Looks good but don't you need the same treatment to atime after your patch 2/2? Honza > --- > fs/inode.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/inode.c b/fs/inode.c > index 35fd688168c5..54237f4242ff 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -168,6 +168,8 @@ int inode_init_always(struct super_block *sb, struct inode *inode) > inode->i_fop = &no_open_fops; > inode->i_ino = 0; > inode->__i_nlink = 1; > + inode->__i_ctime.tv_sec = 0; > + inode->__i_ctime.tv_nsec = 0; > inode->i_opflags = 0; > if (sb->s_xattr) > inode->i_opflags |= IOP_XATTR; > > -- > 2.41.0 >
On Fri, 2023-09-08 at 12:42 +0200, Jan Kara wrote: > On Thu 07-09-23 12:33:47, Jeff Layton wrote: > > With the advent of multigrain timestamps, we use inode_set_ctime_current > > to set the ctime, which can skip updating if the existing ctime appears > > to be in the future. Because we don't initialize this field at > > allocation time, that could prevent the ctime from being initialized > > properly when the inode is instantiated. > > > > Always initialize the ctime field to the epoch so that the filesystem > > can set the timestamps properly later. > > > > Reported-by: kernel test robot <oliver.sang@intel.com> > > Closes: https://lore.kernel.org/oe-lkp/202309071017.a64aca5e-oliver.sang@intel.com > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > Looks good but don't you need the same treatment to atime after your patch > 2/2? > > I don't think so. Most filesystems are doing something along the lines of this when allocating a new inode: inode->i_atime = inode->i_mtime = inode_set_ctime_current(inode); ...and I think they pretty much all have to initialize i_atime properly, since someone could stat the inode before an atime update occurs. > > --- > > fs/inode.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/fs/inode.c b/fs/inode.c > > index 35fd688168c5..54237f4242ff 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -168,6 +168,8 @@ int inode_init_always(struct super_block *sb, struct inode *inode) > > inode->i_fop = &no_open_fops; > > inode->i_ino = 0; > > inode->__i_nlink = 1; > > + inode->__i_ctime.tv_sec = 0; > > + inode->__i_ctime.tv_nsec = 0; > > inode->i_opflags = 0; > > if (sb->s_xattr) > > inode->i_opflags |= IOP_XATTR; > > > > -- > > 2.41.0 > >
On Fri 08-09-23 07:41:45, Jeff Layton wrote: > On Fri, 2023-09-08 at 12:42 +0200, Jan Kara wrote: > > On Thu 07-09-23 12:33:47, Jeff Layton wrote: > > > With the advent of multigrain timestamps, we use inode_set_ctime_current > > > to set the ctime, which can skip updating if the existing ctime appears > > > to be in the future. Because we don't initialize this field at > > > allocation time, that could prevent the ctime from being initialized > > > properly when the inode is instantiated. > > > > > > Always initialize the ctime field to the epoch so that the filesystem > > > can set the timestamps properly later. > > > > > > Reported-by: kernel test robot <oliver.sang@intel.com> > > > Closes: https://lore.kernel.org/oe-lkp/202309071017.a64aca5e-oliver.sang@intel.com > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > Looks good but don't you need the same treatment to atime after your patch > > 2/2? > > > > > > I don't think so. Most filesystems are doing something along the lines > of this when allocating a new inode: > > inode->i_atime = inode->i_mtime = inode_set_ctime_current(inode); > > ...and I think they pretty much all have to initialize i_atime properly, > since someone could stat the inode before an atime update occurs. Ah, right. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > > > --- > > > fs/inode.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/fs/inode.c b/fs/inode.c > > > index 35fd688168c5..54237f4242ff 100644 > > > --- a/fs/inode.c > > > +++ b/fs/inode.c > > > @@ -168,6 +168,8 @@ int inode_init_always(struct super_block *sb, struct inode *inode) > > > inode->i_fop = &no_open_fops; > > > inode->i_ino = 0; > > > inode->__i_nlink = 1; > > > + inode->__i_ctime.tv_sec = 0; > > > + inode->__i_ctime.tv_nsec = 0; > > > inode->i_opflags = 0; > > > if (sb->s_xattr) > > > inode->i_opflags |= IOP_XATTR; > > > > > > -- > > > 2.41.0 > > > > > -- > Jeff Layton <jlayton@kernel.org>
diff --git a/fs/inode.c b/fs/inode.c index 35fd688168c5..54237f4242ff 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -168,6 +168,8 @@ int inode_init_always(struct super_block *sb, struct inode *inode) inode->i_fop = &no_open_fops; inode->i_ino = 0; inode->__i_nlink = 1; + inode->__i_ctime.tv_sec = 0; + inode->__i_ctime.tv_nsec = 0; inode->i_opflags = 0; if (sb->s_xattr) inode->i_opflags |= IOP_XATTR;
With the advent of multigrain timestamps, we use inode_set_ctime_current to set the ctime, which can skip updating if the existing ctime appears to be in the future. Because we don't initialize this field at allocation time, that could prevent the ctime from being initialized properly when the inode is instantiated. Always initialize the ctime field to the epoch so that the filesystem can set the timestamps properly later. Reported-by: kernel test robot <oliver.sang@intel.com> Closes: https://lore.kernel.org/oe-lkp/202309071017.a64aca5e-oliver.sang@intel.com Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/inode.c | 2 ++ 1 file changed, 2 insertions(+)