Re: [GIT PULL] Fsnotify cleanups

[Jan Kara <jack@suse.cz>]

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@suse.cz> 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@suse.com>
SUSE Labs, CR