[PATCH storage] fix #8230: zfspool: serialize import and mount in activate_storage

[PATCH storage] fix #8230: zfspool: serialize import and mount in activate_storage

[Carsten Haerle]

activate_storage() imports a pool on demand and then runs 'zfs mount
-a'. Neither step is serialized against other callers on the same node.
At boot, pvestatd and pve-guests (startall) call it for the same storage
within about a second of each other, which fails in two ways:

Import: while a 'zpool import' is running, the pool is neither listed
by 'zpool list' nor offered by 'zpool import', so the second caller
gets "cannot import '<pool>': no such pool available". The existing
re-check only covers imports that finish within milliseconds. For pools
that take seconds (many datasets, dedup tables) the onboot guest start
fails and startall does not retry.

Mount: both callers then run 'zfs mount -a' and collide on the same
datasets ("mountpoint or dataset is busy"). 'zfs mount -a' exits
non-zero if any dataset fails, so the worker dies even though the
storage's own dataset is mounted. Datasets unrelated to the storage on
the same pool cause the same failure.

Observed with a raidz2 pool whose import takes ~40 s:

  54 s  pvestatd: zpool import data1 (runs until 93 s)
  55 s  pve-guests: Starting VM 124
  58 s  Starting VM 124 failed: could not activate storage 'vmdata1zfs',
        zfs error: cannot import 'data1': no such pool available

Serialize check, import and mount per pool with a node-local lock. A
caller that obtains the lock after another importer re-checks first and
does not import again. Every caller tries the lock for one second
first. If it is busy, non-workers (pvestatd, API requests) fail with an
explicit "import of pool '<pool>' in progress", so pvestatd reports the
storage as inactive for one round instead of blocking. Workers (guest
start, backup, migration) log that they are waiting and then wait for
the lock with a long timeout. A failed 'zfs mount -a' is only fatal if
the storage's dataset is not mounted afterwards, using the same
heuristic the function already uses for its early return.

While at it, log which caller imports the pool ("importing ZFS pool
'<pool>' for storage '<storage>'") and drop the warning that the
re-check emitted for the expected "no such pool" case, so the task log
of a guest start shows what actually happened.

Tested on PVE 9.2.21 / libpve-storage-perl 9.1.13 with 12 reboots on
four nodes: the onboot guests started every time, in both orders
(pvestatd imports and the worker waits at the lock; the worker imports
and pvestatd reports "in progress").

Signed-off-by: Carsten Haerle <carsten.haerle@straightec.de>
---
 src/PVE/Storage/ZFSPoolPlugin.pm | 66 +++++++++++++++++++++++++++-----
 1 file changed, 57 insertions(+), 9 deletions(-)

diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
index e7cd1de..0cb2d1e 100644
--- a/src/PVE/Storage/ZFSPoolPlugin.pm
+++ b/src/PVE/Storage/ZFSPoolPlugin.pm
@@ -713,22 +713,70 @@ sub activate_storage {
     my $pool_imported = sub {
         my @param = ('-o', 'name', '-H', $pool);
         my $res = eval { $class->zfs_request($scfg, undef, 'zpool_list', @param) };
-        warn "$@\n" if $@;
+        # a not yet imported pool is the expected case here, not worth a warning
+        warn "$@\n" if $@ && $@ !~ m/no such pool/;
 
         return defined($res) && $res =~ m/$pool/;
     };
 
-    if (!$pool_imported->()) {
-        # import can only be done if not yet imported!
-        my @param = ('-d', '/dev/disk/by-id/', '-o', 'cachefile=none', $pool);
-        eval { $class->zfs_request($scfg, undef, 'zpool_import', @param) };
+    my $activate = sub {
+        # whoever held the lock before us may have done the work already
+        return 1 if dataset_mounted_heuristic($dataset);
+
+        if (!$pool_imported->()) {
+            print STDERR "importing ZFS pool '$pool' for storage '$storeid'\n";
+            # import can only be done if not yet imported!
+            my @param = ('-d', '/dev/disk/by-id/', '-o', 'cachefile=none', $pool);
+            eval { $class->zfs_request($scfg, undef, 'zpool_import', @param) };
+            if (my $err = $@) {
+                # an import outside of PVE (manual, zfs-import units) can still race
+                die "could not activate storage '$storeid', $err\n" if !$pool_imported->();
+            }
+        }
+
+        # 'zfs mount -a' covers all datasets of all pools. Datasets unrelated to
+        # this storage may fail to mount (e.g. "mountpoint or dataset is busy"
+        # because another caller is mounting, or a previous run was killed by
+        # its timeout). That must not fail the activation as long as our
+        # dataset is mounted.
+        eval { $class->zfs_request($scfg, undef, 'mount', '-a') };
         if (my $err = $@) {
-            # just could've raced with another import, so recheck if it is imported
-            die "could not activate storage '$storeid', $err\n" if !$pool_imported->();
+            die "could not activate storage '$storeid', $err\n"
+                if !dataset_mounted_heuristic($dataset);
+            warn "storage '$storeid': mounting all ZFS datasets failed, $err";
+        }
+        return 1;
+    };
+
+    # Serialize import and mount of the same pool on this node. While a
+    # 'zpool import' is running, the pool is neither listed by 'zpool list'
+    # nor offered by 'zpool import', so a concurrent caller fails with
+    # "no such pool available". The import can take a long time (large pools,
+    # dedup tables), so the immediate re-check alone is not sufficient.
+    # Workers (guest start, backup, migration) wait for the import to finish,
+    # non-workers (pvestatd, API requests) only wait briefly and report the
+    # storage as not yet active.
+    my $lockfile = "/run/lock/pve-zpool-import-$pool.lck";
+    my $lock_busy = sub { return $_[0] =~ m/can't lock file .* got timeout/ };
+
+    # try briefly first, so that a worker can tell what it is waiting for
+    my $res = PVE::Tools::lock_file($lockfile, 1, $activate);
+    if (!$res) {
+        my $err = $@;
+        die $err if !$lock_busy->($err);
+        die "could not activate storage '$storeid', import of pool '$pool' in progress\n"
+            if !PVE::RPCEnvironment->is_worker();
+
+        print STDERR "pool '$pool' is being imported or mounted by another process, waiting...\n";
+        $res = PVE::Tools::lock_file($lockfile, 60 * 10, $activate);
+        if (!$res) {
+            $err = $@;
+            die "could not activate storage '$storeid', "
+                . "timeout waiting for import of pool '$pool'\n"
+                if $lock_busy->($err);
+            die $err;
         }
     }
-    eval { $class->zfs_request($scfg, undef, 'mount', '-a') };
-    die "could not activate storage '$storeid', $@\n" if $@;
     return 1;
 }
 
-- 
2.53.0