Message-ID: <3042db2f803fbc711575ec4f1c4a273912a50904.camel@kernel.org>
Date: Mon, 01 Jul 2024 20:22:07 -0400
From: Jeff Layton <jlayton@...nel.org>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner
 <brauner@...nel.org>, Jan Kara <jack@...e.cz>, Steven Rostedt
 <rostedt@...dmis.org>, Masami Hiramatsu <mhiramat@...nel.org>, Mathieu
 Desnoyers <mathieu.desnoyers@...icios.com>, Chandan Babu R
 <chandan.babu@...cle.com>, Theodore Ts'o <tytso@....edu>, Andreas Dilger
 <adilger.kernel@...ger.ca>, Chris Mason <clm@...com>, Josef Bacik
 <josef@...icpanda.com>, David Sterba <dsterba@...e.com>,  Hugh Dickins
 <hughd@...gle.com>, Andrew Morton <akpm@...ux-foundation.org>,
 kernel-team@...com,  linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org,  linux-trace-kernel@...r.kernel.org,
 linux-xfs@...r.kernel.org,  linux-ext4@...r.kernel.org,
 linux-btrfs@...r.kernel.org, linux-mm@...ck.org,  linux-nfs@...r.kernel.org
Subject: Re: [PATCH 01/10] fs: turn inode ctime fields into a single ktime_t

On Mon, 2024-07-01 at 15:49 -0700, Darrick J. Wong wrote:
> On Wed, Jun 26, 2024 at 09:00:21PM -0400, Jeff Layton wrote:
> > The ctime is not settable to arbitrary values. It always comes from the
> > system clock, so we'll never stamp an inode with a value that can't be
> > represented there. If we disregard people setting their system clock
> > past the year 2262, there is no reason we can't replace the ctime fields
> > with a ktime_t.
> > 
> > Switch the ctime fields to a single ktime_t. Move the i_generation down
> > above i_fsnotify_mask and then move the i_version into the resulting 8
> > byte hole. This shrinks struct inode by 8 bytes total, and should
> > improve the cache footprint as the i_version and ctime are usually
> > updated together.
> > 
> > The one downside I can see to switching to a ktime_t is that if someone
> > has a filesystem with files on it that has ctimes outside the ktime_t
> > range (before ~1678 AD or after ~2262 AD), we won't be able to display
> > them properly in stat() without some special treatment in the
> > filesystem. The operating assumption here is that that is not a
> > practical problem.
> 
> What happens if a filesystem with the ability to store ctimes beyond
> whatever ktime_t supports (AFAICT 2^63-1 nanonseconds on either side of
> the Unix epoch)?  I think the behavior with your patch is that ktime_set
> clamps the ctime on iget because the kernel can't handle it?
> 
> It's a little surprising that the ctime will suddenly jump back in time
> to 2262, but maybe you're right that nobody will notice or care? ;)
> 
> 

Yeah, it'd be clamped at KTIME_MAX when we pull in the inode from disk,
a'la ktime_set.

I think it's important to note that the ctime is not settable from
userland, so if an on-disk ctime is outside of the ktime_t range, there
are only two possibilities:

1) the system clock was set to some time (far) in the future when the
file's metadata was last altered (bad clock? time traveling fs?).

...or...

2) the filesystem has been altered (fuzzing? deliberate doctoring?).

None of these seem like legitimate use cases so I'm arguing that we
shouldn't worry about them.

(...ok maybe the time travel one could be legit, but someone needs to
step up and make a case for it, if so.)

> > 
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > ---
> >  include/linux/fs.h | 26 +++++++++++---------------
> >  1 file changed, 11 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 5ff362277834..5139dec085f2 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -662,11 +662,10 @@ struct inode {
> >  	loff_t			i_size;
> >  	time64_t		i_atime_sec;
> >  	time64_t		i_mtime_sec;
> > -	time64_t		i_ctime_sec;
> >  	u32			i_atime_nsec;
> >  	u32			i_mtime_nsec;
> > -	u32			i_ctime_nsec;
> > -	u32			i_generation;
> > +	ktime_t			__i_ctime;
> > +	atomic64_t		i_version;
> >  	spinlock_t		i_lock;	/* i_blocks, i_bytes, maybe i_size */
> >  	unsigned short          i_bytes;
> >  	u8			i_blkbits;
> > @@ -701,7 +700,6 @@ struct inode {
> >  		struct hlist_head	i_dentry;
> >  		struct rcu_head		i_rcu;
> >  	};
> > -	atomic64_t		i_version;
> >  	atomic64_t		i_sequence; /* see futex */
> >  	atomic_t		i_count;
> >  	atomic_t		i_dio_count;
> > @@ -724,6 +722,8 @@ struct inode {
> >  	};
> >  
> >  
> > +	u32			i_generation;
> > +
> >  #ifdef CONFIG_FSNOTIFY
> >  	__u32			i_fsnotify_mask; /* all events this inode cares about */
> >  	/* 32-bit hole reserved for expanding i_fsnotify_mask */
> > @@ -1608,29 +1608,25 @@ static inline struct timespec64 inode_set_mtime(struct inode *inode,
> >  	return inode_set_mtime_to_ts(inode, ts);
> >  }
> >  
> > -static inline time64_t inode_get_ctime_sec(const struct inode *inode)
> > +static inline struct timespec64 inode_get_ctime(const struct inode *inode)
> >  {
> > -	return inode->i_ctime_sec;
> > +	return ktime_to_timespec64(inode->__i_ctime);
> >  }
> >  
> > -static inline long inode_get_ctime_nsec(const struct inode *inode)
> > +static inline time64_t inode_get_ctime_sec(const struct inode *inode)
> >  {
> > -	return inode->i_ctime_nsec;
> > +	return inode_get_ctime(inode).tv_sec;
> >  }
> >  
> > -static inline struct timespec64 inode_get_ctime(const struct inode *inode)
> > +static inline long inode_get_ctime_nsec(const struct inode *inode)
> >  {
> > -	struct timespec64 ts = { .tv_sec  = inode_get_ctime_sec(inode),
> > -				 .tv_nsec = inode_get_ctime_nsec(inode) };
> > -
> > -	return ts;
> > +	return inode_get_ctime(inode).tv_nsec;
> >  }
> >  
> >  static inline struct timespec64 inode_set_ctime_to_ts(struct inode *inode,
> >  						      struct timespec64 ts)
> >  {
> > -	inode->i_ctime_sec = ts.tv_sec;
> > -	inode->i_ctime_nsec = ts.tv_nsec;
> > +	inode->__i_ctime = ktime_set(ts.tv_sec, ts.tv_nsec);
> >  	return ts;
> >  }
> >  
> > 
> > -- 
> > 2.45.2
> > 
> > 

-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists