[libvirt] [PATCH] Fix performance problem of virStorageVolCreateXMLFrom()

[Daniel P. Berrange]

On Tue, Mar 08, 2011 at 01:01:34PM +0900, Minoru Usui wrote:
> virStorageVolCreateXMLFrom() is slow if destination Pool is LVM.
> Because write block size is not appropriate.
> 
> On linux environment, block size of LVM Pool which is made by virStoragePoolCreateXML()
> is 4096 bytes.
> On the other hand, write block size of virStorageVolCreateXMLFrom() is 512 bytes.
> This value are hard corded in virStorageBackendCopyToFD().
> 
> This patch fix this block size problem on linux.
> 
> [lvm block size]
>   # virsh vol-dumpxml vol.img --pool lvm-pool |grep '<path>'
>       <path>/dev/lvm-pool/vol.img</path>
>   # blockdev --getbsz /dev/lvm-pool/vol.img
>   4096
> 
> [before patch]
>   # echo 1 > /proc/sys/vm/drop_caches
>   # time virsh  vol-create-from lvm-pool vol.xml --inputpool dir-pool vol.img
>   real    1m17.736s
>   user    0m0.007s
>   sys     0m0.021s
> 
> [after patch]
>   # echo 1 > /proc/sys/vm/drop_caches
>   # time virsh  vol-create-from lvm-pool vol.xml --inputpool dir-pool vol.img
>   real    0m36.883s
>   user    0m0.011s
>   sys     0m0.031s

This makes good sense. 512 is clearly too small, even for plain
harddrives which may come with 4096 byte sectors these days.


> Signed-off-by: Minoru Usui <usui at mxm.nes.nec.co.jp>
> ---
>  src/storage/storage_backend.c |   23 +++++++++++++++++++----
>  1 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 174155d..a1a930c 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -36,6 +36,10 @@
>  #include <sys/stat.h>
>  #include <sys/param.h>
>  #include <dirent.h>
> +#ifdef __linux__
> +#include <sys/ioctl.h>
> +#include <linux/fs.h>
> +#endif
>  
>  #if HAVE_SELINUX
>  # include <selinux/selinux.h>
> @@ -108,6 +112,9 @@ enum {
>      TOOL_QCOW_CREATE,
>  };
>  
> +#define READ_BLOCK_SIZE_DEFAULT  (1024 * 1024)
> +#define WRITE_BLOCK_SIZE_DEFAULT (512)
> +
>  static int ATTRIBUTE_NONNULL (2)
>  virStorageBackendCopyToFD(virStorageVolDefPtr vol,
>                            virStorageVolDefPtr inputvol,
> @@ -119,8 +126,10 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
>      int amtread = -1;
>      int ret = 0;
>      unsigned long long remain;
> -    size_t rbytes = 1024 * 1024; /* For Read */
> -    char zerobuf[512];
> +    size_t rbytes = READ_BLOCK_SIZE_DEFAULT;
> +    size_t wbytes = WRITE_BLOCK_SIZE_DEFAULT;
> +    int interval;
> +    char zerobuf[WRITE_BLOCK_SIZE_DEFAULT];

Here there is a fixed size buffer which will be written...

>      char *buf = NULL;
>  
>      if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) {
> @@ -131,6 +140,12 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
>          goto cleanup;
>      }
>  
> +#ifdef __linux__
> +    if (ioctl(fd, BLKBSZGET, &wbytes) < 0) {
> +        wbytes = WRITE_BLOCK_SIZE_DEFAULT;
> +    }
> +#endif

...but the amount written is variable. So this will have an
out of bounds array access if the returned 'wbytes' is greater
than WRITE_BLOCK_SIZE_DEFAULT. I think you need to change
'zerobuf' to be a heap-allocated variable, instead of fixed
sized stack allocated variable. eg

     char *zerobuf;
     if (VIR_ALLOC_N(zerobuf, wbytes) < 0) {
         virReportOOMError();
         goto cleanup;
     }
     ....

> +
>      bzero(&zerobuf, sizeof(zerobuf));

VIR_ALLOC_N zeros, so this line could then be removed.

>  
>      if (VIR_ALLOC_N(buf, rbytes) < 0) {
> @@ -160,7 +175,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
>           * blocks */
>          amtleft = amtread;
>          do {
> -            int interval = ((512 > amtleft) ? amtleft : 512);
> +            interval = ((wbytes > amtleft) ? amtleft : wbytes);
>              int offset = amtread - amtleft;
>  
>              if (is_dest_file && memcmp(buf+offset, zerobuf, interval) == 0) {
> @@ -179,7 +194,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
>                  goto cleanup;
>  
>              }
> -        } while ((amtleft -= 512) > 0);
> +        } while ((amtleft -= interval) > 0);
>      }
>  
>      if (VIR_CLOSE(inputfd) < 0) {


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|