From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.2 required=3.0 tests=ALL_TRUSTED,AWL,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF shortcircuit=no autolearn=ham autolearn_force=no version=3.4.6 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 0BB9D1F55F for ; Fri, 8 Sep 2023 00:49:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=80x24.org; s=selector1; t=1694134161; bh=WT2Rt0piJA/a6yl8aJM1dX7RK6TJgFN4bFTu0XQgajc=; h=Date:From:To:Subject:References:In-Reply-To:From; b=S2LhdznD5GQDl5Nk0WYxjN2apxgInItwcagXuM5m5wOKu2/wG+2h/MCOYwkahRmdd s5yZLWQIe5lDVLq7jo5y3ntYCI2CqzPbePjRakrp9OQnRQsZfAK8yPsW1kRl8Jw6Y6 eX032rVv1YXRd7ltY6pVE7CxWcm9fkx7534BWRGE= Date: Fri, 8 Sep 2023 00:49:20 +0000 From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 2/2] fake_inotify + kqnotify: rewrite and combine code Message-ID: <20230908004920.M875610@dcvr> References: <20230905073725.169-1-e@80x24.org> <20230905073915.M53970@dcvr> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20230905073915.M53970@dcvr> List-Id: 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+ # 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(<{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 +# Copyright (C) all contributors # License: AGPL-3.0+ # # 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);