[pve-devel] [PATCH-SERIES v2 storage 0/5] unsized subvol regression fix and improvements

[pve-devel] [PATCH-SERIES v2 storage 0/5] unsized subvol regression fix and improvements

[Fiona Ebner]

Changes in v2:
* comment out dead code in BTRFS plugin
* improve commit messages regarding clone
* squash patches 5/6 and 6/6 from v1

In list context, the file_size_info() function in Plugin.pm and the
volume_size_info() function in BTRFSPlugin.pm would return 0 for the
size, but in scalar context 1. As reported in the community forum [0],
the change in commit e50dde0 ("volume export: rely on storage plugin's
format"), changing the caller in volume_export() to scalar context
exposed the inconsistency in the return value for the size. This led
to breakage of migration with unsized btrfs subvolumes.

The first patch is the regression fix. Second patch fixes cloning
containers with unsized subvolumes on BTFRS. The rest are independent
further cleanups or improvements in the context of unsized subvols.

[0]: https://forum.proxmox.com/threads/162943/


Fiona Ebner (5):
  plugin: file size info: be consistent about size of directory subvol
  btrfs: fix volume size info for subvolumes in scalar context
  plugin: volume export formats: avoid superfluous file_size_info() call
  api: volume info: do not fail for zero-sized subvolumes
  btrfs: die early for broken subvolume resize

 src/PVE/API2/Storage/Content.pm |  4 +++-
 src/PVE/Storage/BTRFSPlugin.pm  | 30 +++++++++++++++++-------------
 src/PVE/Storage/Plugin.pm       |  5 +----
 3 files changed, 21 insertions(+), 18 deletions(-)

-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 1/5] plugin: file size info: be consistent about size of directory subvol

[Fiona Ebner]

In list context, the file_size_info() function in Plugin.pm would
return 0 for the size of a subvol directory, but in scalar context 1.
As reported in the community forum [0], the change in commit e50dde0
("volume export: rely on storage plugin's format"), changing the
caller in volume_export() to scalar context exposed the inconsistency
in the return value for the size. This led to breakage of migration
with unsized btrfs subvolumes.

Align the returned values to avoid such surprises. Caller that would
treat 0 as an error should use list context and check if it is a
subvol, if they are able to handle them.

NOTE: it is not possible to attach either a subvol, or a volume with
zero size to a VM.

Existing callers of file_size_info() do not break with this change:
GuestImport/OVF.pm - parse_ovf() +
API/Qemu.pm - create_disks():
  Doesn't support subvol directories, dies if size is 0.
Storage/Plugin.pm - create_base() +
Storage/{BTRFS,}Plugin.pm - list_images():
  Checks for definedness of $size.
Storage/{BTRFS,ESXi,}Plugin.pm - volume_size_info():
  Transitive, see below.
Storage/Plugin.pm - volume_export():
  Regressed by commit e50dde0, this change will restore previous
  behavior.
Storage/Plugin.pm - volume_export_formats() +
GuestImport.pm - extract_disk_from_import_file() +
Storage.pm - assert_iso_content():
  Doesn't use the result.
CLI/qm.pm - importdisk:
  Dies early if source is a directory.
CLI/qm.pm - importovf:
  Calls parse_ovf() earlier.

Existing callers of volume_size_info() do not break with this change:
API2/Storage/Content.pm - info:
  Uses list context, not affected by change.
QemuMigrate.pm - scan_local_volumes() +
API2/Qemu.pm - create_disks(), first call:
  Uses list context, not affected by change, does not support subvol
  directories.
API2/Qemu.pm - create_disks(), second call +
API2/Qemu.pm - import_from_volid():
  Doesn't support subvol directories, dies if size is 0.
API2/Qemu.pm - resize_vm +
VZDump/QemuServer.pm - prepare() +
QemuServer.pm - clone_disk() +
QemuServer.pm - create_efidisk():
  Doesn't support subvol directories (see NOTE above), but would not
  die directly if size is 0. (In case of create_efidisk(), the size of
  the just created disk is queried.)
API2/LXC.pm - resize_vm:
  For directory plugins, the subsequent call to resize_volume() fails
  with "can't resize this image format".
  For BTRFS, quotas are currently not supported and the call to
  resize_volume() fails with "failed to get btrfs subvolume ID from:".
  This is because the btrfs 'subvol show' command is invoked with
  '-q', so there is no output. Even if it would work, it would be more
  correct to use 0 as the current size to add to the new quota rather
  than 1.
LXC/Config.pm - rescan_volume():
  This will happily use size=1 from before this change, but that is
  not correct. A subsequent 'pct rescan' will correct the size to 0.
  It is only used when hotplugging an existing subvol directory.
LXC.pm - copy_volume():
  This will happily use size=1 from before this change, but that is
  not correct and will lead to failure "mkfs.ext4: Device size
  reported to be zero.". That is because with non-zero size, the
  allocation of the volume for the clone will be done with 'raw'
  format by the alloc_disk() helper in LXC.pm rather than 'subvol'.
  This change will make cloning containers with unsized subvol
  directories possible.
QemuServer/Cloudinit.pm - commit_cloudinit_disk():
  Doesn't support subvol directories (see NOTE above), will allocate
  a new volume when size is 0.

[0]: https://forum.proxmox.com/threads/162943/

Fixes: e50dde0 ("volume export: rely on storage plugin's format")
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
* Improve commit message regarding clone

 src/PVE/Storage/Plugin.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 65cf43f..ce1530e 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1007,7 +1007,7 @@ sub file_size_info {
     if (S_ISDIR($st->mode)) {
 	$handle_error->("expected format '$file_format', but '$filename' is a directory\n")
 	    if $file_format && $file_format ne 'subvol';
-	return wantarray ? (0, 'subvol', 0, undef, $st->ctime) : 1;
+	return wantarray ? (0, 'subvol', 0, undef, $st->ctime) : 0;
     } elsif ($file_format && $file_format eq 'subvol') {
 	$handle_error->("expected format '$file_format', but '$filename' is not a directory\n");
     }
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 2/5] btrfs: fix volume size info for subvolumes in scalar context

[Fiona Ebner]

Return the same size as in list context. See also commit "plugin: file
size info: be consistent about size of directory subvol".

Fixes cloning containers with unsized subvolumes on BTRFS. Before the
change, this would fail with "mkfs.ext4: Device size reported to be
zero.". That is because with non-zero size, the allocation of the
volume for the clone will be done with 'raw' format by the
alloc_disk() helper in LXC.pm rather than 'subvol'. This change will
make cloning containers with unsized subvol directories possible.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
* Improve commit message regarding clone

 src/PVE/Storage/BTRFSPlugin.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index 1966b6f..b1f7912 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -486,7 +486,7 @@ sub volume_size_info {
 	my $ctime = (stat($path))[10];
 	my ($used, $size) = (0, 0);
 	#my ($used, $size) = btrfs_subvol_quota($class, $path); # uses wantarray
-	return wantarray ? ($size, 'subvol', $used, undef, $ctime) : 1;
+	return wantarray ? ($size, 'subvol', $used, undef, $ctime) : $size;
     }
 
     return PVE::Storage::Plugin::file_size_info($path, $timeout, $format);
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 3/5] plugin: volume export formats: avoid superfluous file_size_info() call

[Fiona Ebner]

The result from the file_size_info() call is not used by
volume_export_formats() and most failure scenarios of file_size_info()
lead to an undefined return value rather than a failure. This includes
the case for a non-existent file. The default path() implementation
doesn't do any existence check either.

An interesting scenario where file_size_info() does fail, is when the
volume is corrupted or not in the queried format. But this is a rare
edge case, so an early check doesn't seem worth it. It will be caught
by volume_export() itself, or in case of VM migration, also when
querying the size during scanning of local volumes.

While checking for the definedness of $size could serve as an early
sanity check, it is not currently done and other plugins don't do such
early checks in their implementation of volume_export_formats()
either. Keep the implementation abstract in Plugin.pm too and avoid
doing IO. Callers that want to do early existence checks or similar
can do so themselves explicitly, covering all plugins.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/Plugin.pm | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index ce1530e..ca70c03 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1733,10 +1733,7 @@ sub volume_export {
 sub volume_export_formats {
     my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
     if ($scfg->{path} && !defined($snapshot) && !defined($base_snapshot)) {
-	my ($file) = $class->path($scfg, $volname, $storeid)
-	    or return;
 	my $format = ($class->parse_volname($volname))[6];
-	my $size = file_size_info($file, undef, $format);
 
 	if ($with_snapshots) {
 	    return ($format.'+size') if ($format eq 'qcow2' || $format eq 'vmdk');
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 4/5] api: volume info: do not fail for zero-sized subvolumes

[Fiona Ebner]

The special case of size being zero is supported if the volume is of
format 'subvol' is a special use case supported in Proxmox VE.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/Storage/Content.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
index fe0ad4a..fbd371c 100644
--- a/src/PVE/API2/Storage/Content.pm
+++ b/src/PVE/API2/Storage/Content.pm
@@ -324,7 +324,9 @@ __PACKAGE__->register_method ({
 
 	my $path = PVE::Storage::path($cfg, $volid);
 	my ($size, $format, $used, $parent) =  PVE::Storage::volume_size_info($cfg, $volid);
-	die "volume_size_info on '$volid' failed\n" if !($format && $size);
+	die "volume_size_info on '$volid' failed - no format\n" if !$format;
+	die "volume_size_info on '$volid' failed - no size\n" if !defined($size);
+	die "volume '$volid' has size zero\n" if !$size && $format ne 'subvol';
 
 	my $entry = {
 	    path => $path,
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 5/5] btrfs: die early for broken subvolume resize

[Fiona Ebner]

In the BTRFS plugin, resize_volume() for a subovlume currently fails
with "failed to get btrfs subvolume ID from: ". This is because the
btrfs 'subvol show' command is invoked with '-q', so there is no
output.

As btrfs quotas are currently not implemented, die early with a clean
error instead and comment out the unused code for now.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
* comment out dead code, squash patches

 src/PVE/Storage/BTRFSPlugin.pm | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index b1f7912..ce21297 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -232,14 +232,15 @@ sub btrfs_cmd {
     return $msg;
 }
 
-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";
-    }
-    return $1;
-}
+# NOTE: this function is currently boken, because btrfs_cmd uses '-q' so there will be no output.
+#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";
+#    }
+#    return $1;
+#}
 
 my sub chattr : prototype($$$) {
     my ($fh, $mask, $xor) = @_;
@@ -497,10 +498,13 @@ sub volume_resize {
 
     my $format = ($class->parse_volname($volname))[6];
     if ($format eq 'subvol') {
-	my $path = $class->filesystem_path($scfg, $volname);
-	my $id = '0/' . $class->btrfs_get_subvol_id($path);
-	$class->btrfs_cmd(['qgroup', 'limit', '--', "${size}k", "0/$id", $path]);
-	return undef;
+	# NOTE: `btrfs send/recv` actually drops quota information so supporting subvolumes with
+	# quotas doesn't play nice with send/recv.
+	die "cannot resize subvolume - btrfs quotas are currently not supported\n";
+	# my $path = $class->filesystem_path($scfg, $volname);
+	# my $id = '0/' . $class->btrfs_get_subvol_id($path);
+	# $class->btrfs_cmd(['qgroup', 'limit', '--', "${size}k", "0/$id", $path]);
+	# return undef;
     }
 
     return PVE::Storage::Plugin::volume_resize(@_);
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel