* [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements
@ 2025-02-28 14:50 Fiona Ebner
2025-02-28 14:50 ` [pve-devel] [PATCH storage 1/6] plugin: file size info: be consistent about size of directory subvol Fiona Ebner
` (6 more replies)
0 siblings, 7 replies; 13+ messages in thread
From: Fiona Ebner @ 2025-02-28 14:50 UTC (permalink / raw)
To: pve-devel
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 (6):
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
btrfs: note that btrfs_get_subvol_id() function is broken
src/PVE/API2/Storage/Content.pm | 4 +++-
src/PVE/Storage/BTRFSPlugin.pm | 6 +++++-
src/PVE/Storage/Plugin.pm | 5 +----
3 files changed, 9 insertions(+), 6 deletions(-)
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH storage 1/6] plugin: file size info: be consistent about size of directory subvol
2025-02-28 14:50 [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements Fiona Ebner
@ 2025-02-28 14:50 ` Fiona Ebner
2025-02-28 14:50 ` [pve-devel] [PATCH storage 2/6] btrfs: fix volume size info for subvolumes in scalar context Fiona Ebner
` (5 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Fiona Ebner @ 2025-02-28 14:50 UTC (permalink / raw)
To: pve-devel
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.". 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>
---
Gotta be a personal record for amount of bits in the commit message
versus amount of bits changed :P
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH storage 2/6] btrfs: fix volume size info for subvolumes in scalar context
2025-02-28 14:50 [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements Fiona Ebner
2025-02-28 14:50 ` [pve-devel] [PATCH storage 1/6] plugin: file size info: be consistent about size of directory subvol Fiona Ebner
@ 2025-02-28 14:50 ` Fiona Ebner
2025-03-03 7:31 ` Wolfgang Bumiller
2025-02-28 14:50 ` [pve-devel] [PATCH storage 3/6] plugin: volume export formats: avoid superfluous file_size_info() call Fiona Ebner
` (4 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Fiona Ebner @ 2025-02-28 14:50 UTC (permalink / raw)
To: pve-devel
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.".
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH storage 3/6] plugin: volume export formats: avoid superfluous file_size_info() call
2025-02-28 14:50 [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements Fiona Ebner
2025-02-28 14:50 ` [pve-devel] [PATCH storage 1/6] plugin: file size info: be consistent about size of directory subvol Fiona Ebner
2025-02-28 14:50 ` [pve-devel] [PATCH storage 2/6] btrfs: fix volume size info for subvolumes in scalar context Fiona Ebner
@ 2025-02-28 14:50 ` Fiona Ebner
2025-02-28 14:50 ` [pve-devel] [PATCH storage 4/6] api: volume info: do not fail for zero-sized subvolumes Fiona Ebner
` (3 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Fiona Ebner @ 2025-02-28 14:50 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH storage 4/6] api: volume info: do not fail for zero-sized subvolumes
2025-02-28 14:50 [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements Fiona Ebner
` (2 preceding siblings ...)
2025-02-28 14:50 ` [pve-devel] [PATCH storage 3/6] plugin: volume export formats: avoid superfluous file_size_info() call Fiona Ebner
@ 2025-02-28 14:50 ` Fiona Ebner
2025-02-28 14:50 ` [pve-devel] [PATCH storage 5/6] btrfs: die early for broken subvolume resize Fiona Ebner
` (2 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Fiona Ebner @ 2025-02-28 14:50 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH storage 5/6] btrfs: die early for broken subvolume resize
2025-02-28 14:50 [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements Fiona Ebner
` (3 preceding siblings ...)
2025-02-28 14:50 ` [pve-devel] [PATCH storage 4/6] api: volume info: do not fail for zero-sized subvolumes Fiona Ebner
@ 2025-02-28 14:50 ` Fiona Ebner
2025-03-03 8:02 ` Wolfgang Bumiller
2025-02-28 14:50 ` [pve-devel] [PATCH storage 6/6] btrfs: note that btrfs_get_subvol_id() function is broken Fiona Ebner
2025-03-03 9:25 ` [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements Fiona Ebner
6 siblings, 1 reply; 13+ messages in thread
From: Fiona Ebner @ 2025-02-28 14:50 UTC (permalink / raw)
To: pve-devel
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.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
src/PVE/Storage/BTRFSPlugin.pm | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index b1f7912..db678cf 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -497,6 +497,9 @@ sub volume_resize {
my $format = ($class->parse_volname($volname))[6];
if ($format eq 'subvol') {
+ # 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]);
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pve-devel] [PATCH storage 6/6] btrfs: note that btrfs_get_subvol_id() function is broken
2025-02-28 14:50 [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements Fiona Ebner
` (4 preceding siblings ...)
2025-02-28 14:50 ` [pve-devel] [PATCH storage 5/6] btrfs: die early for broken subvolume resize Fiona Ebner
@ 2025-02-28 14:50 ` Fiona Ebner
2025-03-03 8:04 ` Wolfgang Bumiller
2025-03-03 9:25 ` [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements Fiona Ebner
6 siblings, 1 reply; 13+ messages in thread
From: Fiona Ebner @ 2025-02-28 14:50 UTC (permalink / raw)
To: pve-devel
The btrfs_get_subvol_id() function is broken, because the btrfs
'subvol show' command is invoked with '-q', so there is no output.
The function currently has no reachable caller.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
src/PVE/Storage/BTRFSPlugin.pm | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index db678cf..e20b36c 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -232,6 +232,7 @@ sub btrfs_cmd {
return $msg;
}
+# 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]);
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH storage 2/6] btrfs: fix volume size info for subvolumes in scalar context
2025-02-28 14:50 ` [pve-devel] [PATCH storage 2/6] btrfs: fix volume size info for subvolumes in scalar context Fiona Ebner
@ 2025-03-03 7:31 ` Wolfgang Bumiller
2025-03-03 9:03 ` Fiona Ebner
0 siblings, 1 reply; 13+ messages in thread
From: Wolfgang Bumiller @ 2025-03-03 7:31 UTC (permalink / raw)
To: Fiona Ebner; +Cc: pve-devel
On Fri, Feb 28, 2025 at 03:50:18PM +0100, Fiona Ebner wrote:
> 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.".
Interesting how it says "reported to be zero", when this commit changes
it from `1` to, well, zero?
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH storage 5/6] btrfs: die early for broken subvolume resize
2025-02-28 14:50 ` [pve-devel] [PATCH storage 5/6] btrfs: die early for broken subvolume resize Fiona Ebner
@ 2025-03-03 8:02 ` Wolfgang Bumiller
0 siblings, 0 replies; 13+ messages in thread
From: Wolfgang Bumiller @ 2025-03-03 8:02 UTC (permalink / raw)
To: Fiona Ebner; +Cc: pve-devel
On Fri, Feb 28, 2025 at 03:50:21PM +0100, Fiona Ebner wrote:
> 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.
Curious. This must have changed in the btrfs CLI at some point. But yes,
bailing out here makes more sense with the code for the initial limit
being commented out...
(They also haven't yet implemented `--format json` for the command… -_-)
>
> As btrfs quotas are currently not implemented, die early with a clean
> error instead.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> src/PVE/Storage/BTRFSPlugin.pm | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
> index b1f7912..db678cf 100644
> --- a/src/PVE/Storage/BTRFSPlugin.pm
> +++ b/src/PVE/Storage/BTRFSPlugin.pm
> @@ -497,6 +497,9 @@ sub volume_resize {
>
> my $format = ($class->parse_volname($volname))[6];
> if ($format eq 'subvol') {
> + # 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]);
> --
> 2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH storage 6/6] btrfs: note that btrfs_get_subvol_id() function is broken
2025-02-28 14:50 ` [pve-devel] [PATCH storage 6/6] btrfs: note that btrfs_get_subvol_id() function is broken Fiona Ebner
@ 2025-03-03 8:04 ` Wolfgang Bumiller
0 siblings, 0 replies; 13+ messages in thread
From: Wolfgang Bumiller @ 2025-03-03 8:04 UTC (permalink / raw)
To: Fiona Ebner; +Cc: pve-devel
On Fri, Feb 28, 2025 at 03:50:22PM +0100, Fiona Ebner wrote:
> The btrfs_get_subvol_id() function is broken, because the btrfs
> 'subvol show' command is invoked with '-q', so there is no output.
>
> The function currently has no reachable caller.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> src/PVE/Storage/BTRFSPlugin.pm | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
> index db678cf..e20b36c 100644
> --- a/src/PVE/Storage/BTRFSPlugin.pm
> +++ b/src/PVE/Storage/BTRFSPlugin.pm
> @@ -232,6 +232,7 @@ sub btrfs_cmd {
> return $msg;
> }
>
> +# NOTE: this function is currently boken, because btrfs_cmd uses '-q' so there will be no output.
IMO we should instead comment out this function (and the now-unreachable
call to it in `volume_resize`, or more specifically, everything after
the new `die` in the affected if-branch).
This *is* easily fixed by removing the `-q` from the command - the logic
otherwise works AFAICT, but we don't need this code right now anyway.
> sub btrfs_get_subvol_id {
> my ($class, $path) = @_;
> my $info = $class->btrfs_cmd(['subvolume', 'show', '--', $path]);
> --
> 2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH storage 2/6] btrfs: fix volume size info for subvolumes in scalar context
2025-03-03 7:31 ` Wolfgang Bumiller
@ 2025-03-03 9:03 ` Fiona Ebner
2025-03-03 9:10 ` Fiona Ebner
0 siblings, 1 reply; 13+ messages in thread
From: Fiona Ebner @ 2025-03-03 9:03 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: pve-devel
Am 03.03.25 um 08:31 schrieb Wolfgang Bumiller:
> On Fri, Feb 28, 2025 at 03:50:18PM +0100, Fiona Ebner wrote:
>> 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.".
>
> Interesting how it says "reported to be zero", when this commit changes
> it from `1` to, well, zero?
When allocating the volume for the clone, we use the same size as the
old volume. The value 0 will lead to using 'subvol' format for the new
volume. With 1, it tries to use 'raw' format and then fails when
attempting to format.
>
>>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH storage 2/6] btrfs: fix volume size info for subvolumes in scalar context
2025-03-03 9:03 ` Fiona Ebner
@ 2025-03-03 9:10 ` Fiona Ebner
0 siblings, 0 replies; 13+ messages in thread
From: Fiona Ebner @ 2025-03-03 9:10 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: pve-devel
Am 03.03.25 um 10:03 schrieb Fiona Ebner:
> Am 03.03.25 um 08:31 schrieb Wolfgang Bumiller:
>> On Fri, Feb 28, 2025 at 03:50:18PM +0100, Fiona Ebner wrote:
>>> 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.".
>>
>> Interesting how it says "reported to be zero", when this commit changes
>> it from `1` to, well, zero?
>
> When allocating the volume for the clone, we use the same size as the
> old volume. The value 0 will lead to using 'subvol' format for the new
> volume. With 1, it tries to use 'raw' format and then fails when
> attempting to format.
(to format the file system).
To be precise, we currently pass along 1/1024 as the size and 'raw'
format to the storage layer :P
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements
2025-02-28 14:50 [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements Fiona Ebner
` (5 preceding siblings ...)
2025-02-28 14:50 ` [pve-devel] [PATCH storage 6/6] btrfs: note that btrfs_get_subvol_id() function is broken Fiona Ebner
@ 2025-03-03 9:25 ` Fiona Ebner
6 siblings, 0 replies; 13+ messages in thread
From: Fiona Ebner @ 2025-03-03 9:25 UTC (permalink / raw)
To: pve-devel
Superseded by v2:
https://lore.proxmox.com/pve-devel/20250303092445.13873-1-f.ebner@proxmox.com/T/#maf24450cef1ab5c66467e3ceb5c4c073fee69fa8
Am 28.02.25 um 15:50 schrieb Fiona Ebner:
> 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 (6):
> 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
> btrfs: note that btrfs_get_subvol_id() function is broken
>
> src/PVE/API2/Storage/Content.pm | 4 +++-
> src/PVE/Storage/BTRFSPlugin.pm | 6 +++++-
> src/PVE/Storage/Plugin.pm | 5 +----
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-03-03 9:25 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-02-28 14:50 [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements Fiona Ebner
2025-02-28 14:50 ` [pve-devel] [PATCH storage 1/6] plugin: file size info: be consistent about size of directory subvol Fiona Ebner
2025-02-28 14:50 ` [pve-devel] [PATCH storage 2/6] btrfs: fix volume size info for subvolumes in scalar context Fiona Ebner
2025-03-03 7:31 ` Wolfgang Bumiller
2025-03-03 9:03 ` Fiona Ebner
2025-03-03 9:10 ` Fiona Ebner
2025-02-28 14:50 ` [pve-devel] [PATCH storage 3/6] plugin: volume export formats: avoid superfluous file_size_info() call Fiona Ebner
2025-02-28 14:50 ` [pve-devel] [PATCH storage 4/6] api: volume info: do not fail for zero-sized subvolumes Fiona Ebner
2025-02-28 14:50 ` [pve-devel] [PATCH storage 5/6] btrfs: die early for broken subvolume resize Fiona Ebner
2025-03-03 8:02 ` Wolfgang Bumiller
2025-02-28 14:50 ` [pve-devel] [PATCH storage 6/6] btrfs: note that btrfs_get_subvol_id() function is broken Fiona Ebner
2025-03-03 8:04 ` Wolfgang Bumiller
2025-03-03 9:25 ` [pve-devel] [PATCH-SERIES storage 0/6] unsized subvol regression fix and improvements Fiona Ebner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal