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.eb...@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@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
