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

[Kleber Souza]

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 at 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 at gmail.com>
>> Signed-off-by: Jan Kara <jack at suse.cz>
>> (backported from commit f367a62a7cad2447d835a9f14fc63997a9137246)
>> [PHLin: backported with the same logic]
>> Signed-off-by: Po-Hsu Lin <po-hsu.lin at 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