diff mbox series

[v2,08/16] fanotify: merge duplicate events on parent and child

Message ID 20200217131455.31107-9-amir73il@gmail.com
State New, archived
Headers show
Series Fanotify event with name info | expand

Commit Message

Amir Goldstein Feb. 17, 2020, 1:14 p.m. UTC
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@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Jan Kara Feb. 26, 2020, 9:18 a.m. UTC | #1
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@gmail.com>

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.

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?

								Honza
Amir Goldstein Feb. 26, 2020, 12:14 p.m. UTC | #2
On Wed, Feb 26, 2020 at 11:18 AM Jan Kara <jack@suse.cz> 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@gmail.com>
>
> 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.
Jan Kara Feb. 26, 2020, 2:38 p.m. UTC | #3
On Wed 26-02-20 14:14:50, Amir Goldstein wrote:
> On Wed, Feb 26, 2020 at 11:18 AM Jan Kara <jack@suse.cz> 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@gmail.com>
> >
> > 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);

Yes.

> > 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.

Yes, that could be a concern.
 
> 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?

Let's go just with the change suggested above for now. We can work on this
later (probably with optimizing of the fanotify merging code).

								Honza
diff mbox series

Patch

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 98c3cbf29003..dab7e9895e02 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -282,6 +282,7 @@  struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 	gfp_t gfp = GFP_KERNEL_ACCOUNT;
 	struct inode *id = fanotify_fid_inode(inode, mask, data, data_type);
 	const struct path *path = fsnotify_data_path(data, data_type);
+	struct dentry *dentry = fsnotify_data_dentry(data, data_type);
 
 	/*
 	 * For queues with unlimited length lost events are not expected and
@@ -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);
 	event->mask = mask;
 	if (FAN_GROUP_FLAG(group, FAN_REPORT_TID))
 		event->pid = get_pid(task_pid(current));