From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [GIT PULL] Fsnotify cleanups
Date: Mon, 11 Jun 2018 15:36:28 +0200 [thread overview]
Message-ID: <20180611133628.e35npuuv425n2425@quack2.suse.cz> (raw)
In-Reply-To: <CAOQ4uxjmRHS_6guXTk=c4XswZ+MQvzL+gqdm7ON4TCKc6TbweA@mail.gmail.com>
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...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2018-06-11 13:36 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-07 15:02 [GIT PULL] Fsnotify cleanups Jan Kara
2018-06-07 16:34 ` Linus Torvalds
2018-06-08 13:27 ` Jan Kara
2018-06-08 20:34 ` Linus Torvalds
2018-06-09 6:57 ` Amir Goldstein
2018-06-09 8:00 ` Amir Goldstein
2018-06-11 16:12 ` Jan Kara
2018-06-11 16:31 ` Amir Goldstein
2018-06-09 17:30 ` Linus Torvalds
2018-06-09 18:46 ` Amir Goldstein
2018-06-10 17:49 ` Amir Goldstein
2018-06-11 13:36 ` Jan Kara [this message]
2018-06-11 13:58 ` Amir Goldstein
2018-06-11 16:03 ` Jan Kara
2018-06-11 16:38 ` Amir Goldstein
2018-06-11 19:51 ` Amir Goldstein
2018-06-13 13:21 ` Jan Kara
2018-06-13 13:56 ` Amir Goldstein
2018-06-13 22:17 ` Amir Goldstein
2018-06-22 16:44 ` Jan Kara
2018-06-23 7:42 ` Amir Goldstein
2018-06-11 11:08 ` Jan Kara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180611133628.e35npuuv425n2425@quack2.suse.cz \
--to=jack@suse.cz \
--cc=amir73il@gmail.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).