]> git.ipfire.org Git - thirdparty/public-inbox.git/commitdiff
fake_inotify + kqnotify: rewrite and combine code
authorEric Wong <e@80x24.org>
Fri, 8 Sep 2023 00:49:20 +0000 (00:49 +0000)
committerEric Wong <e@80x24.org>
Fri, 8 Sep 2023 10:38:39 +0000 (10:38 +0000)
KQNotify is now a subclass of FakeInotify since they're both
faking a subset of inotify; and both require directory scanning
via readdir() to detect new/deleted files.

ctime is no longer used with per-file stat to detect new files
with kevent.  That proved too unreliable either due to low
time resolution of the NetBSD/OpenBSD VFS and/or
Time::HiRes::stat being constrained by floating point to
represent `struct timespec', so instead we fuzz the time a bit
if the ctime is recent and merely compare filenames off readdir.

This fixes t/fake_inotify.t and t/kqnotify.t failures under NetBSD
and also removes workarounds for OpenBSD in t/kqnotify.t.  It
also allows us to to remove delays in tests by being more
aggressive in picking up new/deleted files in watch directories
by adjusting the time to scan if the ctime is recent.

This ought to may improve real-world reliability on all *BSDs
regardless of whether IO::KQueue is installed.

lib/PublicInbox/FakeInotify.pm
lib/PublicInbox/KQNotify.pm
t/dir_idle.t
t/fake_inotify.t
t/kqnotify.t

index 45b80f505557a95ed0cd157b36667477bee3fdc9..3a09b0309c9d033a4f5ff2cb8bac123db72fd152 100644 (file)
@@ -2,12 +2,12 @@
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 
 # for systems lacking Linux::Inotify2 or IO::KQueue, just emulates
-# enough of Linux::Inotify2
+# enough of Linux::Inotify2 we use.
 package PublicInbox::FakeInotify;
 use v5.12;
-use parent qw(Exporter);
 use Time::HiRes qw(stat);
 use PublicInbox::DS qw(add_timer);
+use Errno qw(ENOTDIR ENOENT);
 sub IN_MODIFY () { 0x02 } # match Linux inotify
 # my $IN_MOVED_FROM     0x00000040     /* File was moved from X.  */
 # my $IN_MOVED_TO = 0x80;
@@ -17,98 +17,124 @@ sub IN_DELETE () { 0x200 }
 sub IN_DELETE_SELF () { 0x400 }
 sub IN_MOVE_SELF () { 0x800 }
 
-our @EXPORT_OK = qw(fill_dirlist on_dir_change);
-
 my $poll_intvl = 2; # same as Filesys::Notify::Simple
 
-sub new { bless { watch => {}, dirlist => {} }, __PACKAGE__ }
+sub new { bless {}, __PACKAGE__ }
 
-sub fill_dirlist ($$$) {
-       my ($self, $path, $dh) = @_;
-       my $dirlist = $self->{dirlist}->{$path} = {};
-       while (defined(my $n = readdir($dh))) {
-               $dirlist->{$n} = undef if $n !~ /\A\.\.?\z/;
-       }
-}
+sub on_dir_change ($$$$$) { # used by KQNotify subclass
+       my ($self, $events, $dh, $path, $dir_delete) = @_;
+       my $old = $self->{dirlist}->{$path};
+       my @cur = grep(!/\A\.\.?\z/, readdir($dh));
+       $self->{dirlist}->{$path} = \@cur;
 
-# behaves like Linux::Inotify2->watch
-sub watch {
-       my ($self, $path, $mask) = @_;
-       my @st = stat($path) or return;
-       my $k = "$path\0$mask";
-       $self->{watch}->{$k} = $st[10]; # 10 - ctime
-       if ($mask & IN_DELETE) {
-               opendir(my $dh, $path) or return;
-               fill_dirlist($self, $path, $dh);
+       # new files:
+       my %tmp = map { $_ => undef } @cur;
+       delete @tmp{@$old};
+       push(@$events, map {
+               bless \"$path/$_", 'PublicInbox::FakeInotify::Event'
+       } keys %tmp);
+
+       if ($dir_delete) {
+               %tmp = map { $_ => undef } @$old;
+               delete @tmp{@cur};
+               push(@$events, map {
+                       bless \"$path/$_", 'PublicInbox::FakeInotify::GoneEvent'
+               } keys %tmp);
        }
-       bless [ $self->{watch}, $k ], 'PublicInbox::FakeInotify::Watch';
 }
 
-# also used by KQNotify since it kevent requires readdir on st_nlink
-# count changes.
-sub on_dir_change ($$$$$) {
-       my ($events, $dh, $path, $old_ctime, $dirlist) = @_;
-       my $oldlist = $dirlist->{$path};
-       my $newlist = $oldlist ? {} : undef;
-       while (defined(my $base = readdir($dh))) {
-               next if $base =~ /\A\.\.?\z/;
-               my $full = "$path/$base";
-               my @st = stat($full);
-               if (@st && $st[10] > $old_ctime) {
-                       push @$events,
-                               bless(\$full, 'PublicInbox::FakeInotify::Event')
+sub watch_open ($$$) { # used by KQNotify subclass
+       my ($self, $path, $dir_delete) = @_;
+       my ($fh, @st, @st0, $tries);
+       do {
+again:
+               unless (@st0 = stat($path)) {
+                       warn "W: stat($path): $!" if $! != ENOENT;
+                       return;
                }
-               if (!@st) {
-                       # ignore ENOENT due to race
-                       warn "unhandled stat($full) error: $!\n" if !$!{ENOENT};
-               } elsif ($newlist) {
-                       $newlist->{$base} = undef;
+               if (!(-d _ ? opendir($fh, $path) : open($fh, '<', $path))) {
+                       goto again if $! == ENOTDIR && ++$tries < 10;
+                       warn "W: open($path): $!" if $! != ENOENT;
+                       return;
                }
+               @st = stat($fh) or die "fstat($path): $!";
+       } while ("@st[0,1]" ne "@st0[0,1]" &&
+               ((++$tries < 10) || (warn(<<EOM) && return)));
+E: $path switching inodes too frequently to watch
+EOM
+       if (-d _) {
+               $self->{dirlist}->{$path} = [];
+               on_dir_change($self, [], $fh, $path, $$dir_delete);
+       } else {
+               $$dir_delete = 0;
        }
-       return if !$newlist;
-       delete @$oldlist{keys %$newlist};
-       $dirlist->{$path} = $newlist;
-       push(@$events, map {
-               bless \"$path/$_", 'PublicInbox::FakeInotify::GoneEvent'
-       } keys %$oldlist);
+       bless [ @st[0, 1, 10], $path, $fh ], 'PublicInbox::FakeInotify::Watch'
+}
+
+# behaves like Linux::Inotify2->watch
+sub watch {
+       my ($self, $path, $mask) = @_; # mask is ignored
+       my $dir_delete = $mask & IN_DELETE ? 1 : 0;
+       my $w = watch_open($self, $path, \$dir_delete) or return;
+       pop @$w; # no need to keep $fh open for non-kqueue
+       $self->{watch}->{"$path\0$dir_delete"} = $w;
+}
+
+sub gone ($$$) { # used by KQNotify subclass
+       my ($self, $ident, $path) = @_;
+       delete $self->{watch}->{$ident};
+       delete $self->{dirlist}->{$path};
+       bless(\$path, 'PublicInbox::FakeInotify::SelfGoneEvent');
+}
+
+# fuzz the time for freshly modified directories for low-res VFS
+sub dir_adj ($) {
+       my ($old_ctime) = @_;
+       my $now = Time::HiRes::time;
+       my $diff = $now - $old_ctime;
+       ($diff > -1 && $diff < 1) ? 1 : 0;
 }
 
 # behaves like non-blocking Linux::Inotify2->read
 sub read {
        my ($self) = @_;
-       my $watch = $self->{watch} or return ();
-       my $events = [];
-       my @watch_gone;
-       for my $x (keys %$watch) {
-               my ($path, $mask) = split(/\0/, $x, 2);
-               my @now = stat($path);
-               if (!@now && $!{ENOENT} && ($mask & IN_DELETE_SELF)) {
-                       push @$events, bless(\$path,
-                               'PublicInbox::FakeInotify::SelfGoneEvent');
-                       push @watch_gone, $x;
-                       delete $self->{dirlist}->{$path};
+       my $ret = [];
+       while (my ($ident, $w) = each(%{$self->{watch}})) {
+               if (!@$w) { # cancelled
+                       delete($self->{watch}->{$ident});
+                       next;
                }
-               next if !@now;
-               my $old_ctime = $watch->{$x};
-               $watch->{$x} = $now[10];
-               next if $old_ctime == $now[10];
-               if ($mask & IN_MODIFY) {
-                       push @$events,
-                               bless(\$path, 'PublicInbox::FakeInotify::Event')
-               } elsif ($mask & (MOVED_TO_OR_CREATE | IN_DELETE)) {
-                       if (opendir(my $dh, $path)) {
-                               on_dir_change($events, $dh, $path, $old_ctime,
-                                               $self->{dirlist});
-                       } elsif ($!{ENOENT}) {
-                               push @watch_gone, $x;
-                               delete $self->{dirlist}->{$path};
-                       } else {
-                               warn "W: opendir $path: $!\n";
+               my $dir_delete = (split(/\0/, $ident, 2))[1];
+               my ($old_dev, $old_ino, $old_ctime, $path) = @$w;
+               my @new_st = stat($path);
+               warn "W: stat($path): $!\n" if !@new_st && $! != ENOENT;
+               if (!@new_st || "$old_dev $old_ino" ne "@new_st[0,1]") {
+                       push @$ret, gone($self, $ident, $path);
+                       next;
+               }
+               if (-d _ && $new_st[10] > ($old_ctime - dir_adj($old_ctime))) {
+                       opendir(my $fh, $path) or do {
+                               if ($! == ENOENT || $! == ENOTDIR) {
+                                       push @$ret, gone($self, $ident, $path);
+                               } else {
+                                       warn "W: opendir($path): $!";
+                               }
+                               next;
+                       };
+                       @new_st = stat($fh) or die "fstat($path): $!";
+                       if ("$old_dev $old_ino" ne "@new_st[0,1]") {
+                               push @$ret, gone($self, $ident, $path);
+                               next;
                        }
+                       $w->[2] = $new_st[10];
+                       on_dir_change($self, $ret, $fh, $path, $dir_delete);
+               } elsif ($new_st[10] > $old_ctime) { # regular files, etc
+                       $w->[2] = $new_st[10];
+                       push @$ret, bless(\$path,
+                                       'PublicInbox::FakeInotify::Event');
                }
        }
-       delete @$watch{@watch_gone};
-       @$events;
+       @$ret;
 }
 
 sub poll_once {
@@ -120,15 +146,9 @@ sub poll_once {
 package PublicInbox::FakeInotify::Watch;
 use v5.12;
 
-sub cancel {
-       my ($self) = @_;
-       delete $self->[0]->{$self->[1]};
-}
+sub cancel { @{$_[0]} = () }
 
-sub name {
-       my ($self) = @_;
-       (split(/\0/, $self->[1], 2))[0];
-}
+sub name { $_[0]->[3] }
 
 package PublicInbox::FakeInotify::Event;
 use v5.12;
index 381711fae8f400bf11b3f0bb3316c04d66207336..2efa887d45800aaa67574c3336fadbde06ba11f0 100644 (file)
@@ -5,46 +5,31 @@
 # using IO::KQueue on *BSD systems.
 package PublicInbox::KQNotify;
 use v5.12;
+use parent qw(PublicInbox::FakeInotify);
 use IO::KQueue;
 use PublicInbox::DSKQXS; # wraps IO::KQueue for fork-safe DESTROY
-use PublicInbox::FakeInotify qw(fill_dirlist on_dir_change);
-use Time::HiRes qw(stat);
+use Errno qw(ENOENT);
 
 # NOTE_EXTEND detects rename(2), NOTE_WRITE detects link(2)
 sub MOVED_TO_OR_CREATE () { NOTE_EXTEND|NOTE_WRITE }
 
 sub new {
        my ($class) = @_;
-       bless { dskq => PublicInbox::DSKQXS->new, watch => {} }, $class;
+       bless { dskq => PublicInbox::DSKQXS->new }, $class;
 }
 
 sub watch {
        my ($self, $path, $mask) = @_;
-       my ($fh, $watch);
-       if (-d $path) {
-               opendir($fh, $path) or return;
-               my @st = stat($fh);
-               $watch = bless [ $fh, $path, $st[10] ],
-                       'PublicInbox::KQNotify::Watchdir';
-       } else {
-               open($fh, '<', $path) or return;
-               $watch = bless [ $fh, $path ], 'PublicInbox::KQNotify::Watch';
-       }
-       my $ident = fileno($fh);
+       my $dir_delete = $mask & NOTE_DELETE ? 1 : 0;
+       my $w = $self->watch_open($path, \$dir_delete) or return;
+       $w->[2] = pop @$w; # ctime is unused by this subclass
+       my $ident = fileno($w->[2]) // die "BUG: bad fileno $w->[2]: $!";
        $self->{dskq}->{kq}->EV_SET($ident, # ident (fd)
                EVFILT_VNODE, # filter
                EV_ADD | EV_CLEAR, # flags
                $mask, # fflags
-               0, 0); # data, udata
-       if ($mask & (MOVED_TO_OR_CREATE|NOTE_DELETE|NOTE_LINK|NOTE_REVOKE)) {
-               $self->{watch}->{$ident} = $watch;
-               if ($mask & (NOTE_DELETE|NOTE_LINK|NOTE_REVOKE)) {
-                       fill_dirlist($self, $path, $fh)
-               }
-       } else {
-               die "TODO Not implemented: $mask";
-       }
-       $watch;
+               0, $dir_delete); # data, udata
+       $self->{watch}->{$ident} = $w;
 }
 
 # emulate Linux::Inotify::fileno
@@ -61,54 +46,31 @@ sub blocking {}
 # behave like Linux::Inotify2->read
 sub read {
        my ($self) = @_;
-       my @kevents = $self->{dskq}->{kq}->kevent(0);
        my $events = [];
-       my @gone;
-       my $watch = $self->{watch};
-       for my $kev (@kevents) {
+       for my $kev ($self->{dskq}->{kq}->kevent(0)) {
                my $ident = $kev->[KQ_IDENT];
-               my $mask = $kev->[KQ_FFLAGS];
-               my ($dh, $path, $old_ctime) = @{$watch->{$ident}};
-               if (!defined($old_ctime)) {
-                       push @$events,
-                               bless(\$path, 'PublicInbox::FakeInotify::Event')
-               } elsif ($mask & (MOVED_TO_OR_CREATE|NOTE_DELETE|NOTE_LINK|
-                               NOTE_REVOKE|NOTE_RENAME)) {
-                       my @new_st = stat($path);
-                       if (!@new_st && $!{ENOENT}) {
-                               push @$events, bless(\$path,
-                                               'PublicInbox::FakeInotify::'.
-                                               'SelfGoneEvent');
-                               push @gone, $ident;
-                               delete $self->{dirlist}->{$path};
-                               next;
-                       }
-                       if (!@new_st) {
-                               warn "unhandled stat($path) error: $!\n";
-                               next;
-                       }
-                       $watch->{$ident}->[3] = $new_st[10]; # ctime
-                       rewinddir($dh);
-                       on_dir_change($events, $dh, $path, $old_ctime,
-                                       $self->{dirlist});
+               my $w = $self->{watch}->{$ident} or next;
+               if (!@$w) { # cancelled
+                       delete($self->{watch}->{$ident});
+                       next;
+               }
+               my $dir_delete = $kev->[KQ_UDATA];
+               my ($old_dev, $old_ino, $fh, $path) = @$w;
+               my @new_st = stat($path);
+               warn "W: stat($path): $!\n" if !@new_st && $! != ENOENT;
+               if (!@new_st || "$old_dev $old_ino" ne "@new_st[0,1]") {
+                       push(@$events, $self->gone($ident, $path));
+                       next;
+               }
+               if (-d _) {
+                       rewinddir($fh);
+                       $self->on_dir_change($events, $fh, $path, $dir_delete);
+               } else {
+                       push @$events, bless(\$path,
+                                       'PublicInbox::FakeInotify::Event');
                }
        }
-       delete @$watch{@gone};
        @$events;
 }
 
-package PublicInbox::KQNotify::Watch;
-use v5.12;
-
-sub name { $_[0]->[1] }
-
-sub cancel { close $_[0]->[0] or die "close: $!" }
-
-package PublicInbox::KQNotify::Watchdir;
-use v5.12;
-
-sub name { $_[0]->[1] }
-
-sub cancel { closedir $_[0]->[0] or die "closedir: $!" }
-
 1;
index 50e1dd275c1cdfbdbb9b797db43563c53051eb8b..9e66cc58731f4b3a29213fd177e16bb148900a33 100644 (file)
@@ -14,14 +14,12 @@ $di->add_watches(["$tmpdir/a", "$tmpdir/c"], 1);
 PublicInbox::DS->SetLoopTimeout(1000);
 my $end = 3 + now;
 local @PublicInbox::DS::post_loop_do = (sub { scalar(@x) == 0 && now < $end });
-tick(0.011);
 rmdir("$tmpdir/a/b") or xbail "rmdir $!";
 PublicInbox::DS::event_loop();
-is(scalar(@x), 1, 'got an event') and
+is(scalar(@x), 1, 'got an rmdir event') and
        is($x[0]->[0]->fullname, "$tmpdir/a/b", 'got expected fullname') and
        ok($x[0]->[0]->IN_DELETE, 'IN_DELETE set');
 
-tick(0.011);
 rmdir("$tmpdir/a") or xbail "rmdir $!";
 @x = ();
 $end = 3 + now;
@@ -30,7 +28,6 @@ is(scalar(@x), 1, 'got an event') and
        is($x[0]->[0]->fullname, "$tmpdir/a", 'got expected fullname') and
        ok($x[0]->[0]->IN_DELETE_SELF, 'IN_DELETE_SELF set');
 
-tick(0.011);
 rename("$tmpdir/c", "$tmpdir/j") or xbail "rmdir $!";
 @x = ();
 $end = 3 + now;
index 734ddbfb8f48c5ffc1ffaf1991d0b0f4c46ba0b5..56f64588d71f634db99f4feef95c7dfe50f3f23e 100644 (file)
@@ -1,13 +1,12 @@
 #!perl -w
-# Copyright (C) 2020-2021 all contributors <meta@public-inbox.org>
+# Copyright (C) all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 #
 # Ensure FakeInotify can pick up rename(2) and link(2) operations
 # used by Maildir writing tools
-use strict;
+use v5.12;
 use PublicInbox::TestCommon;
 use_ok 'PublicInbox::FakeInotify';
-my $MIN_FS_TICK = 0.011; # for low-res CONFIG_HZ=100 systems
 my ($tmpdir, $for_destroy) = tmpdir();
 mkdir "$tmpdir/new" or BAIL_OUT "mkdir: $!";
 mkdir "$tmpdir/new/rmd" or BAIL_OUT "mkdir: $!";
@@ -18,35 +17,35 @@ my $fi = PublicInbox::FakeInotify->new;
 my $mask = PublicInbox::FakeInotify::MOVED_TO_OR_CREATE();
 my $w = $fi->watch("$tmpdir/new", $mask);
 
-tick $MIN_FS_TICK;
 rename("$tmpdir/tst", "$tmpdir/new/tst") or BAIL_OUT "rename: $!";
 my @events = map { $_->fullname } $fi->read;
-is_deeply(\@events, ["$tmpdir/new/tst"], 'rename(2) detected');
+is_deeply(\@events, ["$tmpdir/new/tst"], 'rename(2) detected') or
+       diag explain(\@events);
 
-tick $MIN_FS_TICK;
 open $fh, '>', "$tmpdir/tst" or BAIL_OUT "open: $!";
 close $fh or BAIL_OUT "close: $!";
 link("$tmpdir/tst", "$tmpdir/new/link") or BAIL_OUT "link: $!";
 @events = map { $_->fullname } $fi->read;
-is_deeply(\@events, ["$tmpdir/new/link"], 'link(2) detected');
+is_deeply(\@events, ["$tmpdir/new/link"], 'link(2) detected') or
+       diag explain(\@events);
 
 $w->cancel;
-tick $MIN_FS_TICK;
 link("$tmpdir/new/tst", "$tmpdir/new/link2") or BAIL_OUT "link: $!";
 @events = map { $_->fullname } $fi->read;
-is_deeply(\@events, [], 'link(2) not detected after cancel');
+is_deeply(\@events, [], 'link(2) not detected after cancel') or
+       diag explain(\@events);
 $fi->watch("$tmpdir/new", PublicInbox::FakeInotify::IN_DELETE());
 
-tick $MIN_FS_TICK;
 rmdir("$tmpdir/new/rmd") or xbail "rmdir: $!";
 @events = $fi->read;
-is_deeply([map{ $_->fullname }@events], ["$tmpdir/new/rmd"], 'rmdir detected');
-ok($events[0]->IN_DELETE, 'IN_DELETE set on rmdir');
+is_deeply([map{ $_->fullname }@events], ["$tmpdir/new/rmd"], 'rmdir detected') or
+       diag explain(\@events);
+ok($events[-1]->IN_DELETE, 'IN_DELETE set on rmdir');
 
-tick $MIN_FS_TICK;
 unlink("$tmpdir/new/tst") or xbail "unlink: $!";
 @events = grep { ref =~ /Gone/ } $fi->read;
-is_deeply([map{ $_->fullname }@events], ["$tmpdir/new/tst"], 'unlink detected');
+is_deeply([map{ $_->fullname }@events], ["$tmpdir/new/tst"], 'unlink detected') or
+       diag explain(\@events);
 ok($events[0]->IN_DELETE, 'IN_DELETE set on unlink');
 
 PublicInbox::DS->Reset;
index 213818068340caf0ed0771e3e87575031c407b70..edecf2e118413b57cc6e172065c4d1c9e0dd57e7 100644 (file)
@@ -17,41 +17,18 @@ my $kqn = PublicInbox::KQNotify->new;
 my $mask = PublicInbox::KQNotify::MOVED_TO_OR_CREATE();
 my $w = $kqn->watch("$tmpdir/new", $mask);
 
-# Unlike FreeBSD, OpenBSD (tested 7.3) kevent NOTE_EXTEND doesn't detect
-# renames into directories reliably.  It's kevent(3) manpage doesn't
-# document this behavior (unlike FreeBSD), but it sometimes works...
 open my $fh, '>', "$tmpdir/tst";
 close $fh;
 rename("$tmpdir/tst", "$tmpdir/new/tst");
 my $hit = [ map { $_->fullname } $kqn->read ];
-my $try = 0;
-while (!@$hit && $^O eq 'openbsd' && $try++ < 30) {
-       diag "retrying NOTE_EXTEND detection for $^O (#$try)";
-       # kevent can totally ignore the event, so delaying hasn't worked;
-       # keep doing the same thing until kevent notices one of them
-       open $fh, '>', "$tmpdir/tst";
-       close $fh;
-       rename("$tmpdir/tst", "$tmpdir/new/tst");
-       $hit = [ map { $_->fullname } $kqn->read ]
-}
 is_deeply($hit, ["$tmpdir/new/tst"],
                'rename(2) detected (via NOTE_EXTEND)')
                or diag explain($hit);
 
-# OpenBSD (tested 7.3) doesn't reliably trigger NOTE_WRITE on link(2)
-# into directories, but usually it does (and more reliably than rename(2)
-# above) and doesn't drop the event entirely.
 open $fh, '>', "$tmpdir/tst";
 close $fh;
 link("$tmpdir/tst", "$tmpdir/new/link");
 my @read = map { $_->fullname } $kqn->read;
-$try = 0;
-while (!@read && $^O eq 'openbsd' && $try++ < 30) {
-       diag "delaying and retrying NOTE_WRITE detection for $^O (#$try)";
-       tick;
-       # no need to link(2) again, at least, kevent can just be late, here
-       @read = map { $_->fullname } $kqn->read;
-}
 $hit = [ grep(m!/link$!, @read) ];
 is_deeply($hit, ["$tmpdir/new/link"], 'link(2) detected (via NOTE_WRITE)')
        or diag explain(\@read);