Re: [pve-devel] [PATCH v2 storage 1/5] add BTRFS storage plugin

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Wed, Jun 23, 2021 at 02:15:00PM +0200, Fabian Grünbichler wrote:
> On June 22, 2021 2:18 pm, Wolfgang Bumiller wrote:
(...)
> > +sub plugindata {
> > +    return {
> > +	content => [
> > +	    {
> > +		images => 1,
> > +		rootdir => 1,
> > +		vztmpl => 1,
> > +		iso => 1,
> > +		backup => 1,
> > +		snippets => 1,
> > +		none => 1,
> > +	    },
> > +	    { images => 1, rootdir => 1 },
> > +	],
> > +	format => [ { raw => 1, qcow2 => 1, vmdk => 1, subvol => 1 }, 'raw', ],
> 
> I am not really convinced we need this (or rather, that it doesn't 
> cause more confusion than convenience. if I want to use btrfs without 
> any btrfs features, I can already use any btrfs-backed dir as dir 
> storage (just like with ZFS, or $storage-of-my-choice-providing-a-directory).
> 
> e.g., having a qcow2 file on btrfs storage support snapshots but not use 
> btrfs functionality for that seems... weird?

It's a directory storage after all.
But I really don't mind much if we just remove that...

> 
> > +    };
> > +}
> > +
> > +sub options {
> > +    return {
> > +	path => { fixed => 1 },
> > +	nodes => { optional => 1 },
> > +	shared => { optional => 1 },
> > +	disable => { optional => 1 },
> > +	maxfiles => { optional => 1 },
> > +	content => { optional => 1 },
> > +	format => { optional => 1 },
> > +	is_mountpoint => { optional => 1 },
> > +	# TODO: The new variant of mkdir with  `populate` vs `create`...
> > +    };
> > +}
> > +
> > +# Storage implementation
> > +#
> > +# We use the same volume names are directory plugins, but map *raw* disk image file names into a
> > +# subdirectory.
> > +#
> > +# `vm-VMID-disk-ID.raw`
> > +#   -> `images/VMID/vm-VMID-disk-ID/disk.raw`
> > +#   where the `vm-VMID-disk-ID/` subdirectory is a btrfs subvolume
> > +
> > +# Reuse `DirPlugin`'s `check_config`. This simply checks for invalid paths.
> > +sub check_config {
> > +    my ($self, $sectionId, $config, $create, $skipSchemaCheck) = @_;
> > +    return PVE::Storage::DirPlugin::check_config($self, $sectionId, $config, $create, $skipSchemaCheck);
> > +}
> > +
> > +sub activate_storage {
> > +    my ($class, $storeid, $scfg, $cache) = @_;
> > +    return PVE::Storage::DirPlugin::activate_storage($class, $storeid, $scfg, $cache);
> 
> shouldn't this check that we can actually do btrfs stuff on $path?

Right. I had a method for this at some point but hadn't used it here.
Should be easily added in a follow up.

> 
> > +}
> > +
> > +sub status {
> > +    my ($class, $storeid, $scfg, $cache) = @_;
> > +    return PVE::Storage::DirPlugin::status($class, $storeid, $scfg, $cache);
> > +}
> > +
> > +# TODO: sub get_volume_notes {}
> > +
> > +# TODO: sub update_volume_notes {}
> > +
> > +# croak would not include the caller from within this module
> > +sub __error {
> > +    my ($msg) = @_;
> > +    my (undef, $f, $n) = caller(1);
> > +    die "$msg at $f: $n\n";
> > +}
> 
> this is kind of a new style of error handling - might be worthy of a 
> mention somewhere ;)

debug helper... shouldn't be possible to reach this... I should probably
prefix the 2 subs below with a `my` though...
(I mostly ran into such stuff while playing with different ways of
naming/organising snapshot directories and not seeing *where* stuff
happened was annoying...)

> 
> > +
> > +# Given a name (eg. `vm-VMID-disk-ID.raw`), take the part up to the format suffix as the name of
> > +# the subdirectory (subvolume).
> > +sub raw_name_to_dir($) {
> > +    my ($raw) = @_;
> > +
> > +    # For the subvolume directory Strip the `.<format>` suffix:
> > +    if ($raw =~ /^(.*)\.raw$/) {
> > +	return $1;
> > +    }
> > +
> > +    __error "internal error: bad disk name: $raw";
> > +}
> > +
> > +sub raw_file_to_subvol($) {
> > +    my ($file) = @_;
> > +
> > +    if ($file =~ m|^(.*)/disk\.raw$|) {
> > +	return "$1";
> > +    }
> > +
> > +    __error "internal error: bad raw path: $file";
> > +}
> > +
(...)
> > +
> > +sub btrfs_get_subvol_id {
> > +    my ($class, $path) = @_;
> > +    my $info = $class->btrfs_cmd(['subvolume', 'show', '--', $path]);
> > +    if ($info !~ /^\s*(?:Object|Subvolume) ID:\s*(\d+)$/m) {
> > +	die "failed to get btrfs subvolume ID from: $info\n";
> 
> ugh at yet another parser for CLI tool output.. (not your fault I 
> know..)

This one I can actually replace. I just am not sure if I want to do it
all in pure perl or add some btrfs helper to the emerging `pve-rs` lib,
and since this code already existed in the old series, I postponed that
decision.


(...)
> > +    my $imagedir = $class->get_subdir($scfg, 'images');
> > +    $imagedir .= "/$vmid";
> > +    mkpath $imagedir;
> > +
> > +    my $path = $class->filesystem_path($scfg, $volname);
> > +    my $newname = $class->find_free_diskname($storeid, $scfg, $vmid, $format, 1);
> > +
> > +    # For btrfs subvolumes we don't actually need the "link":
> > +    #my $newvolname = "$basevmid/$basename/$vmid/$newname";
> > +    my $newvolname = "$vmid/$newname";
> > +    my $newpath = $class->filesystem_path($scfg, $newvolname);
> > +
> > +    my $subvol = $path;
> > +    my $newsubvol = $newpath;
> > +    if ($format eq 'raw') {
> > +	$subvol = raw_file_to_subvol($subvol);
> > +	$newsubvol = raw_file_to_subvol($newsubvol);
> > +    }
> 
> so actually these are two blocks for getting at $subvol and $newsubvol, 
> one for raw one for subvol. structuring it as such would make it more 
> readable IMHO..

Yeah I suppose those lines could look nicer. Also happens another
time...

> 
> > +
> > +    $class->btrfs_cmd(['subvolume', 'snapshot', '--', $subvol, $newsubvol]);
> > +
> > +    return $newvolname;
> > +}
> > +
> > +sub alloc_image {
> > +    my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
> > +
> > +    if ($fmt ne 'raw' && $fmt ne 'subvol') {
> > +	return PVE::Storage::DirPlugin::alloc_image(@_);
> > +    }
> > +
> > +    # From Plugin.pm:
> > +
> > +    my $imagedir = $class->get_subdir($scfg, 'images') . "/$vmid";
> > +
> > +    mkpath $imagedir;
> > +
> > +    $name = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt, 1) if !$name;
> > +
> > +    my (undef, $tmpfmt) = PVE::Storage::Plugin::parse_name_dir($name);
> > +
> > +    die "illegal name '$name' - wrong extension for format ('$tmpfmt != '$fmt')\n"
> > +	if $tmpfmt ne $fmt;
> > +
> > +    # End copy from Plugin.pm
> > +
> > +    my $subvol = "$imagedir/$name";
> > +    # .raw is not part of the directory name
> > +    $subvol =~ s/\.raw$//;
> 
> why not use your helpers for this (raw_.._to_..)

Here it can refer to a `subvol` as well which the helper doesn't allow,
but sure, could be improved.

> in a similar vain, there are three "add '/disk.raw' to path" sites 
> that might warrant a helper..

ok

(...)
> > +sub volume_snapshot {
> > +    my ($class, $scfg, $storeid, $volname, $snap) = @_;
> > +
> > +    my ($name, $vmid, $format) = ($class->parse_volname($volname))[1,2,6];
> > +    if ($format ne 'subvol' && $format ne 'raw') {
> > +	return PVE::Storage::Plugin::volume_snapshot(@_);
> > +    }
> > +
> > +    my $path = $class->filesystem_path($scfg, $volname);
> > +    my $snap_path = $class->filesystem_path($scfg, $volname, $snap);
> > +
> > +    if ($format eq 'raw') {
> > +	$path = raw_file_to_subvol($path);
> > +	$snap_path = raw_file_to_subvol($snap_path);
> > +    }
> 
> this is a repeating pattern (volname + optional snapshot name => path(s) 
> based on format)

yup, mentioned it above

> 
> > +
> > +    my $snapshot_dir = $class->get_subdir($scfg, 'images') . "/$vmid";
> > +    mkpath $snapshot_dir;
> > +
> > +    $class->btrfs_cmd(['subvolume', 'snapshot', '-r', '--', $path, $snap_path]);
> > +    return undef;
> > +}
> > +
> > +sub volume_rollback_is_possible {
> > +    my ($class, $scfg, $storeid, $volname, $snap) = @_; 
> > +
> > +    return 1; 
> 
> snapshot creation was delegated to Plugin.pm for non-raw and non-subvol, 
> so if we don't drop that part the same logic would need to apply here..

Ah, good catch.

(...)
> > +sub volume_has_feature {
> > +    my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running) = @_;
> > +
> > +    my $features = {
> > +	snapshot => {
> > +	    current => { qcow2 => 1, raw => 1, subvol => 1 },
> > +	    snap => { qcow2 => 1, raw => 1, subvol => 1 }
> > +	},
> > +	clone => {
> > +	    base => { qcow2 => 1, raw => 1, subvol => 1, vmdk => 1 },
> > +	    current => { raw => 1 },
> > +	    snap => { raw => 1 },
> > +	},
> > +	template => { current => { qcow2 => 1, raw => 1, vmdk => 1, subvol => 1 } },
> > +	copy => {
> > +	    base => { qcow2 => 1, raw => 1, subvol => 1, vmdk => 1 },
> > +	    current => { qcow2 => 1, raw => 1, subvol => 1, vmdk => 1 },
> > +	    snap => { qcow2 => 1, raw => 1, subvol => 1 },
> > +	},
> > +	sparseinit => { base => {qcow2 => 1, raw => 1, vmdk => 1 },
> > +			current => {qcow2 => 1, raw => 1, vmdk => 1 } },
> > +    };
> 
> some of that is duplicated from the regular plugin, so if we don't drop 
> the qcow2/.. support here we need to forward those based on $format..

good point