[pve-devel] applied: [PATCH storage] zpool: handle race with other zpool imports

[Fabian Grünbichler]

On Wed, Apr 17, 2019 at 03:00:20PM +0000, Thomas Lamprecht wrote:
> The underlying issue is that a zpool can get imported only once, so
> we first check if it's in `zpool list`, and thus imported, and only
> if it does not shows up there we try to import it.
> 
> But, this can race with either:
> * parallel running activate_storage call, through CLI/API/daemon
> * a zpool import from an admin (a bit unlikely, but hey that's the
>   thing with race conditions ;))
> 
> So refactor the "is pool imported" check into a closure, and call it
> addditionally if the import failed, and silent the error if the pool
> is now listed, and thus imported. This makes it a little bit nicer to
> read too, IMO.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> ---
>  PVE/Storage/ZFSPoolPlugin.pm | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index 047a06e..2e9c4c7 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -516,20 +516,23 @@ sub activate_storage {
>      my $pool = $scfg->{pool};
>      $pool =~ s!/.*$!!;
>  
> -    my @param = ('-o', 'name', '-H', "$pool");
> -    my $res;
> -    eval {
> -	$res = $class->zfs_request($scfg, undef, 'zpool_list', @param);
> -    };
> -    my $err = $@;
> -    if ($err || !defined($res) || $res !~ $pool) {
> -	eval {
> -	    @param = ('-d', '/dev/disk/by-id/', '-o', 'cachefile=none', "$pool");
> -	    $class->zfs_request($scfg, undef, 'zpool_import', @param);
> -	};
> +    my $pool_imported = sub {
> +	my @param = ('-o', 'name', '-H', "$pool");
> +	my $res = eval { $class->zfs_request($scfg, undef, 'zpool_list', @param) };
>  	if ($@) {
> -	    warn "$err\n";
> -	    die "could not activate storage '$storeid', $@\n";
> +	    warn "$@\n";
> +	    return undef;
> +	}
> +	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) };
> +	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->();
>  	}
>      }
>      return 1;
> -- 
> 2.20.1

thanks!