[libvirt] [PATCH] Fix sparse volume allocation reporting
I have a sparse volume with a capacity of 1000M, and an allocation of a little over 750M. 'du' prints the correct sizes, but 'virsh vol-dumpxml' shows: <capacity>1048576000</capacity> <allocation>6406307840</allocation> This is because we were calculating the allocation size using the requested fs block size, instead of what stat actually uses as a block size (DEV_BSIZE in sys/params.h). sys/params.h looks to be present in mingw32-runtime, so I didn't add a configure check for it. The attached patch fixes allocation listing for me. Thanks, Cole
On Tue, Mar 24, 2009 at 03:29:21PM -0400, Cole Robinson wrote:
I have a sparse volume with a capacity of 1000M, and an allocation of a little over 750M. 'du' prints the correct sizes, but 'virsh vol-dumpxml' shows:
<capacity>1048576000</capacity> <allocation>6406307840</allocation>
This is because we were calculating the allocation size using the requested fs block size, instead of what stat actually uses as a block size (DEV_BSIZE in sys/params.h). sys/params.h looks to be present in mingw32-runtime, so I didn't add a configure check for it.
That's weird but the mistake is understandable. The man page (Fedora 9 maybe it was updated) states: ----------------------------------------------------------------- The st_blocks field indicates the number of blocks allocated to the file, 512-byte units. (This may be smaller than st_size/512 when the file has holes.) ----------------------------------------------------------------- that's even worse I think.
The attached patch fixes allocation listing for me.
Looks fine to me, I wonder if that will work correctly on solaris for example. Coding wise I'm just a bit surprized with the declaration in the middle of a block, but I probably need to get over it :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Mar 24, 2009 at 03:29:21PM -0400, Cole Robinson wrote:
I have a sparse volume with a capacity of 1000M, and an allocation of a little over 750M. 'du' prints the correct sizes, but 'virsh vol-dumpxml' shows:
<capacity>1048576000</capacity> <allocation>6406307840</allocation>
This is because we were calculating the allocation size using the requested fs block size, instead of what stat actually uses as a block size (DEV_BSIZE in sys/params.h). sys/params.h looks to be present in mingw32-runtime, so I didn't add a configure check for it.
#if HAVE_SELINUX @@ -204,8 +205,15 @@ virStorageBackendUpdateVolTargetInfoFD(virConnectPtr conn, if (allocation) { if (S_ISREG(sb.st_mode)) { #ifndef __MINGW32__ - *allocation = (unsigned long long)sb.st_blocks * - (unsigned long long)sb.st_blksize; + + unsigned long long blksize; +#ifdef DEV_BSIZE + blksize = (unsigned long long) DEV_BSIZE; +#else + blksize = (unsigned long long)sb.st_blksize; +#endif + *allocation = (unsigned long long)sb.st_blocks * blksize;
Having read the man page again, I'm inclined to say using st_blksize is always wrong no matter what, because it is quite clear that 'st_blocks' is always in 512 byte units. So perhaps we might be better of doing #ifndef DEV_BSIZE #define DEV_BSIZE 512 #endif And then always using DEV_BSIZE. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Mar 26, 2009 at 01:19:12PM +0000, Daniel P. Berrange wrote:
On Tue, Mar 24, 2009 at 03:29:21PM -0400, Cole Robinson wrote: [...] Having read the man page again, I'm inclined to say using st_blksize is always wrong no matter what, because it is quite clear that 'st_blocks' is always in 512 byte units. So perhaps we might be better of doing
#ifndef DEV_BSIZE #define DEV_BSIZE 512 #endif
And then always using DEV_BSIZE.
In those kind of cases I go down to the spec and it states (in the informative section though): ----------------------------------------- http://www.opengroup.org/onlinepubs/000095399/basedefs/sys/stat.h.html "The unit for the st_blocks member of the stat structure is not defined within IEEE Std 1003.1-2001. In some implementations it is 512 bytes. It may differ on a file system basis. There is no correlation between values of the st_blocks and st_blksize, and the f_bsize (from <sys/statvfs.h>) structure members. Traditionally, some implementations defined the multiplier for st_blocks in <sys/param.h> as the symbol DEV_BSIZE." ----------------------------------------- So I agree with Dan, we need to drop st_blksize in any volume size computation, and fallback to 512 if not defined, apparently only src/storage_backend.c referenced it :-) I still find the "It may differ on a file system basis" to be a bit frightening considering the sandard doesn't seems to indicate how to extract that information from the filesystem :-( , oh well ... Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Mar 26, 2009 at 05:55:00PM +0100, Daniel Veillard wrote:
On Thu, Mar 26, 2009 at 01:19:12PM +0000, Daniel P. Berrange wrote:
On Tue, Mar 24, 2009 at 03:29:21PM -0400, Cole Robinson wrote: [...] Having read the man page again, I'm inclined to say using st_blksize is always wrong no matter what, because it is quite clear that 'st_blocks' is always in 512 byte units. So perhaps we might be better of doing
#ifndef DEV_BSIZE #define DEV_BSIZE 512 #endif
And then always using DEV_BSIZE.
In those kind of cases I go down to the spec and it states (in the informative section though):
----------------------------------------- http://www.opengroup.org/onlinepubs/000095399/basedefs/sys/stat.h.html "The unit for the st_blocks member of the stat structure is not defined within IEEE Std 1003.1-2001. In some implementations it is 512 bytes. It may differ on a file system basis. There is no correlation between values of the st_blocks and st_blksize, and the f_bsize (from <sys/statvfs.h>) structure members.
Traditionally, some implementations defined the multiplier for st_blocks in <sys/param.h> as the symbol DEV_BSIZE." -----------------------------------------
So I agree with Dan, we need to drop st_blksize in any volume size computation, and fallback to 512 if not defined, apparently only src/storage_backend.c referenced it :-)
I still find the "It may differ on a file system basis" to be a bit frightening considering the sandard doesn't seems to indicate how to extract that information from the filesystem :-( , oh well ...
I guess if we come across a filesystem where it is not 512, then someone will have created an ioctl() or equivalent to let us find out the true value. Failing that, relying on 512 seems like a good plan. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Thu, Mar 26, 2009 at 05:55:00PM +0100, Daniel Veillard wrote:
On Thu, Mar 26, 2009 at 01:19:12PM +0000, Daniel P. Berrange wrote:
On Tue, Mar 24, 2009 at 03:29:21PM -0400, Cole Robinson wrote: [...] Having read the man page again, I'm inclined to say using st_blksize is always wrong no matter what, because it is quite clear that 'st_blocks' is always in 512 byte units. So perhaps we might be better of doing
#ifndef DEV_BSIZE #define DEV_BSIZE 512 #endif
And then always using DEV_BSIZE. In those kind of cases I go down to the spec and it states (in the informative section though):
----------------------------------------- http://www.opengroup.org/onlinepubs/000095399/basedefs/sys/stat.h.html "The unit for the st_blocks member of the stat structure is not defined within IEEE Std 1003.1-2001. In some implementations it is 512 bytes. It may differ on a file system basis. There is no correlation between values of the st_blocks and st_blksize, and the f_bsize (from <sys/statvfs.h>) structure members.
Traditionally, some implementations defined the multiplier for st_blocks in <sys/param.h> as the symbol DEV_BSIZE." -----------------------------------------
So I agree with Dan, we need to drop st_blksize in any volume size computation, and fallback to 512 if not defined, apparently only src/storage_backend.c referenced it :-)
I still find the "It may differ on a file system basis" to be a bit frightening considering the sandard doesn't seems to indicate how to extract that information from the filesystem :-( , oh well ...
I guess if we come across a filesystem where it is not 512, then someone will have created an ioctl() or equivalent to let us find out the true value. Failing that, relying on 512 seems like a good plan.
Daniel
Okay, updated patch attached. Thanks, Cole
On Thu, Apr 02, 2009 at 02:39:58PM -0400, Cole Robinson wrote:
Daniel P. Berrange wrote:
On Thu, Mar 26, 2009 at 05:55:00PM +0100, Daniel Veillard wrote:
So I agree with Dan, we need to drop st_blksize in any volume size computation, and fallback to 512 if not defined, apparently only src/storage_backend.c referenced it :-)
I still find the "It may differ on a file system basis" to be a bit frightening considering the sandard doesn't seems to indicate how to extract that information from the filesystem :-( , oh well ...
I guess if we come across a filesystem where it is not 512, then someone will have created an ioctl() or equivalent to let us find out the true value. Failing that, relying on 512 seems like a good plan.
Daniel
Okay, updated patch attached.
ACK, looks good to me.
commit 3f289f7c1ea04cb5d14bf125959ba41b13b64443 Author: Cole Robinson <crobinso@redhat.com> Date: Wed Apr 1 16:00:58 2009 -0400
Fix sparse volume allocation reporting.
diff --git a/src/storage_backend.c b/src/storage_backend.c index 71b8020..79c070c 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -36,6 +36,7 @@ #include <fcntl.h> #include <stdint.h> #include <sys/stat.h> +#include <sys/param.h> #include <dirent.h>
#if HAVE_SELINUX @@ -64,6 +65,9 @@ #include "storage_backend_fs.h" #endif
+#ifndef DEV_BSIZE +#define DEV_BSIZE 512 +#endif
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -211,7 +215,7 @@ virStorageBackendUpdateVolTargetInfoFD(virConnectPtr conn, if (S_ISREG(sb.st_mode)) { #ifndef __MINGW32__ *allocation = (unsigned long long)sb.st_blocks * - (unsigned long long)sb.st_blksize; + (unsigned long long)DEV_BSIZE; #else *allocation = sb.st_size; #endif
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Cole Robinson wrote:
Daniel P. Berrange wrote:
On Thu, Mar 26, 2009 at 05:55:00PM +0100, Daniel Veillard wrote:
On Thu, Mar 26, 2009 at 01:19:12PM +0000, Daniel P. Berrange wrote:
On Tue, Mar 24, 2009 at 03:29:21PM -0400, Cole Robinson wrote: [...] Having read the man page again, I'm inclined to say using st_blksize is always wrong no matter what, because it is quite clear that 'st_blocks' is always in 512 byte units. So perhaps we might be better of doing
#ifndef DEV_BSIZE #define DEV_BSIZE 512 #endif
And then always using DEV_BSIZE. In those kind of cases I go down to the spec and it states (in the informative section though):
----------------------------------------- http://www.opengroup.org/onlinepubs/000095399/basedefs/sys/stat.h.html "The unit for the st_blocks member of the stat structure is not defined within IEEE Std 1003.1-2001. In some implementations it is 512 bytes. It may differ on a file system basis. There is no correlation between values of the st_blocks and st_blksize, and the f_bsize (from <sys/statvfs.h>) structure members.
Traditionally, some implementations defined the multiplier for st_blocks in <sys/param.h> as the symbol DEV_BSIZE." -----------------------------------------
So I agree with Dan, we need to drop st_blksize in any volume size computation, and fallback to 512 if not defined, apparently only src/storage_backend.c referenced it :-)
I still find the "It may differ on a file system basis" to be a bit frightening considering the sandard doesn't seems to indicate how to extract that information from the filesystem :-( , oh well ... I guess if we come across a filesystem where it is not 512, then someone will have created an ioctl() or equivalent to let us find out the true value. Failing that, relying on 512 seems like a good plan.
Daniel
Okay, updated patch attached.
Applied now. Thanks, Cole
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard