From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 409637513B for ; Wed, 23 Jun 2021 14:44:10 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2BD8CAED2 for ; Wed, 23 Jun 2021 14:43:40 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 65263AEC5 for ; Wed, 23 Jun 2021 14:43:38 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 2B0964229E for ; Wed, 23 Jun 2021 14:43:33 +0200 (CEST) Date: Wed, 23 Jun 2021 14:43:31 +0200 From: Wolfgang Bumiller To: Fabian =?utf-8?Q?Gr=C3=BCnbichler?= Cc: Proxmox VE development discussion Message-ID: <20210623124331.ksyp7zej77htcvd6@olga.proxmox.com> References: <20210622121828.84178-1-w.bumiller@proxmox.com> <20210622121828.84178-2-w.bumiller@proxmox.com> <1624446697.vi3jolanip.astroid@nora.none> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1624446697.vi3jolanip.astroid@nora.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.771 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v2 storage 1/5] add BTRFS storage plugin X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Jun 2021 12:44:10 -0000 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 `.` 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