On Wed, Feb 26, 2020 at 11:18 AM Jan Kara <jack@xxxxxxx> wrote:
>
> On Mon 17-02-20 15:14:47, Amir Goldstein wrote:
> > With inotify, when a watch is set on a directory and on its child, an
> > event on the child is reported twice, once with wd of the parent watch
> > and once with wd of the child watch without the filename.
> >
> > With fanotify, when a watch is set on a directory and on its child, an
> > event on the child is reported twice, but it has the exact same
> > information - either an open file descriptor of the child or an encoded
> > fid of the child.
> >
> > The reason that the two identical events are not merged is because the
> > tag used for merging events in the queue is the child inode in one event
> > and parent inode in the other.
> >
> > For events with path or dentry data, use the dentry instead of inode as
> > the tag for event merging, so that the event reported on parent will be
> > merged with the event reported on the child.
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>
> I agree that reporting identical event twice seems wasteful but ...
>
> > @@ -312,7 +313,12 @@ struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
> > if (!event)
> > goto out;
> > init: __maybe_unused
> > - fsnotify_init_event(&event->fse, inode);
> > + /*
> > + * Use the dentry instead of inode as tag for event queue, so event
> > + * reported on parent is merged with event reported on child when both
> > + * directory and child watches exist.
> > + */
> > + fsnotify_init_event(&event->fse, (void *)dentry ?: inode);
>
> ... this seems quite ugly and also previously we could merge 'inode' events
> with others and now we cannot because some will carry "dentry where event
> happened" and other ones "inode with watch" as object identifier. So if you
> want to do this, I'd use "inode where event happened" as object identifier
> for fanotify.
<scratch head> Why didn't I think of that?...
I suppose you mean to just use:
fsnotify_init_event(&event->fse, id);
>
> Hum, now thinking about this, maybe we could clean this up even a bit more.
> event->inode is currently used only by inotify and fanotify for merging
> purposes. Now inotify could use its 'wd' instead of inode with exactly the
> same results, fanotify path or fid check is at least as strong as the inode
> check. So only for the case of pure "inode" events, we need to store inode
> identifier in struct fanotify_event - and we can do that in the union with
> struct path and completely remove the 'inode' member from fsnotify_event.
> Am I missing something?
That generally sounds good and I did notice it is strange that wd is not
being compared.
However, I think I was worried that comparing fid+name (in following patches)
would be more expensive than comparing dentry (or object inode) as a
"rule out first" in merge, so I preferred to keep the tag/dentry/id comparison
for fanotify_fid case.
Given this analysis (and assuming it is correct), would you like me to
just go a head
with the change suggested above? or anything beyond that?
Thanks,
Amir.
