Re: linux-next fsnotify mod breaks tail -f

[Amir Goldstein]

On Fri, Dec 11, 2020 at 1:45 AM Hugh Dickins <hughd@xxxxxxxxxx> wrote:
>
> Hi Jan, Amir,
>
> There's something wrong with linux-next commit ca7fbf0d29ab
> ("fsnotify: fix events reported to watching parent and child").
>
> If I revert that commit, no problem;
> but here's a one-line script "tailed":
>
> for i in 1 2 3 4 5; do date; sleep 1; done &
>
> Then if I run that (same result doing ./tailed after chmod a+x):
>
> sh tailed >log; tail -f log
>
> the "tail -f log" behaves in one of three ways:
>
> 1) On a console, before graphical screen, no problem,
>    it shows the five lines coming from "date" as you would expect.
> 2) From xterm or another tty, shows just the first line from date,
>    but after I wait and Ctrl-C out, "cat log" shows all five lines.
> 3) From xterm or another tty, doesn't even show that first line.
>
> The before/after graphical screen thing seems particularly weird:
> I expect you'll end up with a simpler explanation for what's
> causing that difference.
>
> tailed and log are on ext4, if that's relevant;
> ah, I just tried on tmpfs, and saw no problem there.

Nice riddle Hugh :)
Thanks for this early testing!

I was able to reproduce this.
The outcome does not depend on the type of terminal or filesystem
it depends on the existence of a watch on the parent dir of the log file.
Running ' inotifywait -m . &' will stop tail from getting notifications:

echo > log
tail -f log &
sleep 1
echo "can you see this?" >> log
inotifywait -m . &
sleep 1
echo "how about this?" >> log
kill $(jobs -p)

I suppose with a graphical screen you have systemd or other services
in the system watching the logs/home dir in your test env.

Attached fix patch. I suppose Jan will want to sqhash it.

We missed a subtle logic change in the switch from inode/child marks
to parent/inode marks terminology.

Before the change (!inode_mark && child_mark) meant that name
was not NULL and should be discarded (which the old code did).
After the change (!parent_mark && inode_mark) is not enough to
determine if name should be discarded (it should be discarded only
for "events on child"), so another check is needed.

Thanks,
Amir.
From c7ea57c66c8c9f9607928bf7c55fc409eecc3e57 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Fri, 11 Dec 2020 10:19:36 +0200
Subject: [PATCH] fsnotify: fix for fix events reported to watching parent and
 child

The child watch is expecting an event without file name and without
the ON_CHILD flag.

Reported-by: Hugh Dickins <hughd@xxxxxxxxxx>
Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---
 fs/notify/fsnotify.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index a0da9e766992..30d422b8c0fc 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -291,13 +291,18 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
 		}
 		if (!inode_mark)
 			return 0;
+	}
 
+	if (mask & FS_EVENT_ON_CHILD) {
 		/*
 		 * Some events can be sent on both parent dir and child marks
 		 * (e.g. FS_ATTRIB).  If both parent dir and child are
 		 * watching, report the event once to parent dir with name (if
 		 * interested) and once to child without name (if interested).
+		 * The child watcher is expecting an event without a file name
+		 * and without the FS_EVENT_ON_CHILD flag.
 		 */
+		mask &= ~FS_EVENT_ON_CHILD;
 		dir = NULL;
 		name = NULL;
 	}
-- 
2.25.1