[pve-devel] [PATCH storage v2 0/3] zfspoolplugin: check if mounted instead of imported

[pve-devel] [PATCH storage v2 0/3] zfspoolplugin: check if mounted instead of imported

[Stoiko Ivanov]

v1->v2:
after a short talk off-list with Thomas (Thanks!), we decided to parse
/proc/mounts to check if the pool is properly imported and mounted:
* it catches a few corner cases (root-dataset having canmount set to off)
* since zfs mount -a traverses the dataset in-order - so it should not
  cause a false-detection (which would have been caught by running
  zfs get mounted on the root-dataset only)
* it is quite a bit faster
* separated the cleanup from the actual patch
* tried to structure the patches a bit more (feel free to squash 2/3 and 3/3)


original note for v1:
rfc -> v1:
* incorporated Thomas' feedback (Thanks):
** we now check both properties mounted,canmount and only run mount -a
   if the dataset is not mounted, but should be by config
** this prevents the code from constantly running zfs mount -a in case
   the system has canmount=off on the root-dataset
** however in that case, if the (child) datasets were not mounted due to
   a timeout, they will not get mounted (as before)


Stoiko Ivanov (3):
  zfspoolplugin: activate_storage: minor cleanup
  zfspoolplugin: check if mounted instead of imported
  zfspoolplugin: check if imported before importing

 PVE/Storage/ZFSPoolPlugin.pm | 49 +++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 14 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH storage v2 1/3] zfspoolplugin: activate_storage: minor cleanup

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 PVE/Storage/ZFSPoolPlugin.pm | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index 105d802..be7b1b9 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -529,22 +529,20 @@ sub activate_storage {
     $pool =~ s!/.*$!!;
 
     my $pool_imported = sub {
-	my @param = ('-o', 'name', '-H', "$pool");
+	my @param = ('-o', 'name', '-H', $pool);
 	my $res = eval { $class->zfs_request($scfg, undef, 'zpool_list', @param) };
-	if ($@) {
-	    warn "$@\n";
-	    return undef;
-	}
+	warn "$@\n" if $@;
+
 	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");
+	my @param = ('-d', '/dev/disk/by-id/', '-o', 'cachefile=none', $pool);
 	eval { $class->zfs_request($scfg, undef, 'zpool_import', @param) };
 	if (my $err = $@) {
 	    # just could've raced with another import, so recheck if it is imported
-	    die "could not activate storage '$storeid', $@\n" if !$pool_imported->();
+	    die "could not activate storage '$storeid', $err\n" if !$pool_imported->();
 	}
     }
     return 1;
-- 
2.20.1

[pve-devel] [PATCH storage v2 2/3] zfspoolplugin: check if mounted instead of imported

[Stoiko Ivanov]

This patch addresses an issue we recently saw on a production machine:
* after booting a ZFS pool failed to get imported (due to an empty
  /etc/zfs/zpool.cache)
* pvestatd/guest-startall eventually tried to import the pool
* the pool was imported, yet the datasets of the pool remained
  not mounted

A bit of debugging showed that `zpool import <poolname>` is not
atomic, in fact it does fork+exec `mount` with appropriate parameters.
If an import ran longer than the hardcoded timeout of 15s, it could
happen that the pool got imported, but the zpool command (and its
forks) got terminated due to timing out.

reproducing this is straight-forward by setting (drastic) bw+iops
limits on a guests' disk (which contains a zpool) - e.g.:
`qm set 100 -scsi1 wd:vm-100-disk-1,iops_rd=10,iops_rd_max=20,\
iops_wr=15,iops_wr_max=20,mbps_rd=10,mbps_rd_max=15,mbps_wr=10,\
mbps_wr_max=15`
afterwards running `timeout 15 zpool import <poolname>` resulted in
that situation in the guest on my machine

The patch changes the check in activate_storage for the ZFSPoolPlugin,
to check if any dataset below the 'pool' (which can also be a sub-dataset)
is mounted by parsing /proc/mounts:
* this is cheaper than running `zfs get` or `zpool list`
* it catches a properly imported and mounted pool in case the
  root-dataset has 'canmount' set to off (or noauto), as long
  as any dataset below is mounted

After trying to import the pool, we also run `zfs mount -a` (in case
another check of /proc/mounts fails).

Potential for regression:
* running `zfs mount -a` is problematic, if a dataset is manually
  umounted after booting (without setting 'canmount')
* a pool without any mounted dataset (no mountpoint property set and
  only zvols) - will result in repeated calls to `zfs mount -a`

both of the above seem unlikely and should not occur, if using our
tooling.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 PVE/Storage/ZFSPoolPlugin.pm | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index be7b1b9..a524515 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -7,6 +7,7 @@ use IO::File;
 use Net::IP;
 use POSIX;
 
+use PVE::ProcFSTools;
 use PVE::RPCEnvironment;
 use PVE::Storage::Plugin;
 use PVE::Tools qw(run_command);
@@ -525,8 +526,26 @@ sub activate_storage {
     my ($class, $storeid, $scfg, $cache) = @_;
 
     # Note: $scfg->{pool} can include dataset <pool>/<dataset>
-    my $pool = $scfg->{pool};
-    $pool =~ s!/.*$!!;
+    my $dataset = $scfg->{pool};
+    my $pool = ($dataset =~ s!/.*$!!r);
+
+    my $dataset_mounted = sub {
+	my $mounted = 0;
+	my $dataset_dec = PVE::ProcFSTools::decode_mount($dataset);
+	my $mounts = eval { PVE::ProcFSTools::parse_proc_mounts() };
+	warn "$@\n" if $@;
+	foreach my $mp (@$mounts) {
+	    my ($what, $dir, $fs) = @$mp;
+	    next if $fs ne 'zfs';
+	    # check for root-dataset of storage or any child-dataset.
+	    # root-dataset could have 'canmount=off'. If any child is mounted
+	    # heuristically assume that `zfs mount -a` was successful
+	    next if $what !~ m!^$dataset_dec(?:/|$)!;
+	    $mounted = 1;
+	    last;
+	}
+	return $mounted;
+    };
 
     my $pool_imported = sub {
 	my @param = ('-o', 'name', '-H', $pool);
@@ -536,7 +555,7 @@ sub activate_storage {
 	return defined($res) && $res =~ m/$pool/;
     };
 
-    if (!$pool_imported->()) {
+    if (!$dataset_mounted->()) {
 	# 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) };
@@ -544,6 +563,8 @@ sub activate_storage {
 	    # just could've raced with another import, so recheck if it is imported
 	    die "could not activate storage '$storeid', $err\n" if !$pool_imported->();
 	}
+	eval { $class->zfs_request($scfg, undef, 'mount', '-a') };
+	die "could not activate storage '$storeid', $@\n" if $@;
     }
     return 1;
 }
-- 
2.20.1

[pve-devel] [PATCH storage v2 3/3] zfspoolplugin: check if imported before importing

[Stoiko Ivanov]

This commit is a small performance optimization to the previous one:
`zpool list` is cheaper than `zpool import -d /dev..` (the latter
scans the disks in the provided directory for zfs signatures,
unconditionally)

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 PVE/Storage/ZFSPoolPlugin.pm | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index a524515..4806ba5 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -556,12 +556,14 @@ sub activate_storage {
     };
 
     if (!$dataset_mounted->()) {
-	# 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 = $@) {
-	    # just could've raced with another import, so recheck if it is imported
-	    die "could not activate storage '$storeid', $err\n" if !$pool_imported->();
+	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) };
+	    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->();
+	    }
 	}
 	eval { $class->zfs_request($scfg, undef, 'mount', '-a') };
 	die "could not activate storage '$storeid', $@\n" if $@;
-- 
2.20.1

[pve-devel] applied: [PATCH storage v2 1/3] zfspoolplugin: activate_storage: minor cleanup

[Thomas Lamprecht]

On 19.02.21 13:45, Stoiko Ivanov wrote:
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  PVE/Storage/ZFSPoolPlugin.pm | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH storage v2 2/3] zfspoolplugin: check if mounted instead of imported

[Thomas Lamprecht]

On 19.02.21 13:45, Stoiko Ivanov wrote:
> This patch addresses an issue we recently saw on a production machine:
> * after booting a ZFS pool failed to get imported (due to an empty
>   /etc/zfs/zpool.cache)
> * pvestatd/guest-startall eventually tried to import the pool
> * the pool was imported, yet the datasets of the pool remained
>   not mounted
> 
> A bit of debugging showed that `zpool import <poolname>` is not
> atomic, in fact it does fork+exec `mount` with appropriate parameters.
> If an import ran longer than the hardcoded timeout of 15s, it could
> happen that the pool got imported, but the zpool command (and its
> forks) got terminated due to timing out.
> 
> reproducing this is straight-forward by setting (drastic) bw+iops
> limits on a guests' disk (which contains a zpool) - e.g.:
> `qm set 100 -scsi1 wd:vm-100-disk-1,iops_rd=10,iops_rd_max=20,\
> iops_wr=15,iops_wr_max=20,mbps_rd=10,mbps_rd_max=15,mbps_wr=10,\
> mbps_wr_max=15`
> afterwards running `timeout 15 zpool import <poolname>` resulted in
> that situation in the guest on my machine
> 
> The patch changes the check in activate_storage for the ZFSPoolPlugin,
> to check if any dataset below the 'pool' (which can also be a sub-dataset)
> is mounted by parsing /proc/mounts:
> * this is cheaper than running `zfs get` or `zpool list`
> * it catches a properly imported and mounted pool in case the
>   root-dataset has 'canmount' set to off (or noauto), as long
>   as any dataset below is mounted
> 
> After trying to import the pool, we also run `zfs mount -a` (in case
> another check of /proc/mounts fails).
> 
> Potential for regression:
> * running `zfs mount -a` is problematic, if a dataset is manually
>   umounted after booting (without setting 'canmount')
> * a pool without any mounted dataset (no mountpoint property set and
>   only zvols) - will result in repeated calls to `zfs mount -a`
> 
> both of the above seem unlikely and should not occur, if using our
> tooling.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  PVE/Storage/ZFSPoolPlugin.pm | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH storage v2 3/3] zfspoolplugin: check if imported before importing

[Thomas Lamprecht]

On 19.02.21 13:45, Stoiko Ivanov wrote:
> This commit is a small performance optimization to the previous one:
> `zpool list` is cheaper than `zpool import -d /dev..` (the latter
> scans the disks in the provided directory for zfs signatures,
> unconditionally)
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  PVE/Storage/ZFSPoolPlugin.pm | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
>

applied, with the cleanups we talked off-list about, thanks!