* [PATCH 2/2] fake_inotify + kqnotify: rewrite and combine code
@ 2023-09-08 0:49 7% ` Eric Wong
0 siblings, 0 replies; 1+ results
From: Eric Wong @ 2023-09-08 0:49 UTC (permalink / raw)
To: meta
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.
---
It was a total PITA to develop and test this patch
across 3 BSDs with and without IO::KQueue installed.
And still other *BSD problems with -watch in t/imapd*.t ...
lib/PublicInbox/FakeInotify.pm | 186 ++++++++++++++++++---------------
lib/PublicInbox/KQNotify.pm | 96 +++++------------
t/dir_idle.t | 5 +-
t/fake_inotify.t | 27 +++--
t/kqnotify.t | 23 ----
5 files changed, 146 insertions(+), 191 deletions(-)
diff --git a/lib/PublicInbox/FakeInotify.pm b/lib/PublicInbox/FakeInotify.pm
index 45b80f50..3a09b030 100644
--- a/lib/PublicInbox/FakeInotify.pm
+++ b/lib/PublicInbox/FakeInotify.pm
@@ -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;
diff --git a/lib/PublicInbox/KQNotify.pm b/lib/PublicInbox/KQNotify.pm
index 381711fa..2efa887d 100644
--- a/lib/PublicInbox/KQNotify.pm
+++ b/lib/PublicInbox/KQNotify.pm
@@ -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;
diff --git a/t/dir_idle.t b/t/dir_idle.t
index 50e1dd27..9e66cc58 100644
--- a/t/dir_idle.t
+++ b/t/dir_idle.t
@@ -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;
diff --git a/t/fake_inotify.t b/t/fake_inotify.t
index 734ddbfb..56f64588 100644
--- a/t/fake_inotify.t
+++ b/t/fake_inotify.t
@@ -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;
diff --git a/t/kqnotify.t b/t/kqnotify.t
index 21381806..edecf2e1 100644
--- a/t/kqnotify.t
+++ b/t/kqnotify.t
@@ -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);
^ permalink raw reply related [relevance 7%]
Results 1-1 of 1 | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2023-09-05 7:37 [PATCH 1/2] dskqxs: get rid of needless confess check Eric Wong
2023-09-05 7:39 ` there's no 2/2, yet :x Eric Wong
2023-09-08 0:49 7% ` [PATCH 2/2] fake_inotify + kqnotify: rewrite and combine code Eric Wong
Code repositories for project(s) associated with this public inbox
https://80x24.org/public-inbox.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).