Skip to content

Commit

Permalink
perf(syncoid): Regression: Bad performance due to zfs get all
Browse files Browse the repository at this point in the history
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:
#818 (comment)
  • Loading branch information
Deltik committed Jun 13, 2024
1 parent 3a5f84c commit 8a023d6
Showing 1 changed file with 64 additions and 18 deletions.
82 changes: 64 additions & 18 deletions syncoid
Original file line number Diff line number Diff line change
Expand Up @@ -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 %rhost_zfs_get_features;

my %snaps;
my $exitcode = 0;

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -1393,6 +1396,49 @@ sub checkcommands {
return %avail;
}

sub check_zfs_get_features {
my ($rhost, $mysudocmd, $zfscmd) = @_;

return if exists $rhost_zfs_get_features{$rhost};

my $host = $rhost ? (split(/\s+/, $rhost))[-1] : "localhost";

writelog('DEBUG', "Checking `zfs get` features on host \"$host\"...");

$rhost_zfs_get_features{$rhost} = {
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/) {
$rhost_zfs_get_features{$rhost}->{supports_type_filter} = 1;
}

writelog('DEBUG', "Host \"$host\" has `zfs get -t`?: $rhost_zfs_get_features{$rhost}->{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 @{$rhost_zfs_get_features{$rhost}->{supported_properties}}, $prop;
}
}

writelog('DEBUG', "Host \"$host\" ZFS properties: @{$rhost_zfs_get_features{$rhost}->{supported_properties}}");

return $rhost_zfs_get_features{$rhost};
}


sub iszfsbusy {
my ($rhost,$fs,$isroot) = @_;
if ($rhost ne '') { $rhost = "$sshcmd $rhost"; }
Expand Down Expand Up @@ -1869,22 +1915,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";
check_zfs_get_features($rhost, $mysudocmd, $zfscmd);

my @properties = @{$rhost_zfs_get_features{$rhost}->{supported_properties}};
my $type_filter = "";
if ($rhost_zfs_get_features{$rhost}->{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 |";
Expand All @@ -1894,13 +1946,7 @@ sub getsnaps {
}
open FH, $getsnapcmd;
my @rawsnaps = <FH>;
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;
Expand Down Expand Up @@ -1938,7 +1984,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'};
Expand Down

0 comments on commit 8a023d6

Please sign in to comment.