Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20231004185530.82088-1-jlayton@kernel.org> <20231004185530.82088-3-jlayton@kernel.org>
 <CAOQ4uxgtyaBTM1bOSSGmsk+F4ZwsK+-N5ZZ3wAt_nv_E6G3C7Q@mail.gmail.com> <9af5c896da0c39c66d0555879c04c23fd853c9de.camel@kernel.org>
In-Reply-To: <9af5c896da0c39c66d0555879c04c23fd853c9de.camel@kernel.org>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Thu, 5 Oct 2023 16:07:01 +0300
Message-ID: <CAOQ4uxhXG=oMKLgCPwnLAToxtF7MCgv7jk72iG9gZ_6B2exbwg@mail.gmail.com>
Subject: Re: [PATCH v2 89/89] fs: move i_generation into new hole created
 after timestamp conversion
To:     Jeff Layton <jlayton@kernel.org>
Cc:     Christian Brauner <brauner@kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>, Jan Kara <jack@suse.cz>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Oct 5, 2023 at 3:45=E2=80=AFPM Jeff Layton <jlayton@kernel.org> wro=
te:
>
> On Thu, 2023-10-05 at 08:08 +0300, Amir Goldstein wrote:
> > On Wed, Oct 4, 2023 at 9:56=E2=80=AFPM Jeff Layton <jlayton@kernel.org>=
 wrote:
> > >
> > > The recent change to use discrete integers instead of struct timespec=
64
> > > 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 t=
o
> > > 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 i=
node 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.