Re: [PATCH v2 11/16] fanotify: prepare to encode both parent and child fid's

[Jan Kara <jack@suse.cz>]

On Thu 27-02-20 11:06:18, Amir Goldstein wrote:
> > > So overall I think this would be better. The question is whether the
> > > resulting code will really be more readable. I hope so because the
> > > structures are definitely nicer this way and things belonging logically
> > > together are now together. But you never know until you convert the code...
> > > Would you be willing to try this refactoring?
> >
> > Yes, but I would like to know what you think about the two 6 byte holes
> > Just let that space be wasted for the sake of nicer abstraction?
> > It seems like too much to me.
> >
> 
> What if we unite the fh and name into one struct and keep a 32bit hash of
> fh+name inside?
> 
> This will allow us to mitigate the cost of memcmp of fh+name in merge
> and get rid of objectid in fsnotify_event as you suggested.

I definitely want to get rid of objectid in the long run but I wouldn't
necessarily tie it to this series.

What I had in mind to do for fanotify to speed up merging (in the light of
your report) was to associate a hash with each fanotify event based on
values we care about most (probably store it in the same word as fanotify
event type) and compare based on this hash first. Possibly, we could also
add a small hash table (say 128 entries) to each fanotify group based on this
hash to speed up looking up candidates for merging.

> struct fanotify_fh_name {
>          union {
>                 struct {
>                        u8 fh_type;
>                        u8 fh_len;
>                        u8 name_len;
>                        u32 hash;
>                 };
>                 u64 hash_len;
>         };
>         union {
>                 unsigned char fh[FANOTIFY_INLINE_FH_LEN];
>                 unsigned char *ext_fh;
>         };
>         char name[0];
> };

So based on the above I wouldn't add just name hash to fanotify_fh_name at
this point...

								Honza

> struct fanotify_fid_event {
>         struct fanotify_event fae;
>         __kernel_fsid_t fsid;
>         struct fanotify_fh_name object_fh; /* name is empty */
> };
> 
> struct fanofify_name_event {
>         struct fanotify_fid_event ffe;
>         struct fanotify_fh_name dirent;
> };
> 
> So the only anomaly is that we use struct fanotify_fh_name
> to describe object_fh which never has a name.
> 
> I think we can live with that and trying to beat that would be
> over abstraction.
> 
> Thoughts?
> 
> Thanks,
> Amir.
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR