[pve-devel] [PATCH storage] plugin: file size info: use fallback for actual size

[pve-devel] [PATCH storage] plugin: file size info: use fallback for actual size

[Fiona Ebner]

The actual-size property is an optional property in the QAPI
definition for ImageInfo. If it's not set, simply use the information
from stat() as a fallback. This is essentially the same
raw_get_allocated_file_size() in QEMU does anyways.

Reported in the community forum:
https://forum.proxmox.com/threads/118443/post-513421

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

Thanks to Mira for setting up a GlusterFS instance and discussing the
issue with me!

I'm not sure why QEMU fails here, didn't see much that could go wrong
beside the fstat() call failing. But our stat() call in the beginning
of file_size_info already succeeded at that point :/ The mysteries of
QEMU+GlusterFS.

Also, it's a bit strange to call qemu-img info regardless of whether
the image is a VM image or not. E.g., this results in the format
property for backups to always be raw, rather than the backup format.
Should we change that (for 8.0)?

 PVE/Storage/Plugin.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 8a41df1..7773ac3 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -899,6 +899,7 @@ sub file_size_info {
     }
 
     my ($size, $format, $used, $parent) = $info->@{qw(virtual-size format actual-size backing-filename)};
+    $used ||= $st->blocks * 512;
 
     ($size) = ($size =~ /^(\d+)$/) or die "size '$size' not an integer\n"; # untaint
     # coerce back from string
-- 
2.30.2

Re: [pve-devel] [PATCH storage] plugin: file size info: use fallback for actual size

[Thomas Lamprecht]

Am 28/11/2022 um 16:55 schrieb Fiona Ebner:
> The actual-size property is an optional property in the QAPI
> definition for ImageInfo. If it's not set, simply use the information
> from stat() as a fallback. This is essentially the same
> raw_get_allocated_file_size() in QEMU does anyways.
> 
> Reported in the community forum:
> https://forum.proxmox.com/threads/118443/post-513421
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Thanks to Mira for setting up a GlusterFS instance and discussing the
> issue with me!
> 
> I'm not sure why QEMU fails here, didn't see much that could go wrong
> beside the fstat() call failing. But our stat() call in the beginning
> of file_size_info already succeeded at that point :/ The mysteries of
> QEMU+GlusterFS.
> 
> Also, it's a bit strange to call qemu-img info regardless of whether
> the image is a VM image or not. E.g., this results in the format
> property for backups to always be raw, rather than the backup format.
> Should we change that (for 8.0)?
> 
>  PVE/Storage/Plugin.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 8a41df1..7773ac3 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -899,6 +899,7 @@ sub file_size_info {
>      }
>  
>      my ($size, $format, $used, $parent) = $info->@{qw(virtual-size format actual-size backing-filename)};
> +    $used ||= $st->blocks * 512;

in general OK, but can we really be sure that blocks are always 512 bytes?

>  
>      ($size) = ($size =~ /^(\d+)$/) or die "size '$size' not an integer\n"; # untaint
>      # coerce back from string

Re: [pve-devel] [PATCH storage] plugin: file size info: use fallback for actual size

[Fiona Ebner]

Am 11.01.23 um 16:46 schrieb Thomas Lamprecht:
> Am 28/11/2022 um 16:55 schrieb Fiona Ebner:
>> The actual-size property is an optional property in the QAPI
>> definition for ImageInfo. If it's not set, simply use the information
>> from stat() as a fallback. This is essentially the same
>> raw_get_allocated_file_size() in QEMU does anyways.
>>
>> Reported in the community forum:
>> https://forum.proxmox.com/threads/118443/post-513421
>>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Thanks to Mira for setting up a GlusterFS instance and discussing the
>> issue with me!
>>
>> I'm not sure why QEMU fails here, didn't see much that could go wrong
>> beside the fstat() call failing. But our stat() call in the beginning
>> of file_size_info already succeeded at that point :/ The mysteries of
>> QEMU+GlusterFS.
>>
>> Also, it's a bit strange to call qemu-img info regardless of whether
>> the image is a VM image or not. E.g., this results in the format
>> property for backups to always be raw, rather than the backup format.
>> Should we change that (for 8.0)?
>>
>>  PVE/Storage/Plugin.pm | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>> index 8a41df1..7773ac3 100644
>> --- a/PVE/Storage/Plugin.pm
>> +++ b/PVE/Storage/Plugin.pm
>> @@ -899,6 +899,7 @@ sub file_size_info {
>>      }
>>  
>>      my ($size, $format, $used, $parent) = $info->@{qw(virtual-size format actual-size backing-filename)};
>> +    $used ||= $st->blocks * 512;
> 
> in general OK, but can we really be sure that blocks are always 512 bytes?
> 

Initially, I only checked 'man 2 stat' where this is the case:
>                blkcnt_t  st_blocks;      /* Number of 512B blocks allocated */

But checking again now, 'perldoc -f stat' (File::stat mentions it uses
Perl's builtin stat()) actually states:
>              12 blocks   actual number of system-specific blocks allocated
>                          on disk (often, but not always, 512 bytes each)

Trying to decipher the Perl 5 source code, I /think/ it will just use
stat(2) on Linux (a quick check with strace seems to confirm this) and
I'd say it would be surprising if not, but I'm not 100% sure.

That said, the original issue here was GlusterFS reporting an incorrect
value (see the forum thread). The new fallback introduced by this patch
would only help if 'qemu-img info' fails to determine the size for some
other reason (it also just does st_blocks * 512 in
raw_get_allocated_file_size()), so I'm not sure the patch is even worth
it after all.

Re: [pve-devel] [PATCH storage] plugin: file size info: use fallback for actual size

[Wolfgang Bumiller]

On Thu, Jan 12, 2023 at 09:33:51AM +0100, Fiona Ebner wrote:
> Am 11.01.23 um 16:46 schrieb Thomas Lamprecht:
> > Am 28/11/2022 um 16:55 schrieb Fiona Ebner:
> >> The actual-size property is an optional property in the QAPI
> >> definition for ImageInfo. If it's not set, simply use the information
> >> from stat() as a fallback. This is essentially the same
> >> raw_get_allocated_file_size() in QEMU does anyways.
> >>
> >> Reported in the community forum:
> >> https://forum.proxmox.com/threads/118443/post-513421
> >>
> >> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> >> ---
> >>
> >> Thanks to Mira for setting up a GlusterFS instance and discussing the
> >> issue with me!
> >>
> >> I'm not sure why QEMU fails here, didn't see much that could go wrong
> >> beside the fstat() call failing. But our stat() call in the beginning
> >> of file_size_info already succeeded at that point :/ The mysteries of
> >> QEMU+GlusterFS.
> >>
> >> Also, it's a bit strange to call qemu-img info regardless of whether
> >> the image is a VM image or not. E.g., this results in the format
> >> property for backups to always be raw, rather than the backup format.
> >> Should we change that (for 8.0)?
> >>
> >>  PVE/Storage/Plugin.pm | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> >> index 8a41df1..7773ac3 100644
> >> --- a/PVE/Storage/Plugin.pm
> >> +++ b/PVE/Storage/Plugin.pm
> >> @@ -899,6 +899,7 @@ sub file_size_info {
> >>      }
> >>  
> >>      my ($size, $format, $used, $parent) = $info->@{qw(virtual-size format actual-size backing-filename)};
> >> +    $used ||= $st->blocks * 512;
> > 
> > in general OK, but can we really be sure that blocks are always 512 bytes?
> > 
> 
> Initially, I only checked 'man 2 stat' where this is the case:
> >                blkcnt_t  st_blocks;      /* Number of 512B blocks allocated */
> 
> But checking again now, 'perldoc -f stat' (File::stat mentions it uses
> Perl's builtin stat()) actually states:
> >              12 blocks   actual number of system-specific blocks allocated
> >                          on disk (often, but not always, 512 bytes each)
> 
> Trying to decipher the Perl 5 source code, I /think/ it will just use
> stat(2) on Linux (a quick check with strace seems to confirm this) and
> I'd say it would be surprising if not, but I'm not 100% sure.

There aren't too many choices there. It may switch to `statx` at some
point where there's only `stx_blocks` which is specifically documented
as "Number of 512B blocks allocated".
The terminology in the I/O layer around blocks, sectors and "groups of
512 bytes" is quite annoying really.

But hey, at least it's not `statvfs` which fills a struct with
"garbage" on some systems (see `man 3 statvfs` on freebsd14)

As for why glusterfs would fail with our stat() having succeeded, that's
just glusterfs. Don't question it...

> 
> That said, the original issue here was GlusterFS reporting an incorrect
> value (see the forum thread). The new fallback introduced by this patch
> would only help if 'qemu-img info' fails to determine the size for some
> other reason (it also just does st_blocks * 512 in
> raw_get_allocated_file_size()), so I'm not sure the patch is even worth
> it after all.