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@xxxxxxxxxxxxxxxxx>
> ---
> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list