Received-SPF: pass (google.com: domain of linux-kernel+bounces-183624-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 20 May 2024 12:43:47 +0200
From: Jan Kara <jack@suse.cz>
To: Jeff Layton <jlayton@kernel.org>
Cc: Christian Brauner <brauner@kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>, Jan Kara <jack@suse.cz>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-fsdevel@vger.kernel.org, Amir Goldstein <amir73il@gmail.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fs: switch timespec64 fields in inode to discrete
 integers
Message-ID: <20240520104347.j5fz4eem52bv2wzg@quack3>
References: <20240517-amtime-v1-1-7b804ca4be8f@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240517-amtime-v1-1-7b804ca4be8f@kernel.org>

On Fri 17-05-24 20:08:40, Jeff Layton wrote:
> Adjacent struct timespec64's pack poorly. Switch them to discrete
> integer fields for the seconds and nanoseconds. This shaves 8 bytes off
> struct inode with a garden-variety Fedora Kconfig on x86_64, but that
> also moves the i_lock into the previous cacheline, away from the fields
> it protects.
> 
> To remedy that, move i_generation above the i_lock, which moves the new
> 4-byte hole to just after the i_fsnotify_mask in my setup. Amir has
> plans to use that to expand the i_fsnotify_mask, so add a comment to
> that effect as well.
> 
> Cc: Amir Goldstein <amir73il@gmail.com>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

I like this! Although it doesn't possibly result in less memory being used
by the slab cache in the end as Matthew points out, it still makes the
struct more dense and if nothing else it gives us more space for future
growth ;).  So feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> For reference (according to pahole):
> 
>     Before:	/* size: 624, cachelines: 10, members: 53 */
>     After: 	/* size: 616, cachelines: 10, members: 56 */
> 
> I've done some lightweight testing with this and it seems fine, but I
> wouldn't mind seeing it sit in -next for a bit to make sure I haven't
> missed anything.
> ---
>  include/linux/fs.h | 48 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b7b9b3b79acc..45e8766de7d8 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -660,9 +660,13 @@ struct inode {
>  	};
>  	dev_t			i_rdev;
>  	loff_t			i_size;
> -	struct timespec64	__i_atime;
> -	struct timespec64	__i_mtime;
> -	struct timespec64	__i_ctime; /* use inode_*_ctime accessors! */
> +	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;
>  	spinlock_t		i_lock;	/* i_blocks, i_bytes, maybe i_size */
>  	unsigned short          i_bytes;
>  	u8			i_blkbits;
> @@ -719,10 +723,10 @@ struct inode {
>  		unsigned		i_dir_seq;
>  	};
>  
> -	__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 */
>  	struct fsnotify_mark_connector __rcu	*i_fsnotify_marks;
>  #endif
>  
> @@ -1544,23 +1548,27 @@ struct timespec64 inode_set_ctime_current(struct inode *inode);
>  
>  static inline time64_t inode_get_atime_sec(const struct inode *inode)
>  {
> -	return inode->__i_atime.tv_sec;
> +	return inode->i_atime_sec;
>  }
>  
>  static inline long inode_get_atime_nsec(const struct inode *inode)
>  {
> -	return inode->__i_atime.tv_nsec;
> +	return inode->i_atime_nsec;
>  }
>  
>  static inline struct timespec64 inode_get_atime(const struct inode *inode)
>  {
> -	return inode->__i_atime;
> +	struct timespec64 ts = { .tv_sec  = inode_get_atime_sec(inode),
> +				 .tv_nsec = inode_get_atime_nsec(inode) };
> +
> +	return ts;
>  }
>  
>  static inline struct timespec64 inode_set_atime_to_ts(struct inode *inode,
>  						      struct timespec64 ts)
>  {
> -	inode->__i_atime = ts;
> +	inode->i_atime_sec = ts.tv_sec;
> +	inode->i_atime_nsec = ts.tv_nsec;
>  	return ts;
>  }
>  
> @@ -1569,28 +1577,32 @@ static inline struct timespec64 inode_set_atime(struct inode *inode,
>  {
>  	struct timespec64 ts = { .tv_sec  = sec,
>  				 .tv_nsec = nsec };
> +
>  	return inode_set_atime_to_ts(inode, ts);
>  }
>  
>  static inline time64_t inode_get_mtime_sec(const struct inode *inode)
>  {
> -	return inode->__i_mtime.tv_sec;
> +	return inode->i_mtime_sec;
>  }
>  
>  static inline long inode_get_mtime_nsec(const struct inode *inode)
>  {
> -	return inode->__i_mtime.tv_nsec;
> +	return inode->i_mtime_nsec;
>  }
>  
>  static inline struct timespec64 inode_get_mtime(const struct inode *inode)
>  {
> -	return inode->__i_mtime;
> +	struct timespec64 ts = { .tv_sec  = inode_get_mtime_sec(inode),
> +				 .tv_nsec = inode_get_mtime_nsec(inode) };
> +	return ts;
>  }
>  
>  static inline struct timespec64 inode_set_mtime_to_ts(struct inode *inode,
>  						      struct timespec64 ts)
>  {
> -	inode->__i_mtime = ts;
> +	inode->i_mtime_sec = ts.tv_sec;
> +	inode->i_mtime_nsec = ts.tv_nsec;
>  	return ts;
>  }
>  
> @@ -1604,23 +1616,27 @@ static inline struct timespec64 inode_set_mtime(struct inode *inode,
>  
>  static inline time64_t inode_get_ctime_sec(const struct inode *inode)
>  {
> -	return inode->__i_ctime.tv_sec;
> +	return inode->i_ctime_sec;
>  }
>  
>  static inline long inode_get_ctime_nsec(const struct inode *inode)
>  {
> -	return inode->__i_ctime.tv_nsec;
> +	return inode->i_ctime_nsec;
>  }
>  
>  static inline struct timespec64 inode_get_ctime(const struct inode *inode)
>  {
> -	return inode->__i_ctime;
> +	struct timespec64 ts = { .tv_sec  = inode_get_ctime_sec(inode),
> +				 .tv_nsec = inode_get_ctime_nsec(inode) };
> +
> +	return ts;
>  }
>  
>  static inline struct timespec64 inode_set_ctime_to_ts(struct inode *inode,
>  						      struct timespec64 ts)
>  {
> -	inode->__i_ctime = ts;
> +	inode->i_ctime_sec = ts.tv_sec;
> +	inode->i_ctime_nsec = ts.tv_nsec;
>  	return ts;
>  }
>  
> 
> ---
> base-commit: 7ee332c9f12bc5b380e36919cd7d056592a7073f
> change-id: 20240517-amtime-68a7ebc76f45
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR