From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 137C46B943 for ; Fri, 19 Feb 2021 09:39:49 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EF91216C75 for ; Fri, 19 Feb 2021 09:39:18 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 39C5E16C67 for ; Fri, 19 Feb 2021 09:39:18 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C1C6641ACB for ; Fri, 19 Feb 2021 09:39:17 +0100 (CET) Message-ID: <22c86411-c687-f81a-78d1-67779ec00cc2@proxmox.com> Date: Fri, 19 Feb 2021 09:39:16 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:86.0) Gecko/20100101 Thunderbird/86.0 Content-Language: en-US To: Proxmox VE development discussion , Stoiko Ivanov References: <20210218163635.28639-1-s.ivanov@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210218163635.28639-1-s.ivanov@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.059 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [zfspoolplugin.pm] Subject: Re: [pve-devel] [PATCH storage] zfspoolplugin: check if mounted instead of imported X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Feb 2021 08:39:49 -0000 On 18.02.21 17:36, 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 ` 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 ` resulted in > that situation in the guest on my machine > > The patch changes the check in activate_storage for the ZFSPoolPlugin, > to check the 'mounted' and 'canmount' properties of the configured > 'pool' (which can also be a sub-dataset): > * if the zfs get fails, we try to import the pool (and run `zfs > mount -a` if the dataset is not mounted after importing) > * if canmount != 'yes' we assume everything is ok > * if it reports the dataset as not mounted, we run `zfs mount -a` > > The choice of running `zfs mount -a` has potential for regression, in > case someone manually umounts some dataset after booting the system > (but does not set the canmount option). > > Signed-off-by: Stoiko Ivanov > --- > 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) > > PVE/Storage/ZFSPoolPlugin.pm | 38 ++++++++++++++++++++++++++++-------- > 1 file changed, 30 insertions(+), 8 deletions(-) > > diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm > index 105d802..3d25e6a 100644 > --- a/PVE/Storage/ZFSPoolPlugin.pm > +++ b/PVE/Storage/ZFSPoolPlugin.pm > @@ -525,27 +525,49 @@ sub activate_storage { > my ($class, $storeid, $scfg, $cache) = @_; > > # Note: $scfg->{pool} can include dataset / > - my $pool = $scfg->{pool}; > - $pool =~ s!/.*$!!; > + my $dataset = $scfg->{pool}; > + my $pool = ($dataset =~ s!/.*$!!r); > > - my $pool_imported = sub { > - my @param = ('-o', 'name', '-H', "$pool"); > - my $res = eval { $class->zfs_request($scfg, undef, 'zpool_list', @param) }; > + my $dataset_mounted = sub { > + my ($mounted, $canmount) = eval { > + $class->zfs_get_properties($scfg, 'mounted,canmount', $dataset) > + }; > if ($@) { > warn "$@\n"; > return undef; > } > + #canmount is off|noauto, zfs get worked - assume pool is correctly imported but didn't we learn that imported does not mean that it is necessarily mounted??? Shouldn't a method named "dataset_mounted" observer that difference? > + return 'yes' if $canmount =~ /^(?:off|noauto)$/; > + return $mounted; why not return 1 and 0, or 1 and undef and just stick to if ($mounted) below? For such a simple thing this seems doing it a bit complicated, IMO > + }; > + > + my $mount_all = sub { > + eval { $class->zfs_request($scfg, undef, 'mount', '-a') }; > + die "could not activate storage '$storeid', $@\n" if $@; > + }; > + > + my $pool_imported = sub { > + my @param = ('-o', 'name', '-H', "$pool"); > + my $res = eval { $class->zfs_request($scfg, undef, 'zpool_list', @param) }; > + warn "$@\n" if $@; > + that pool_imported refactoring is unrelated and should be in its own patch > return defined($res) && $res =~ m/$pool/; > }; > > - if (!$pool_imported->()) { > + my $mounted = $dataset_mounted->(); > + if (!$mounted) { > + # error from zfs get mounted - pool import necessary > # 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); unrelated to patch, so should be in own > 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->(); forgot to mention in v1, but that should be done in an preparatory patch You can bundle a few cleanups together, but cleanups and the actual meat of a rather important patch fixing an issue we had off and one for years should be separated. > } > + $mount_all->() if !$dataset_mounted->(); > + } elsif ($mounted =~ /^no$/) { if we go for this, then use `$mounted eq 'no'` > + # best effort mount everything > + $mount_all->(); > } > return 1; > } >