From f16c15c9baf757cc481e3733874d554d6b901e5c Mon Sep 17 00:00:00 2001 From: Nick Liu Date: Wed, 12 Jun 2024 20:02:49 -0500 Subject: [PATCH 1/3] fix(syncoid): Regression: Fallback recursion `$rhost` mutated already MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An oversight during the merging of subroutine `getsnapsfallback` into `getsnaps` (https://github.com/jimsalterjrs/sanoid/pull/818/commits/e301b5b1) caused the recursive call to prepend `$sshcmd` to `$rhost` again, leading to this error: ```shell # syncoid --debug root@192.168.122.26:rpool/before syncoid-test-11/after … [TRUNCATED] … WARNING: snapshot listing failed, trying fallback command DEBUG: getting list of snapshots on rpool/before using ssh ssh -S /tmp/syncoid-root19216812226-1718239169-761069-2198 root@192.168.122.26 zfs get -Hpd 1 all ''"'"'rpool/before'"'"'' |... root@192.168.122.26: Command not found. CRITICAL ERROR: snapshots couldn't be listed for rpool/before (exit code 256) at /usr/sbin/syncoid line 1900. ``` Fixes: https://github.com/jimsalterjrs/sanoid/pull/818#issuecomment-2164155867 --- syncoid | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/syncoid b/syncoid index 79ad45f..bb773b6 100755 --- a/syncoid +++ b/syncoid @@ -1874,6 +1874,8 @@ sub getsnaps { my $fsescaped = escapeshellparam($fs); if ($isroot) { $mysudocmd = ''; } else { $mysudocmd = $sudocmd; } + my $rhostOriginal = $rhost; + if ($rhost ne '') { $rhost = "$sshcmd $rhost"; # double escaping needed @@ -1895,7 +1897,7 @@ sub getsnaps { close FH or do { if (!$use_fallback) { writelog('WARN', "snapshot listing failed, trying fallback command"); - return getsnaps($type, $rhost, $fs, $isroot, 1, %snaps); + return getsnaps($type, $rhostOriginal, $fs, $isroot, 1, %snaps); } die "CRITICAL ERROR: snapshots couldn't be listed for $fs (exit code $?)"; }; From a2adaf849946ecc7329784531b56d46ff13460ce Mon Sep 17 00:00:00 2001 From: Nick Liu Date: Wed, 12 Jun 2024 20:05:47 -0500 Subject: [PATCH 2/3] fix(syncoid): Regression: `zfs get` parser not compatible with fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the fallback mode, non-snapshot datasets would be included in the output, leading to this parsing error: ```shell # syncoid --debug root@192.168.122.26:rpool/before syncoid-test-11/after … [TRUNCATED] … WARNING: snapshot listing failed, trying fallback command DEBUG: getting list of snapshots on rpool/before using ssh -S /tmp/syncoid-root19216812226-1718239740-763496-6973 root@192.168.122.26 zfs get -Hpd 1 all ''"'"'rpool/before'"'"'' |... CRITICAL ERROR: Unexpected dataset format in rpool/before type filesystem - at /usr/sbin/syncoid line 1914. ``` Fixes: https://github.com/jimsalterjrs/sanoid/pull/818#issuecomment-2164155867 --- syncoid | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/syncoid b/syncoid index bb773b6..b6946d9 100755 --- a/syncoid +++ b/syncoid @@ -1911,7 +1911,7 @@ sub getsnaps { die "CRITICAL ERROR: Unexpected line format in $line" unless defined $value; my (undef, $snap) = split /@/, $dataset; - die "CRITICAL ERROR: Unexpected dataset format in $line" unless $snap; + next unless length $snap; if (!snapisincluded($snap)) { next; } From 191ddfe600a2df9819e5c3db589ec9f95134ff79 Mon Sep 17 00:00:00 2001 From: Nick Liu Date: Wed, 12 Jun 2024 20:10:20 -0500 Subject: [PATCH 3/3] perf(syncoid): Regression: Bad performance due to `zfs get all` Return to using `zfs get guid,creation,createtxg` to avoid the performance overhead of `zfs get all`. There is a per-host global cache to remember: * if `zfs get` supports the type filter argument, `-t snapshot` and * which ZFS properties are available. The feature check introduced by subroutine `check_zfs_get_features` eliminates the need for a `getsnaps` fallback recursion, as there is no longer any guesswork into what features `zfs get` supports. Fixes: https://github.com/jimsalterjrs/sanoid/pull/818#issuecomment-2157567968 --- syncoid | 81 ++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 63 insertions(+), 18 deletions(-) diff --git a/syncoid b/syncoid index b6946d9..67373c6 100755 --- a/syncoid +++ b/syncoid @@ -194,6 +194,9 @@ if (length $args{'insecure-direct-connection'}) { # warn user of anything missing, then continue with sync. my %avail = checkcommands(); +# host => { supports_type_filter => 1/0, supported_properties => ['guid', 'creation', ...] } +my %host_zfs_get_features; + my %snaps; my $exitcode = 0; @@ -415,10 +418,10 @@ sub syncdataset { if (!defined($receivetoken)) { # build hashes of the snaps on the source and target filesystems. - %snaps = getsnaps('source',$sourcehost,$sourcefs,$sourceisroot,0); + %snaps = getsnaps('source',$sourcehost,$sourcefs,$sourceisroot); if ($targetexists) { - my %targetsnaps = getsnaps('target',$targethost,$targetfs,$targetisroot,0); + my %targetsnaps = getsnaps('target',$targethost,$targetfs,$targetisroot); my %sourcesnaps = %snaps; %snaps = (%sourcesnaps, %targetsnaps); } @@ -858,10 +861,10 @@ sub syncdataset { # snapshots first. # regather snapshots on source and target - %snaps = getsnaps('source',$sourcehost,$sourcefs,$sourceisroot,0); + %snaps = getsnaps('source',$sourcehost,$sourcefs,$sourceisroot); if ($targetexists) { - my %targetsnaps = getsnaps('target',$targethost,$targetfs,$targetisroot,0); + my %targetsnaps = getsnaps('target',$targethost,$targetfs,$targetisroot); my %sourcesnaps = %snaps; %snaps = (%sourcesnaps, %targetsnaps); } @@ -1393,6 +1396,48 @@ sub checkcommands { return %avail; } +sub check_zfs_get_features { + my ($rhost, $mysudocmd, $zfscmd) = @_; + my $host = $rhost ? (split(/\s+/, $rhost))[-1] : "localhost"; + + return $host_zfs_get_features{$host} if exists $host_zfs_get_features{$host}; + + writelog('DEBUG', "Checking `zfs get` features on host \"$host\"..."); + + $host_zfs_get_features{$host} = { + supports_type_filter => 0, + supported_properties => [] + }; + + my $check_t_option_cmd = "$rhost $mysudocmd $zfscmd get -H -t snapshot '' ''"; + open my $fh_t, "$check_t_option_cmd 2>&1 |"; + my $output_t = <$fh_t>; + close $fh_t; + + if ($output_t !~ /^\Qinvalid option\E/) { + $host_zfs_get_features{$host}->{supports_type_filter} = 1; + } + + writelog('DEBUG', "Host \"$host\" has `zfs get -t`?: $host_zfs_get_features{$host}->{supports_type_filter}"); + + my @properties_to_check = ('guid', 'creation', 'createtxg'); + foreach my $prop (@properties_to_check) { + my $check_prop_cmd = "$rhost $mysudocmd $zfscmd get -H $prop ''"; + open my $fh_p, "$check_prop_cmd 2>&1 |"; + my $output_p = <$fh_p>; + close $fh_p; + + if ($output_p !~ /^\Qbad property list: invalid property\E/) { + push @{$host_zfs_get_features{$host}->{supported_properties}}, $prop; + } + } + + writelog('DEBUG', "Host \"$host\" ZFS properties: @{$host_zfs_get_features{$host}->{supported_properties}}"); + + return $host_zfs_get_features{$host}; +} + + sub iszfsbusy { my ($rhost,$fs,$isroot) = @_; if ($rhost ne '') { $rhost = "$sshcmd $rhost"; } @@ -1869,22 +1914,28 @@ sub dumphash() { } sub getsnaps { - my ($type,$rhost,$fs,$isroot,$use_fallback,%snaps) = @_; + my ($type,$rhost,$fs,$isroot,%snaps) = @_; my $mysudocmd; my $fsescaped = escapeshellparam($fs); if ($isroot) { $mysudocmd = ''; } else { $mysudocmd = $sudocmd; } - my $rhostOriginal = $rhost; - if ($rhost ne '') { $rhost = "$sshcmd $rhost"; # double escaping needed $fsescaped = escapeshellparam($fsescaped); } - my $getsnapcmd = $use_fallback - ? "$rhost $mysudocmd $zfscmd get -Hpd 1 all $fsescaped" - : "$rhost $mysudocmd $zfscmd get -Hpd 1 -t snapshot all $fsescaped"; + my $host_features = check_zfs_get_features($rhost, $mysudocmd, $zfscmd); + + my @properties = @{$host_features->{supported_properties}}; + my $type_filter = ""; + if ($host_features->{supports_type_filter}) { + $type_filter = "-t snapshot"; + } else { + push @properties, 'type'; + } + my $properties_string = join(',', @properties); + my $getsnapcmd = "$rhost $mysudocmd $zfscmd get -Hpd 1 $type_filter $properties_string $fsescaped"; if ($debug) { $getsnapcmd = "$getsnapcmd |"; @@ -1894,13 +1945,7 @@ sub getsnaps { } open FH, $getsnapcmd; my @rawsnaps = ; - close FH or do { - if (!$use_fallback) { - writelog('WARN', "snapshot listing failed, trying fallback command"); - return getsnaps($type, $rhostOriginal, $fs, $isroot, 1, %snaps); - } - die "CRITICAL ERROR: snapshots couldn't be listed for $fs (exit code $?)"; - }; + close FH or die "CRITICAL ERROR: snapshots couldn't be listed for $fs (exit code $?)"; my %snap_data; my %creationtimes; @@ -1938,7 +1983,7 @@ sub getsnaps { } for my $snap (keys %snap_data) { - if (!$use_fallback || $snap_data{$snap}{'type'} eq 'snapshot') { + if (length $type_filter || $snap_data{$snap}{'type'} eq 'snapshot') { $snaps{$type}{$snap}{'guid'} = $snap_data{$snap}{'guid'}; $snaps{$type}{$snap}{'createtxg'} = $snap_data{$snap}{'createtxg'}; $snaps{$type}{$snap}{'creation'} = $snap_data{$snap}{'creation'};