| Message ID | 20231004185530.82088-3-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: new accessor methods for inode atime and mtime | expand |
On Wed, Oct 4, 2023 at 9:56 PM Jeff Layton <jlayton@kernel.org> wrote: > > The recent change to use discrete integers instead of struct timespec64 > shaved 8 bytes off of struct inode, but it also moves the i_lock > into the previous cacheline, away from the fields that it protects. > > Move i_generation above the i_lock, which moves the new 4 byte hole to > just after the i_fsnotify_mask in my setup. Might be good to mention that this hole has a purpose... > > Suggested-by: Amir Goldstein <amir73il@gmail.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > include/linux/fs.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 485b5e21c8e5..686c9f33e725 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -677,6 +677,7 @@ struct inode { > 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; > @@ -733,7 +734,6 @@ struct inode { > unsigned i_dir_seq; > }; > > - __u32 i_generation; > > #ifdef CONFIG_FSNOTIFY > __u32 i_fsnotify_mask; /* all events this inode cares about */ If you post another version, please leave a comment here + /* 32bit hole reserved for expanding i_fsnotify_mask to 64bit */ Thanks, Amir.
On Thu, 2023-10-05 at 08:08 +0300, Amir Goldstein wrote: > On Wed, Oct 4, 2023 at 9:56 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > The recent change to use discrete integers instead of struct timespec64 > > shaved 8 bytes off of struct inode, but it also moves the i_lock > > into the previous cacheline, away from the fields that it protects. > > > > Move i_generation above the i_lock, which moves the new 4 byte hole to > > just after the i_fsnotify_mask in my setup. > > Might be good to mention that this hole has a purpose... > > > > > Suggested-by: Amir Goldstein <amir73il@gmail.com> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > include/linux/fs.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 485b5e21c8e5..686c9f33e725 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -677,6 +677,7 @@ struct inode { > > 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; > > @@ -733,7 +734,6 @@ struct inode { > > unsigned i_dir_seq; > > }; > > > > - __u32 i_generation; > > > > #ifdef CONFIG_FSNOTIFY > > __u32 i_fsnotify_mask; /* all events this inode cares about */ > > If you post another version, please leave a comment here > > + /* 32bit hole reserved for expanding i_fsnotify_mask to 64bit */ > Sure. I suppose we could create a union there too if you really want to reserve it: union { __u32 i_fsnotify_mask; __u64 __i_fsnotify_mask_ext; };
On Thu, Oct 5, 2023 at 3:45 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Thu, 2023-10-05 at 08:08 +0300, Amir Goldstein wrote: > > On Wed, Oct 4, 2023 at 9:56 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > The recent change to use discrete integers instead of struct timespec64 > > > shaved 8 bytes off of struct inode, but it also moves the i_lock > > > into the previous cacheline, away from the fields that it protects. > > > > > > Move i_generation above the i_lock, which moves the new 4 byte hole to > > > just after the i_fsnotify_mask in my setup. > > > > Might be good to mention that this hole has a purpose... > > > > > > > > Suggested-by: Amir Goldstein <amir73il@gmail.com> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > include/linux/fs.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > > index 485b5e21c8e5..686c9f33e725 100644 > > > --- a/include/linux/fs.h > > > +++ b/include/linux/fs.h > > > @@ -677,6 +677,7 @@ struct inode { > > > 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; > > > @@ -733,7 +734,6 @@ struct inode { > > > unsigned i_dir_seq; > > > }; > > > > > > - __u32 i_generation; > > > > > > #ifdef CONFIG_FSNOTIFY > > > __u32 i_fsnotify_mask; /* all events this inode cares about */ > > > > If you post another version, please leave a comment here > > > > + /* 32bit hole reserved for expanding i_fsnotify_mask to 64bit */ > > > > Sure. > > I suppose we could create a union there too if you really want to > reserve it: > > union { > __u32 i_fsnotify_mask; > __u64 __i_fsnotify_mask_ext; > }; > No need. I was thinking it is going to be unsigned long i_fsnotify_mask; on 32bit arch, we either not support the high bitmask events or we fold them with the lower 32bit in the inode mask, because i_fsnotify_mask is an optimization to avoid going into inode marks iteration. Thanks, Amir.
diff --git a/include/linux/fs.h b/include/linux/fs.h index 485b5e21c8e5..686c9f33e725 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -677,6 +677,7 @@ struct inode { 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; @@ -733,7 +734,6 @@ struct inode { unsigned i_dir_seq; }; - __u32 i_generation; #ifdef CONFIG_FSNOTIFY __u32 i_fsnotify_mask; /* all events this inode cares about */
The recent change to use discrete integers instead of struct timespec64 shaved 8 bytes off of struct inode, but it also moves the i_lock into the previous cacheline, away from the fields that it protects. Move i_generation above the i_lock, which moves the new 4 byte hole to just after the i_fsnotify_mask in my setup. Suggested-by: Amir Goldstein <amir73il@gmail.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- include/linux/fs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)