[pve-devel] [PATCH storage] zfs: improve get_latest_snapshot

[pve-devel] [PATCH storage] zfs: improve get_latest_snapshot

[Fabian Ebner]

by only requesting snapshots for the relevant dataset, instead of all
snapshots in all pools, and avoiding an unnecessary loop.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Storage/ZFSPoolPlugin.pm | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index c4be70f..fd7f844 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -400,19 +400,16 @@ sub zfs_get_latest_snapshot {
 
     my $vname = ($class->parse_volname($volname))[1];
 
-    # abort rollback if snapshot is not the latest
-    my @params = ('-t', 'snapshot', '-o', 'name', '-s', 'creation');
+    # can't use -S, because zfs list won't reverse order when creation time
+    # is the same second, breaking (at least) our tests.
+    my @params = ('-H', '-t', 'snapshot', '-o', 'name', '-s', 'creation', "$scfg->{pool}\/$vname");
     my $text = $class->zfs_request($scfg, undef, 'list', @params);
     my @snapshots = split(/\n/, $text);
 
     my $recentsnap;
-    foreach (@snapshots) {
-        if (/$scfg->{pool}\/$vname/) {
-            s/^.*@//;
-            $recentsnap = $_;
-        }
+    if (scalar(@snapshots) > 0) {
+	($recentsnap = $snapshots[-1]) =~ s/^.*@//; # last element
     }
-
     return $recentsnap;
 }
 
-- 
2.30.2

Re: [pve-devel] [PATCH storage] zfs: improve get_latest_snapshot

[Fabian Ebner]

Am 06.08.21 um 11:11 schrieb Fabian Ebner:
> by only requesting snapshots for the relevant dataset, instead of all
> snapshots in all pools, and avoiding an unnecessary loop.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>   PVE/Storage/ZFSPoolPlugin.pm | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index c4be70f..fd7f844 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -400,19 +400,16 @@ sub zfs_get_latest_snapshot {
>   
>       my $vname = ($class->parse_volname($volname))[1];
>   
> -    # abort rollback if snapshot is not the latest
> -    my @params = ('-t', 'snapshot', '-o', 'name', '-s', 'creation');
> +    # can't use -S, because zfs list won't reverse order when creation time
> +    # is the same second, breaking (at least) our tests.
> +    my @params = ('-H', '-t', 'snapshot', '-o', 'name', '-s', 'creation', "$scfg->{pool}\/$vname");
>       my $text = $class->zfs_request($scfg, undef, 'list', @params);
>       my @snapshots = split(/\n/, $text);
>   
>       my $recentsnap;
> -    foreach (@snapshots) {
> -        if (/$scfg->{pool}\/$vname/) {
> -            s/^.*@//;
> -            $recentsnap = $_;
> -        }
> +    if (scalar(@snapshots) > 0) {
> +	($recentsnap = $snapshots[-1]) =~ s/^.*@//; # last element
>       }
> -
>       return $recentsnap;
>   }
>   
> 

After an off-list discussion with Fabian G. about bug #3111, the plan is 
to extend volume_rollback_is_possible to inform its caller which 
snapshots are preventing a rollback. volume_rollback_is_possible is the 
only caller of zfs_get_latest_snapshot, so this patch will most likely 
be superseded by the series fixing #3111.