[E,SRU,PATCHv2,1/1] fanotify: merge duplicate events on parent and child

From: Amir Goldstein <amir73il@gmail.com>

BugLink: https://bugs.launchpad.net/bugs/1878748

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
object id 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 victim inode instead of the
watched inode as the object id for event merging, so that the event
reported on parent will be merged with the event reported on the child.

Link: https://lore.kernel.org/r/20200319151022.31456-9-amir73il@gmail.com
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
(backported from commit f367a62a7cad2447d835a9f14fc63997a9137246)
[PHLin: backported with the same logic]
Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
---
 fs/notify/fanotify/fanotify.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

On Wed, May 20, 2020 at 03:59:43PM +0800, Po-Hsu Lin wrote:
> From: Amir Goldstein <amir73il@gmail.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1878748
> 
> 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
> object id 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 victim inode instead of the
> watched inode as the object id for event merging, so that the event
> reported on parent will be merged with the event reported on the child.
> 
> Link: https://lore.kernel.org/r/20200319151022.31456-9-amir73il@gmail.com
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> (backported from commit f367a62a7cad2447d835a9f14fc63997a9137246)
> [PHLin: backported with the same logic]
> Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
> ---
>  fs/notify/fanotify/fanotify.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 5778d13..f1ca308 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -314,7 +314,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 victim inode instead of the watching inode as the id 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, (unsigned long)id);
>  	event->mask = mask;
>  	if (FAN_GROUP_FLAG(group, FAN_REPORT_TID))
>  		event->pid = get_pid(task_pid(current));

Though fsnotify_init_event expects an inode before commit
dfc2d2594e4a79204a3967585245f00644b8f838, id is an inode, and it is only used
for comparisons, as pointed out by that same commit.

As it has been tested, I am acking.

Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

> -- 
> 2.7.4
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

On 2020-05-20 12:34, Thadeu Lima de Souza Cascardo wrote:
> On Wed, May 20, 2020 at 03:59:43PM +0800, Po-Hsu Lin wrote:
>> From: Amir Goldstein <amir73il@gmail.com>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1878748
>>
>> 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
>> object id 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 victim inode instead of the
>> watched inode as the object id for event merging, so that the event
>> reported on parent will be merged with the event reported on the child.
>>
>> Link: https://lore.kernel.org/r/20200319151022.31456-9-amir73il@gmail.com
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> Signed-off-by: Jan Kara <jack@suse.cz>
>> (backported from commit f367a62a7cad2447d835a9f14fc63997a9137246)
>> [PHLin: backported with the same logic]
>> Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
>> ---
>>  fs/notify/fanotify/fanotify.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
>> index 5778d13..f1ca308 100644
>> --- a/fs/notify/fanotify/fanotify.c
>> +++ b/fs/notify/fanotify/fanotify.c
>> @@ -314,7 +314,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 victim inode instead of the watching inode as the id 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, (unsigned long)id);
>>  	event->mask = mask;
>>  	if (FAN_GROUP_FLAG(group, FAN_REPORT_TID))
>>  		event->pid = get_pid(task_pid(current));
> 
> Though fsnotify_init_event expects an inode before commit
> dfc2d2594e4a79204a3967585245f00644b8f838, id is an inode, and it is only used
> for comparisons, as pointed out by that same commit.

It's used only for a single comparison but the argument expected didn't change,
so we would be introducing a new build warning:

/tmp/kernel-kleber-f361613-kguP/build/fs/notify/fanotify/fanotify.c: In function 'fanotify_alloc_event':
  CC [M]  fs/ocfs2/dcache.o
/tmp/kernel-kleber-f361613-kguP/build/fs/notify/fanotify/fanotify.c:322:35: warning: passing argument 2 of 'fsnotify_init_event' makes pointer from integer without a cast [-Wint-conversion]
  322 |  fsnotify_init_event(&event->fse, (unsigned long)id);
      |                                   ^~~~~~~~~~~~~~~~~
      |                                   |
      |                                   long unsigned int
In file included from /tmp/kernel-kleber-f361613-kguP/build/fs/notify/fanotify/fanotify.c:4:
/tmp/kernel-kleber-f361613-kguP/build/include/linux/fsnotify_backend.h:501:26: note: expected 'struct inode *' but argument is of type 'long unsigned int'
  501 |            struct inode *inode)
      |            ~~~~~~~~~~~~~~^~~~~


I think we should either keep it as 'struct inode *' and pass only 'id', or we should
backport dfc2d2594e4a79204a3967585245f00644b8f838 as pre-req. I think the former
would be simpler.

So I'm NAK'ing this patch.


Thanks,
Kleber