[PATCH storage v3 4/4] zfsnvme: wait up to 30 seconds for the shared storage lock

[Joaquin Varela <joaquinvarela@neatech.ar>]

The core takes the shared storage lock without a timeout around
vdisk_alloc, vdisk_free, vdisk_clone, vdisk_create_base, rename_volume
and rename_snapshot, so cfs_lock applies its default of 10 seconds for
acquiring it. A zfsnvme allocation holds that lock for several SSH
round trips (read the target state, reserve the NSID, create the zvol,
wait for its device, export and verify the namespace), and keeps it
while it waits for the target lock, which a destroy or rollback on the
same target can hold for several seconds. When two guests get disks on
one storage at the same time, the second one can then fail with "got
lock request timeout" although nothing went wrong.

Override cluster_lock_storage() to wait up to 30 seconds for the shared
lock when the caller passes no timeout, the same acquisition budget as
for the target lock. An explicit timeout is passed on unchanged (for 0,
cfs_lock still applies its own default of 10 seconds), and so are the
execution timeout, error handling and exclusion of the parent method.

Signed-off-by: Joaquin Varela <joaquinvarela@neatech.ar>
---
 src/PVE/Storage/ZFSNVMePlugin.pm |  9 ++++++++
 src/test/zfsnvme_test.pm         | 35 +++++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/ZFSNVMePlugin.pm b/src/PVE/Storage/ZFSNVMePlugin.pm
index dfd43d43..67595350 100644
--- a/src/PVE/Storage/ZFSNVMePlugin.pm
+++ b/src/PVE/Storage/ZFSNVMePlugin.pm
@@ -2452,6 +2452,15 @@ sub clone_image($class, $scfg, $storeid, $volname, $vmid, $snap = undef) {
     return "$basename/$name";
 }
 
+sub cluster_lock_storage($class, $storeid, $shared, $timeout, $func, @param) {
+    # Remote lifecycle operations take several SSH round trips while the core
+    # holds this lock. Allow a bounded queue of such operations, using the same
+    # acquisition budget as the target lock. Keep caller-supplied deadlines and
+    # the parent's execution timeout, error handling and exclusion unchanged.
+    $timeout = $nvmet_lock_wait if $shared && !defined($timeout);
+    return $class->SUPER::cluster_lock_storage($storeid, $shared, $timeout, $func, @param);
+}
+
 sub alloc_image($class, $storeid, $scfg, $vmid, $fmt, $name, $size) {
     die "unsupported format '$fmt'" if $fmt ne 'raw';
     die "illegal name '$name' - should be 'vm-$vmid-*'\n"
diff --git a/src/test/zfsnvme_test.pm b/src/test/zfsnvme_test.pm
index 8a963bfe..7a218bf5 100644
--- a/src/test/zfsnvme_test.pm
+++ b/src/test/zfsnvme_test.pm
@@ -1108,6 +1108,39 @@ my @ownership = (
 # The shared storage lock
 # ---------------------------------------------------------------------------
 
+subtest 'cluster lock timeout defaults and passthrough' => sub {
+    my @base_calls;
+    $parent_mock->redefine(
+        cluster_lock_storage => sub($class, $storeid, $shared, $timeout, $func, @param) {
+            push @base_calls, [$class, $storeid, $shared, $timeout, @param];
+            return $func->(@param);
+        },
+    );
+    for my $case (
+        ['shared default', 1, undef, 30],
+        ['shared explicit', 1, 12, 12],
+        ['shared zero (the parent applies its own default)', 1, 0, 0],
+        ['local default', 0, undef, undef],
+    ) {
+        my ($label, $shared, $timeout, $expected) = $case->@*;
+        my $result = $PLUGIN->cluster_lock_storage(
+            'nvmetest',
+            $shared,
+            $timeout,
+            sub(@args) { return \@args },
+            'left',
+            'right',
+        );
+        is_deeply(
+            $base_calls[-1],
+            [$PLUGIN, 'nvmetest', $shared, $expected, 'left', 'right'],
+            "$label: the parent gets its arguments",
+        );
+        is_deeply($result, ['left', 'right'], "$label: the callback runs with its parameters");
+    }
+    $parent_mock->unmock('cluster_lock_storage');
+};
+
 subtest 'vdisk_alloc dispatches under the shared storage lock' => sub {
     my $cluster_mock = Test::MockModule->new('PVE::Cluster');
     my (@locks, @allocations);
@@ -1131,7 +1164,7 @@ subtest 'vdisk_alloc dispatches under the shared storage lock' => sub {
         'nvmetest:vm-105-disk-0',
         'core allocation returns the allocated volume ID',
     );
-    is_deeply(\@locks, [['nvmetest', undef]], 'cfs_lock_storage gets the timeout of the core');
+    is_deeply(\@locks, [['nvmetest', 30]], 'cfs_lock_storage waits the bounded default');
     is_deeply(
         \@allocations,
         [['nvmetest', 105, 'raw', 'vm-105-disk-0', 131_072]],