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@xxxxxxxxx>
> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
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@xxxxxxx>
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@xxxxxxxxxx>
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
