[pve-devel] applied-series: [PATCH storage 2/2] lvm thin: don't assume that a thin pool and its volumes are active

[Fabian Grünbichler]

On November 8, 2021 8:49 am, Fabian Ebner wrote:
> Am 05.11.21 um 11:29 schrieb Fabian Ebner:
>> There are cases where autoactivation can fail, as reported in the
>> community forum [0]. And it could also be that a volume was
>> deactivated by something outside of our control.
>> 
>> It doesn't seem strictly necessary to activate the thin pool itself
>> (creating/removing/activating LVs within the pool still works if it's
>> not active), but it does not report usage information as long as
>> neither the pool nor any of its LVs are active. Activate the pool for
>> that, for being able to use the flag in status(), and it should also
>> serve as a good indicator that there's a problem with the pool if it
>> can't be activated.
> 
> For the user reporting the issue, the autoactivation manages to activate 
> the _tdata _tmeta component LVs [0] and then crashes (or at least fails 
> without reverting the partial activation). But as long as the component 
> LVs are activate, neither the pool LV nor any LVs in the pool can be 
> activated. Meaning that this patch does not help for that scenario.

applied anyway, since the changes do make sense for other situations 
where volumes are not activated (anymore/automatically/..)

> 
> [0]: 
> https://forum.proxmox.com/threads/local-lvm-not-available-after-kernel-update-on-pve-7.97406/post-428298
> 
>> 
>> Before activating, check the (cached) lv_state from lvm_list_volumes.
>> It's necessary to update the cache in activate_storage, because the
>> flag is re-used in status(). Also update it for other (de)activations
>> to be more future-proof.
>> 
>> [0]: https://forum.proxmox.com/threads/local-lvm-not-available-after-kernel-update-on-pve-7.97406
>> 
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
>>   PVE/Storage/LVMPlugin.pm     |  1 +
>>   PVE/Storage/LvmThinPlugin.pm | 64 +++++++++++++++++++++++-------------
>>   2 files changed, 42 insertions(+), 23 deletions(-)
>> 
>> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
>> index 139d391..b44aeb8 100644
>> --- a/PVE/Storage/LVMPlugin.pm
>> +++ b/PVE/Storage/LVMPlugin.pm
>> @@ -173,6 +173,7 @@ sub lvm_list_volumes {
>>   
>>   	my $d = {
>>   	    lv_size => int($lv_size),
>> +	    lv_state => substr($lv_attr, 4, 1),
>>   	    lv_type => $lv_type,
>>   	};
>>   	$d->{pool_lv} = $pool_lv if $pool_lv;
>> diff --git a/PVE/Storage/LvmThinPlugin.pm b/PVE/Storage/LvmThinPlugin.pm
>> index 34e57b2..ea7b9b3 100644
>> --- a/PVE/Storage/LvmThinPlugin.pm
>> +++ b/PVE/Storage/LvmThinPlugin.pm
>> @@ -202,43 +202,61 @@ sub status {
>>   
>>       return if !$info || $info->{lv_type} ne 't' || !$info->{lv_size};
>>   
>> -    return ($info->{lv_size}, $info->{lv_size} - $info->{used}, $info->{used}, 1);
>> +    return (
>> +	$info->{lv_size},
>> +	$info->{lv_size} - $info->{used},
>> +	$info->{used},
>> +	$info->{lv_state} eq 'a' ? 1 : 0,
>> +    );
>> +}
>> +
>> +my $activate_lv = sub {
>> +    my ($vg, $lv, $cache) = @_;
>> +
>> +    my $lvs = $cache->{lvs} ||= PVE::Storage::LVMPlugin::lvm_list_volumes();
>> +
>> +    die "no such logical volume $vg/$lv" if !$lvs->{$vg} || !$lvs->{$vg}->{$lv};
>> +
>> +    return if $lvs->{$vg}->{$lv}->{lv_state} eq 'a';
>> +
>> +    run_command(['lvchange', '-ay', '-K', "$vg/$lv"], errmsg => "activating LV '$vg/$lv' failed");
>> +
>> +    $lvs->{$vg}->{$lv}->{lv_state} = 'a'; # update cache
>> +
>> +    return;
>> +};
>> +
>> +sub activate_storage {
>> +    my ($class, $storeid, $scfg, $cache) = @_;
>> +
>> +    $class->SUPER::activate_storage($storeid, $scfg, $cache);
>> +
>> +    $activate_lv->($scfg->{vgname}, $scfg->{thinpool}, $cache);
>>   }
>>   
>>   sub activate_volume {
>>       my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
>>   
>>       my $vg = $scfg->{vgname};
>> +    my $lv = $snapname ? "snap_${volname}_$snapname" : $volname;
>>   
>> -    # only snapshot volumes needs activation
>> -    if ($snapname) {
>> -	my $snapvol = "snap_${volname}_$snapname";
>> -	my $cmd = ['/sbin/lvchange', '-ay', '-K', "$vg/$snapvol"];
>> -	run_command($cmd, errmsg => "activate_volume '$vg/$snapvol' error");
>> -    } elsif ($volname =~ /^base-/) {
>> -	my $cmd = ['/sbin/lvchange', '-ay', '-K', "$vg/$volname"];
>> -	run_command($cmd, errmsg => "activate_volume '$vg/$volname' error");
>> -    } else {
>> -	# other volumes are active by default
>> -    }
>> +    $activate_lv->($vg, $lv, $cache);
>>   }
>>   
>>   sub deactivate_volume {
>>       my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
>>   
>> +    return if !$snapname && $volname !~ /^base-/; # other volumes are kept active
>> +
>>       my $vg = $scfg->{vgname};
>> +    my $lv = $snapname ? "snap_${volname}_$snapname" : $volname;
>>   
>> -    # we only deactivate snapshot volumes
>> -    if ($snapname) {
>> -	my $snapvol = "snap_${volname}_$snapname";
>> -	my $cmd = ['/sbin/lvchange', '-an', "$vg/$snapvol"];
>> -	run_command($cmd, errmsg => "deactivate_volume '$vg/$snapvol' error");
>> -    } elsif ($volname =~ /^base-/) {
>> -	my $cmd = ['/sbin/lvchange', '-an', "$vg/$volname"];
>> -	run_command($cmd, errmsg => "deactivate_volume '$vg/$volname' error");
>> -    } else {
>> -	# other volumes are kept active
>> -    }
>> +    run_command(['lvchange', '-an', "$vg/$lv"], errmsg => "deactivate_volume '$vg/$lv' error");
>> +
>> +    $cache->{lvs}->{$vg}->{$lv}->{lv_state} = '-' # update cache
>> +	if $cache->{lvs} && $cache->{lvs}->{$vg} && $cache->{lvs}->{$vg}->{$lv};
>> +
>> +    return;
>>   }
>>   
>>   sub clone_image {
>> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>