On Thu, 2023-10-05 at 08:08 +0300, Amir Goldstein wrote:
> On Wed, Oct 4, 2023 at 9:56 PM Jeff Layton <[email protected]> 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 <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > 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;
};
--
Jeff Layton <[email protected]>
On Thu, Oct 5, 2023 at 3:45 PM Jeff Layton <[email protected]> wrote:
>
> On Thu, 2023-10-05 at 08:08 +0300, Amir Goldstein wrote:
> > On Wed, Oct 4, 2023 at 9:56 PM Jeff Layton <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Jeff Layton <[email protected]>
> > > ---
> > > 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.