diff mbox series

[1/2] fs: initialize inode->__i_ctime to the epoch

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

Commit Message

Jeff Layton Sept. 7, 2023, 4:33 p.m. UTC
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(+)

Comments

Jan Kara Sept. 8, 2023, 10:42 a.m. UTC | #1
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
>
Jeff Layton Sept. 8, 2023, 11:41 a.m. UTC | #2
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
> >
Jan Kara Sept. 8, 2023, 12:10 p.m. UTC | #3
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 mbox series

Patch

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;