[pve-devel] [PATCH manager v2 1/2] Fix #2124: Add support for zstd

[Alwin Antreich]

On Mon, Feb 03, 2020 at 05:51:38PM +0100, Stefan Reiter wrote:
> On 1/31/20 5:00 PM, Alwin Antreich wrote:
> > Adds the zstd to the compression selection for backup on the GUI and the
> > .zst extension to the backup file filter.
> > 
> > Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> > ---
> > 
> >   PVE/VZDump.pm                            | 6 ++++--
> >   www/manager6/form/CompressionSelector.js | 3 ++-
> >   2 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> > index 3caa7ab8..21032fd6 100644
> > --- a/PVE/VZDump.pm
> > +++ b/PVE/VZDump.pm
> > @@ -592,6 +592,8 @@ sub compressor_info {
> >   	} else {
> >   	    return ('gzip --rsyncable', 'gz');
> >   	}
> > +    } elsif ($opt_compress eq 'zstd') {
> > +	return ('zstd', 'zst');
> 
> Did some testing, two things I noticed, first one regarding this patch
> especially:
> 
> 1) By default zstd uses only one core. I feel like this should be increased
> (or made configurable as with pigz?). Also, zstd has an '--rsyncable flag'
> like gzip, might be good to include that too (according to the man page it
> only has a 'negligible impact on compression ratio').
Thanks for spotting, I put this into my v3.

> 
> 2) The task log is somewhat messed up... It seems zstd prints a status as
> well, additionally to our own progress meter:
True, I will silence the output. In turn, this makes it also similar to
the lzo,gzip compression output.

> 
> 
> _03-17_05_09.vma.zst : 13625 MB...     progress 94% (read 32298172416 bytes,
> duration 34 sec)
> 
> _03-17_05_09.vma.zst : 13668 MB...
> _03-17_05_09.vma.zst : 13721 MB...
> _03-17_05_09.vma.zst : 13766 MB...
> _03-17_05_09.vma.zst : 13821 MB...
> _03-17_05_09.vma.zst : 13869 MB...
> _03-17_05_09.vma.zst : 13933 MB...     progress 95% (read 32641777664 bytes,
> duration 35 sec)
> 
> _03-17_05_09.vma.zst : 14014 MB...
> _03-17_05_09.vma.zst : 14091 MB...
> 
> 
> Looks a bit unsightly IMO.
> 
> But functionality wise it works fine, tried with a VM and a container, so
> 
> Tested-by: Stefan Reiter <s.reiter at proxmox.com>
Thanks for testing.

> 
> for the series.
> 
> >       } else {
> >   	die "internal error - unknown compression option '$opt_compress'";
> >       }
> > @@ -603,7 +605,7 @@ sub get_backup_file_list {
> >       my $bklist = [];
> >       foreach my $fn (<$dir/${bkname}-*>) {
> >   	next if $exclude_fn && $fn eq $exclude_fn;
> > -	if ($fn =~ m!/(${bkname}-(\d{4})_(\d{2})_(\d{2})-(\d{2})_(\d{2})_(\d{2})\.(tgz|((tar|vma)(\.(gz|lzo))?)))$!) {
> > +	if ($fn =~ m!/(${bkname}-(\d{4})_(\d{2})_(\d{2})-(\d{2})_(\d{2})_(\d{2})\.(tgz|((tar|vma)(\.(gz|lzo|zst))?)))$!) {
> >   	    $fn = "$dir/$1"; # untaint
> >   	    my $t = timelocal ($7, $6, $5, $4, $3 - 1, $2);
> >   	    push @$bklist, [$fn, $t];
> > @@ -863,7 +865,7 @@ sub exec_backup_task {
> >   		debugmsg ('info', "delete old backup '$d->[0]'", $logfd);
> >   		unlink $d->[0];
> >   		my $logfn = $d->[0];
> > -		$logfn =~ s/\.(tgz|((tar|vma)(\.(gz|lzo))?))$/\.log/;
> > +		$logfn =~ s/\.(tgz|((tar|vma)(\.(gz|lzo|zst))?))$/\.log/;
> >   		unlink $logfn;
> >   	    }
> >   	}
> > diff --git a/www/manager6/form/CompressionSelector.js b/www/manager6/form/CompressionSelector.js
> > index 8938fc0e..e8775e71 100644
> > --- a/www/manager6/form/CompressionSelector.js
> > +++ b/www/manager6/form/CompressionSelector.js
> > @@ -4,6 +4,7 @@ Ext.define('PVE.form.CompressionSelector', {
> >       comboItems: [
> >                   ['0', Proxmox.Utils.noneText],
> >                   ['lzo', 'LZO (' + gettext('fast') + ')'],
> > -                ['gzip', 'GZIP (' + gettext('good') + ')']
> > +                ['gzip', 'GZIP (' + gettext('good') + ')'],
> > +                ['zstd', 'ZSTD (' + gettext('better') + ')']
> >       ]
> >   });
> >