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@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 :|