From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id BA0371FF0AF for ; Fri, 06 Nov 2026 01:50:45 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 84185216FD; Fri, 06 Nov 2026 01:49:46 +0100 (CET) From: Thomas Lamprecht To: pve-devel@lists.proxmox.com Subject: [PATCH v3 storage 07/76] iscsi: rate-limit session rescans per target Date: Fri, 6 Nov 2026 00:30:42 +0100 Message-ID: <20261105233336.2178064-8-t.lamprecht@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261105233336.2178064-1-t.lamprecht@proxmox.com> References: <20261105233336.2178064-1-t.lamprecht@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793926177719 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.340 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: NHOM7RA4D5DFGHVHU3O4VDYNOR5NUBNO X-Message-ID-Hash: NHOM7RA4D5DFGHVHU3O4VDYNOR5NUBNO X-MailFrom: t.lamprecht@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 --- 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