[PATCH v3 storage 07/76] iscsi: rate-limit session rescans per target

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

The node-wide rescan limit lets frequent activation of one storage
starve other targets, leaving new LUNs and capacity changes unseen.

Limit rescans to once every ten seconds per target instead. Protect the
persistent timestamp with a file lock, released before rescanning.
Storage entries for the same target share the limit; unmanaged sessions
are not rescanned. Bookkeeping failures warn and skip the rescan without
making established sessions unusable.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
Whether pvestatd (the main caller, but not the only one) should rescan
as often as every 10s is another question.

 src/PVE/Storage/ISCSIPlugin.pm     |  54 +++++++++-----
 src/test/Makefile                  |   3 +-
 src/test/run_iscsi_rescan_tests.pl | 109 +++++++++++++++++++++++++++++
 3 files changed, 146 insertions(+), 20 deletions(-)
 create mode 100755 src/test/run_iscsi_rescan_tests.pl

diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index 3bba27a6..20605859 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -3,7 +3,8 @@ package PVE::Storage::ISCSIPlugin;
 use strict;
 use warnings;
 
-use File::stat;
+use Digest::SHA qw(sha256_hex);
+use Fcntl qw(LOCK_EX LOCK_NB);
 use IO::Dir;
 use IO::File;
 
@@ -212,27 +213,42 @@ sub iscsi_logout {
     run_command([$ISCSIADM, '--mode', 'node', '--targetname', $target, '--logout']);
 }
 
-my $rescan_filename = "/var/run/pve-iscsi-rescan.lock";
+our $ISCSI_RESCAN_DIR = '/run';
+
+sub claim_iscsi_rescan {
+    my ($target) = @_;
+
+    my $filename = "$ISCSI_RESCAN_DIR/pve-iscsi-rescan-" . sha256_hex($target);
+    my $claimed = eval {
+        my $fh = IO::File->new($filename, '+>>') or die "cannot open '$filename' - $!\n";
+        flock($fh, LOCK_EX | LOCK_NB) or die "cannot lock '$filename' - $!\n";
+        $fh->seek(0, 0) or die "cannot seek '$filename' - $!\n";
+        my $last = $fh->getline();
+        die "cannot read '$filename' - $!\n" if $fh->error();
+        die "invalid timestamp in '$filename'\n" if defined($last) && $last !~ m/^\d+\n$/;
+        my $now = time();
+        my $elapsed = defined($last) ? $now - $last : undef;
+        my $claim = !defined($elapsed) || $elapsed < 0 || $elapsed >= 10;
+
+        if ($claim) {
+            $fh->truncate(0) or die "cannot update '$filename' - $!\n";
+            $fh->print("$now\n") && $fh->flush() or die "cannot update '$filename' - $!\n";
+        }
+        $fh->close() or die "cannot close '$filename' - $!\n";
+        $claim;
+    };
+    if (my $err = $@) {
+        warn "could not rate-limit iSCSI rescan for target '$target' - $err";
+        return 0;
+    }
+    return $claimed;
+}
 
 sub iscsi_session_rescan {
-    my $session_list = shift;
+    my ($target, $session_list) = @_;
 
     assert_iscsi_support();
-
-    my $rstat = stat($rescan_filename);
-
-    if (!$rstat) {
-        if (my $fh = IO::File->new($rescan_filename, "a")) {
-            utime undef, undef, $fh;
-            close($fh);
-        }
-    } else {
-        my $atime = $rstat->atime;
-        my $tdiff = time() - $atime;
-        # avoid frequent rescans
-        return if !($tdiff < 0 || $tdiff > 10);
-        utime undef, undef, $rescan_filename;
-    }
+    return if !claim_iscsi_rescan($target);
 
     foreach my $session (@$session_list) {
         my $cmd = [$ISCSIADM, '--mode', 'session', '--sid', $session->{session_id}, '--rescan'];
@@ -517,7 +533,7 @@ sub activate_storage {
         warn $@ if $@;
     } else {
         # make sure we get all devices
-        iscsi_session_rescan($sessions);
+        iscsi_session_rescan($scfg->{target}, $sessions);
     }
 }
 
diff --git a/src/test/Makefile b/src/test/Makefile
index 0b9b95b2..404a1524 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -40,8 +40,9 @@ test_ovf: run_ovf_tests.pl
 test_volume_access: run_volume_access_tests.pl
 	./run_volume_access_tests.pl
 
-test_iscsi: run_iscsi_discovery_tests.pl run_iscsi_session_tests.pl run_iscsi_volume_tests.pl
+test_iscsi: run_iscsi_discovery_tests.pl run_iscsi_rescan_tests.pl run_iscsi_session_tests.pl run_iscsi_volume_tests.pl
 	./run_iscsi_discovery_tests.pl
+	./run_iscsi_rescan_tests.pl
 	./run_iscsi_session_tests.pl
 	./run_iscsi_volume_tests.pl
 
diff --git a/src/test/run_iscsi_rescan_tests.pl b/src/test/run_iscsi_rescan_tests.pl
new file mode 100755
index 00000000..22d70c9c
--- /dev/null
+++ b/src/test/run_iscsi_rescan_tests.pl
@@ -0,0 +1,109 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Digest::SHA qw(sha256_hex);
+use Errno qw(EACCES ENOSPC);
+use Fcntl qw(LOCK_EX LOCK_NB);
+use File::Temp qw(tempdir);
+use Test::MockModule;
+use Test::More;
+
+use lib ('.', '..');
+use PVE::Storage::ISCSIPlugin;
+
+my $dir = tempdir(CLEANUP => 1);
+local $PVE::Storage::ISCSIPlugin::ISCSI_RESCAN_DIR = $dir;
+my $plugin = Test::MockModule->new('PVE::Storage::ISCSIPlugin');
+my @rescans;
+my $target;
+$plugin->mock(
+    iscsi_session =>
+        sub { [{ session_id => 1, portal => 'portal' }, { session_id => 2, portal => 'portal' }] },
+    iscsi_portals => sub { ['portal'] },
+    run_command => sub {
+        my ($cmd) = @_;
+        push @rescans, $cmd->[-2];
+        is($cmd->[-1], '--rescan', 'only session rescans run');
+        my $filename = "$dir/pve-iscsi-rescan-" . sha256_hex($target);
+        open(my $fh, '>>', $filename) or die $!;
+        ok(flock($fh, LOCK_EX | LOCK_NB), 'timestamp lock is released before rescan');
+    },
+);
+
+my sub activate {
+    my ($storeid, $name) = @_;
+    $target = $name;
+    PVE::Storage::ISCSIPlugin->activate_storage($storeid, { target => $target }, {});
+}
+
+activate('first', 'iqn.first');
+is_deeply(\@rescans, [1, 2], 'all sessions of first target rescanned');
+activate('second', 'iqn.second/../unsafe');
+is_deeply(\@rescans, [1, 2, 1, 2], 'second target has its own window and a safe filename');
+activate('duplicate', 'iqn.first');
+is(scalar(@rescans), 4, 'duplicate storage shares the target allowance');
+activate('second', 'iqn.second/../unsafe');
+is(scalar(@rescans), 4, 'independent activation cache does not bypass persisted claim');
+my $filename = "$dir/pve-iscsi-rescan-" . sha256_hex('iqn.first');
+my $inode = (stat($filename))[1];
+open(my $stamp, '>', $filename) or die $!;
+print $stamp (time() - 11) . "\n";
+close($stamp) or die $!;
+activate('duplicate', 'iqn.first');
+is_deeply(\@rescans, [1, 2, 1, 2, 1, 2], 'next window rescans every session again');
+activate('second', 'iqn.second/../unsafe');
+is(scalar(@rescans), 6, 'other target remains rate limited');
+
+is((stat($filename))[1], $inode, 'timestamp updates do not replace the locked inode');
+my @files = glob("$dir/*");
+is(scalar(@files), 2, 'exactly one timestamp and lock file per target');
+
+my $io = Test::MockModule->new('IO::File');
+my $new = IO::File->can('new');
+my $print = IO::File->can('print');
+my $close = IO::File->can('close');
+my $failure = '';
+$io->mock(
+    new => sub {
+        if ($failure eq 'open') { $! = EACCES; return undef; }
+        $new->(@_);
+    },
+    print => sub {
+        if ($failure eq 'update') { $! = ENOSPC; return 0; }
+        $print->(@_);
+    },
+    close => sub {
+        my $res = $close->(@_);
+        if ($failure eq 'close') { $! = ENOSPC; return 0; }
+        return $res;
+    },
+);
+for my $case (qw(open update close lock)) {
+    my $name = "iqn.failure-$case";
+    my $file = "$dir/pve-iscsi-rescan-" . sha256_hex($name);
+    my $lock;
+    if ($case eq 'lock') {
+        open($lock, '>>', $file) or die $!;
+        flock($lock, LOCK_EX | LOCK_NB) or die $!;
+    }
+    $failure = $case;
+    my $warnings = '';
+    local $SIG{__WARN__} = sub { $warnings .= $_[0] };
+    my $before = scalar(@rescans);
+    eval { activate('failure', $name) };
+    is($@, '', "$case failure does not fail activation");
+    like(
+        $warnings,
+        qr/^could not rate-limit iSCSI rescan for target '\Q$name\E' - /,
+        "$case failure identifies best-effort bookkeeping",
+    );
+    is(scalar(@rescans), $before, "$case failure skips the rescan");
+    close($lock) if $lock;
+}
+$failure = '';
+activate('recovered', 'iqn.failure-update');
+is(scalar(@rescans), 8, 'empty timestamp after failed update permits a subsequent first claim');
+
+done_testing();
-- 
2.47.3