Added Amir to CC as the author of those changes.
On Thu 07-06-18 09:34:53, Linus Torvalds wrote:
> On Thu, Jun 7, 2018 at 8:02 AM Jan Kara <jack@xxxxxxx> wrote:
> >
> > several fsnotify cleanups unifying handling of different watch
> > types.
>
> Grr.
>
> Why is this growing things like "fsnotify_obj_inode()" helpers in <linux/fs.h>?
>
> It has nothing to do with generic fs code. The only things that can
> possibly use that already have to include fsnotify-specific headers,
> where things like this belong.
Fair point. When I was merging Amir's patches I've actually tried to put
this function in fsnotify-specific headers but compilation was failing for
some reason I don't remember and so I left it in fs.h. Anyway, this is easy
to fix.
> It also adds a "struct fsnotify_obj i_fsnotify" to the struct inode,
> and marks it "packed", so now architectures that have issues with
> alignment might have issues depending on random changes to 'struct
> inode'.
Yeah, "packed" is ugly but I wanted to avoid growing of struct inode just
because of unnecessary padding on 64-bit archs... And I missed the fact
that the packed structure is not properly aligned as a whole. Thanks for
catching this!
The alignment could be fixed by having
struct inode {
...
struct fsnotify_obj i_fsnotify __attribute__ ((aligned(sizeof(void *))));
...
}
but that's too ugly even for my twisted taste. And before someone asks,
adding aligned attribute to the definition of fsnotify_obj structure makes
it grow the padding at the end again.
I'll ponder a bit more about this. I guess I'll try just moving the 'mask'
field from struct inode to struct fsnotify_connector. That avoids all the
alignment / size issues, makes struct inode smaller, and allows
simplification of fsnotify code as well. Just we'll have to benchmark how
much the additional dereference to access the mask is visible.
> Plus it (again) causes more disturbance to a core header file that
> fsnotify shouldn't touch. We had a forward declaration and a pointer.
>
> So no. I'm not pulling this. I don't think it's a "cleanup". Maybe it
> cleans up the fsnotify code, but it uglifies code that is much more
> important.
Understood.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
