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 035621F55F for ; Thu, 7 Sep 2023 21:56:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=80x24.org; s=selector1; t=1694123788; bh=RnSf0EnxgmPRE5L3WU0gFVdQ57Z4tBEHo3VH3YYnZRY=; h=From:To:Subject:Date:From; b=NpuMm52sqTYw0ZV9WEYLGDLmAe5n21oYcHYPMmfaI9Cbm2C6yZ1/rphXfgkLMyBkt RsbmsYfQRiNrHa/4z0PkGPaPNurKz4elagY9OiGj83485Pesp8V4PTfaweBcXQXl/B iP/K/UwFmdaP9qeXE/18WCXVf0Gwk5cZBTxVUZ88= From: Eric Wong To: spew@80x24.org Subject: [PATCH] fake_inotify + kqnotify: rewrite and combine code Date: Thu, 7 Sep 2023 21:56:27 +0000 Message-ID: <20230907215627.2186626-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: KQNotify is now a subclass of FakeInotify since they're similar in that they both require directory scanning via readdir()+stat() and comparing against previous-known st_ctime. Getting the up-to-date ctime of the latest file is critical in getting t/fake_inotify.t to pass under NetBSD. This fixes t/fake_inotify.t and t/kqnotify.t failures under NetBSD; and may improve real-world reliability on all *BSDs regardless of whether IO::KQueue is installed. --- lib/PublicInbox/FakeInotify.pm | 169 +++++++++++++++++---------------- lib/PublicInbox/KQNotify.pm | 75 ++------------- t/dir_idle.t | 2 +- t/fake_inotify.t | 22 +++-- 4 files changed, 111 insertions(+), 157 deletions(-) diff --git a/lib/PublicInbox/FakeInotify.pm b/lib/PublicInbox/FakeInotify.pm index 45b80f50..7b178c8e 100644 --- a/lib/PublicInbox/FakeInotify.pm +++ b/lib/PublicInbox/FakeInotify.pm @@ -17,97 +17,112 @@ 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 on_dir_change ($$$$$) { + my ($self, $events, $dh, $path, $old_ctime) = @_; + my $old = $self->{dirlist}->{$path}; # only set if watching IN_DELETE + my $latest = $old_ctime; + my @cur = grep(!/\A\.\.?\z/, readdir($dh)); + for (@cur) { + my $f = "$path/$_"; + if (my @st = stat($f)) { + if ($st[10] > $old_ctime) { + $latest = $st[10] if $st[10] > $latest; + push(@$events, bless(\$f, + 'PublicInbox::FakeInotify::Event')); + } + } else { # ignore ENOENT due to race + warn "W: stat($f): $!\n" if !$!{ENOENT}; + } + } + # -watch only cares for new files, lei wants deletes + if ($old) { + $self->{dirlist}->{$path} = \@cur; + my %gone = map { $_ => undef } @$old; + delete @gone{@cur}; + push(@$events, map { + bless \"$path/$_", 'PublicInbox::FakeInotify::GoneEvent' + } keys %gone); + } + # updating directory ctime w/ $latest is required for t/fake_inotify.t + # to pass on NetBSD 9.3 (even when all FS ops are serialized, + # I wonder if ctime updates get delayed/batched somehow). + $latest; +} -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 watch_open ($$$) { # used by KQNotify subclass + my ($self, $path, $dir_delete) = @_; + my ($fh, @st, @st0, $tries); + do { + @st0 = stat($path) or return; + (-d _ ? opendir($fh, $path) : open($fh, '<', $path)) or return; + @st = stat($fh) or die "fstat($path): $!"; + } while ("@st[0,1]" ne "@st0[0,1]" && + ((++$tries < 10) || (warn(<{dirlist}->{$path} = [] if $dir_delete; + my $ct = on_dir_change($self, [], $fh, $path, $st[10]); + $st[10] = $ct if $ct > $st[10]; } + bless [ @st[0, 1, 10], $path, $fh ], 'PublicInbox::FakeInotify::Watch' } # 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); - } - bless [ $self->{watch}, $k ], 'PublicInbox::FakeInotify::Watch'; + my ($self, $path, $mask) = @_; # mask is ignored + my $w = watch_open($self, $path, $mask & IN_DELETE) or return; + pop @$w; # no need to keep $fh open for non-kqueue + $self->{watch}->{$path} = $w; } -# 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') - } - if (!@st) { - # ignore ENOENT due to race - warn "unhandled stat($full) error: $!\n" if !$!{ENOENT}; - } elsif ($newlist) { - $newlist->{$base} = undef; +sub gone_event ($$$) { + my ($self, $ident, $path) = @_; + delete $self->{watch}->{$ident}; + delete $self->{dirlist}->{$path}; + bless(\$path, 'PublicInbox::FakeInotify::SelfGoneEvent'); +} + +sub ckwatch_i ($$$) { # used by KQNotify subclass + my ($self, $events, $ident) = @_; + my $w = $self->{watch}->{$ident} or return; + return delete($self->{watch}->{$ident}) if !@$w; # cancelled + # $fh is only defined for KQNotify subclass + my ($old_dev, $old_ino, $old_ctime, $path, $fh) = @$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, gone_event($self, $ident, $path); + } elsif ($fh || $new_st[10] > $old_ctime) { + $w->[2] = $new_st[10]; + if (-d _) { + if ($fh) { # kevent requires FDs stay open + rewinddir($fh); + } elsif (!opendir($fh, $path)) { + push @$events, gone_event($self, $ident, $path); + return; + } + my $ctime = on_dir_change($self, $events, $fh, $path, + $old_ctime); + $w->[2] = $ctime if $ctime > $w->[2]; + } else { + push @$events, bless(\$path, + 'PublicInbox::FakeInotify::Event'); } } - return if !$newlist; - delete @$oldlist{keys %$newlist}; - $dirlist->{$path} = $newlist; - push(@$events, map { - bless \"$path/$_", 'PublicInbox::FakeInotify::GoneEvent' - } keys %$oldlist); } # 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}; - } - 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"; - } - } + for my $ident (keys %{$self->{watch}}) { + ckwatch_i($self, $events, $ident); } - delete @$watch{@watch_gone}; @$events; } @@ -120,15 +135,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..d75e4716 100644 --- a/lib/PublicInbox/KQNotify.pm +++ b/lib/PublicInbox/KQNotify.pm @@ -5,46 +5,28 @@ # 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); # 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 $w = $self->watch_open($path, $mask & NOTE_DELETE) or return; + my $ident = fileno($w->[4]); $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; + $self->{watch}->{$ident} = $w; } # emulate Linux::Inotify::fileno @@ -61,54 +43,11 @@ 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) { - 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}); - } + for my $kev ($self->{dskq}->{kq}->kevent(0)) { + $self->ckwatch_i($events, $kev->[KQ_IDENT]); } - 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..70711103 100644 --- a/t/dir_idle.t +++ b/t/dir_idle.t @@ -17,7 +17,7 @@ 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'); diff --git a/t/fake_inotify.t b/t/fake_inotify.t index 734ddbfb..9f42c592 100644 --- a/t/fake_inotify.t +++ b/t/fake_inotify.t @@ -1,10 +1,10 @@ #!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 @@ -21,32 +21,38 @@ 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: $!"; +tick $MIN_FS_TICK; @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;