Re: [GIT PULL] Fsnotify cleanups

[Amir Goldstein <amir73il@gmail.com>]

On Mon, Jun 11, 2018 at 4:36 PM, Jan Kara <jack@suse.cz> wrote:
>
> On Sun 10-06-18 20:49:16, Amir Goldstein wrote:
> > On Sat, Jun 9, 2018 at 9:46 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > > On Sat, Jun 9, 2018 at 8:30 PM, Linus Torvalds
> > > <torvalds@linux-foundation.org> wrote:
> > >> On Fri, Jun 8, 2018 at 11:57 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >>>
> > >>> We embed fsnotify_obj in struct inode and fsnotify_obj_mask in struct mount.
> > >>
> > >> So I'd *really* like to see just a pointer, not an embedded struct.
> > >>
> > >> Yes, if you get rid of the mask from the embedded struct (so that it
> > >> only contains a pointer), you do get rid of the odd alignment issues
> > >> and the need for "packed".
> > >>
> > >> But from previous experience, once you embed a structure, that
> > >> structure tends to grow. Because it can, and it's so convenient.
> > >> Suddently it has a spinlock in it too etc.
> > >>
> > >
> > > Fair enough.
> > >
> > >> So if you can make do with just the pointer, it would actually be
> > >> nicer to expose it as such. Then you can also avoid the header file
> > >> dependency chain, because you can just pre-declare the structure (like
> > >> it does now) and
> > >>
> > >>     struct fsnotify_mark_connector;
> > >>     ..
> > >>         struct fsnotify_mark_connector __rcu    *i_fsnotify_marks;
> > >>
> > >> in the inode. That way the core header files don't need to worry about
> > >> the fsnotify details, and don't need to include fsnotify headers.
> > >>
> > >> And we can do inode packing without having to know (not that it
> > >> happens all that often - everybody would *love* to shrink the inode
> > >> structure, but it's just hard. Because everybody also wants to put
> > >> their own data into the inode ..)
> > >>
> > >> Can't the generalization code just take a pointer to a __rcu pointer
> > >> to fsnotify_mark_connector, obviating the need for the fsnotify_obj
> > >> structure definition?
> > >>
> > >
> >
> > Jan,
> >
> > I reworked the cleanup patches to get rid of fsnotify_obj and pushed to:
> > https://github.com/amir73il/linux.git fsnotify-cleanup
>
> Thanks!
>
> > Only last 5 patches from fsnotify_for_v4.18-rc1 have been modified
> > and I removed your S-O-B from the modified patches.
> >
> > This leaves struct inode unchanged, in fact no changes to code outside
> > fsnotify/audit at all.
> >
> > mask is now a member of connector for the purpose of generalizing
> > add/remove mark, but struct inode/mount still have a copy of the mask
> > for the purpose of the VFS optimizations.
>
> Looking through those patches, is it really beneficial to add mask to
> connector when you keep it in inode / vfsmount? A helper function to get
> mask from connector would make the same refactoring possible as well, won't
> it?
>
> And adding a helper function to set mask given connector would get rid of
> the remaining checks for connector type due to mask manipulations...
>

By moving the checks for object type into the helper?
Maybe I am missing your point.

Anyway, my thinking was:

What do we have to loose from keeping the mask also inside the connector?

Not much. We didn't change the size of connector struct
and it hardly adds any complexity / performance cost.

What do we have to gain from keeping the mask also inside the connector?

We can later get rid of the copy of mask in inode struct as I wrote.
I will follow up on that.

Thanks,
Amir.