[libvirt] [PATCH v2] Fix performance problem of virStorageVolCreateXMLFrom()
This patch changes zerobuf variable from array to VIR_ALLOC_N(). Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- src/storage/storage_backend.c | 38 +++++++++++++++++++++++++++++--------- 1 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2eede74..3a22da4 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 bytes = 1024 * 1024; - char zerobuf[512]; + size_t rbytes = READ_BLOCK_SIZE_DEFAULT; + size_t wbytes = WRITE_BLOCK_SIZE_DEFAULT; + int interval; + char *zerobuf; char *buf = NULL; if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { @@ -131,9 +140,19 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, goto cleanup; } - bzero(&zerobuf, sizeof(zerobuf)); +#ifdef __linux__ + if (ioctl(fd, BLKBSZGET, &wbytes) < 0) { + wbytes = WRITE_BLOCK_SIZE_DEFAULT; + } +#endif + + if (VIR_ALLOC_N(zerobuf, wbytes) < 0) { + ret = -errno; + virReportOOMError(); + goto cleanup; + } - if (VIR_ALLOC_N(buf, bytes) < 0) { + if (VIR_ALLOC_N(buf, rbytes) < 0) { ret = -errno; virReportOOMError(); goto cleanup; @@ -144,10 +163,10 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, while (amtread != 0) { int amtleft; - if (remain < bytes) - bytes = remain; + if (remain < rbytes) + rbytes = remain; - if ((amtread = saferead(inputfd, buf, bytes)) < 0) { + if ((amtread = saferead(inputfd, buf, rbytes)) < 0) { ret = -errno; virReportSystemError(errno, _("failed reading from file '%s'"), @@ -160,7 +179,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 +198,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, goto cleanup; } - } while ((amtleft -= 512) > 0); + } while ((amtleft -= interval) > 0); } if (VIR_CLOSE(inputfd) < 0) { @@ -196,6 +215,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, cleanup: VIR_FORCE_CLOSE(inputfd); + VIR_FREE(zerobuf); VIR_FREE(buf); return ret; -- Minoru Usui <usui@mxm.nes.nec.co.jp>
On 03/08/2011 04:06 AM, Minoru Usui wrote:
This patch changes zerobuf variable from array to VIR_ALLOC_N().
Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp>
Thanks for contributing! This looks like your first patch in, so I updated AUTHORS; let me know if I need to adjust any spelling.
--- src/storage/storage_backend.c | 38 +++++++++++++++++++++++++++++--------- 1 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2eede74..3a22da4 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
This needs indentation tweaking to keep 'make syntax-check' happy when cppi is installed.
+#define READ_BLOCK_SIZE_DEFAULT (1024 * 1024) +#define WRITE_BLOCK_SIZE_DEFAULT (512)
Technically, you don't need () around a single integer constant, but it didn't hurt to leave them in. Why isn't this 4k, though?
+#ifdef __linux__ + if (ioctl(fd, BLKBSZGET, &wbytes) < 0) { + wbytes = WRITE_BLOCK_SIZE_DEFAULT; + } +#endif
What's wrong with fstat(fd) and using st.st_blksize on all other platforms where the ioctl doesn't exist? That may still be too small on some platforms, but it's more likely to be write than hard-coding things at 512 (for example, on Cygwin, st.st_blksize is 64k, which is indeed the optimal size for NTFS drives). Also, I don't know of any file system that supports sparse files with holes smaller than st_blksize. Here's what I squashed in before pushing: diff --git i/AUTHORS w/AUTHORS index c0c1780..0e56468 100644 --- i/AUTHORS +++ w/AUTHORS diff --git i/src/storage/storage_backend.c w/src/storage/storage_backend.c index b3eba10..fc08c68 100644 --- i/src/storage/storage_backend.c +++ w/src/storage/storage_backend.c @@ -1,7 +1,7 @@ /* * storage_backend.c: internal storage driver backend contract * - * Copyright (C) 2007-2010 Red Hat, Inc. + * Copyright (C) 2007-2011 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -37,8 +37,8 @@ #include <sys/param.h> #include <dirent.h> #ifdef __linux__ -#include <sys/ioctl.h> -#include <linux/fs.h> +# include <sys/ioctl.h> +# include <linux/fs.h> #endif #if HAVE_SELINUX @@ -113,7 +113,7 @@ enum { }; #define READ_BLOCK_SIZE_DEFAULT (1024 * 1024) -#define WRITE_BLOCK_SIZE_DEFAULT (512) +#define WRITE_BLOCK_SIZE_DEFAULT (4 * 1024) static int ATTRIBUTE_NONNULL (2) virStorageBackendCopyToFD(virStorageVolDefPtr vol, @@ -127,10 +127,11 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, int ret = 0; unsigned long long remain; size_t rbytes = READ_BLOCK_SIZE_DEFAULT; - size_t wbytes = WRITE_BLOCK_SIZE_DEFAULT; + size_t wbytes = 0; int interval; char *zerobuf; char *buf = NULL; + struct stat st; if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { ret = -errno; @@ -142,9 +143,13 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, #ifdef __linux__ if (ioctl(fd, BLKBSZGET, &wbytes) < 0) { - wbytes = WRITE_BLOCK_SIZE_DEFAULT; + wbytes = 0; } #endif + if ((wbytes == 0) && fstat(fd, &st) == 0) + wbytes = st.st_blksize; + if (wbytes < WRITE_BLOCK_SIZE_DEFAULT) + wbytes = WRITE_BLOCK_SIZE_DEFAULT; if (VIR_ALLOC_N(zerobuf, wbytes) < 0) { ret = -errno; -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Hi, Eric. Thank you for merging my patch. On Mon, 14 Mar 2011 21:04:26 -0600 Eric Blake <eblake@redhat.com> wrote:
On 03/08/2011 04:06 AM, Minoru Usui wrote:
This patch changes zerobuf variable from array to VIR_ALLOC_N().
Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp>
Thanks for contributing! This looks like your first patch in, so I updated AUTHORS; let me know if I need to adjust any spelling.
Thanks for updating AUTHORS. Above spelling of Signed-off-by is correct. But..., I sent another patch to you and libvirt-devel before. Please check it, if you have any time. http://www.mail-archive.com/libvir-list@redhat.com/msg32801.html
--- src/storage/storage_backend.c | 38 +++++++++++++++++++++++++++++--------- 1 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2eede74..3a22da4 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
This needs indentation tweaking to keep 'make syntax-check' happy when cppi is installed.
Thank you for telling me about it. I agree.
+#define READ_BLOCK_SIZE_DEFAULT (1024 * 1024) +#define WRITE_BLOCK_SIZE_DEFAULT (512)
Technically, you don't need () around a single integer constant, but it didn't hurt to leave them in. Why isn't this 4k, though?
I don't know 4k is appropriate value for all platform. So I don't change 512. If 4k is best, I think default value changes from 512 to 4k, too.
+#ifdef __linux__ + if (ioctl(fd, BLKBSZGET, &wbytes) < 0) { + wbytes = WRITE_BLOCK_SIZE_DEFAULT; + } +#endif
What's wrong with fstat(fd) and using st.st_blksize on all other platforms where the ioctl doesn't exist? That may still be too small on some platforms, but it's more likely to be write than hard-coding things at 512 (for example, on Cygwin, st.st_blksize is 64k, which is indeed the optimal size for NTFS drives). Also, I don't know of any file system that supports sparse files with holes smaller than st_blksize.
I agree with you. Your code looks good for me.
Here's what I squashed in before pushing:
diff --git i/AUTHORS w/AUTHORS index c0c1780..0e56468 100644 --- i/AUTHORS +++ w/AUTHORS diff --git i/src/storage/storage_backend.c w/src/storage/storage_backend.c index b3eba10..fc08c68 100644 --- i/src/storage/storage_backend.c +++ w/src/storage/storage_backend.c @@ -1,7 +1,7 @@ /* * storage_backend.c: internal storage driver backend contract * - * Copyright (C) 2007-2010 Red Hat, Inc. + * Copyright (C) 2007-2011 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -37,8 +37,8 @@ #include <sys/param.h> #include <dirent.h> #ifdef __linux__ -#include <sys/ioctl.h> -#include <linux/fs.h> +# include <sys/ioctl.h> +# include <linux/fs.h> #endif
#if HAVE_SELINUX @@ -113,7 +113,7 @@ enum { };
#define READ_BLOCK_SIZE_DEFAULT (1024 * 1024) -#define WRITE_BLOCK_SIZE_DEFAULT (512) +#define WRITE_BLOCK_SIZE_DEFAULT (4 * 1024)
static int ATTRIBUTE_NONNULL (2) virStorageBackendCopyToFD(virStorageVolDefPtr vol, @@ -127,10 +127,11 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, int ret = 0; unsigned long long remain; size_t rbytes = READ_BLOCK_SIZE_DEFAULT; - size_t wbytes = WRITE_BLOCK_SIZE_DEFAULT; + size_t wbytes = 0; int interval; char *zerobuf; char *buf = NULL; + struct stat st;
if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { ret = -errno; @@ -142,9 +143,13 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
#ifdef __linux__ if (ioctl(fd, BLKBSZGET, &wbytes) < 0) { - wbytes = WRITE_BLOCK_SIZE_DEFAULT; + wbytes = 0; } #endif + if ((wbytes == 0) && fstat(fd, &st) == 0) + wbytes = st.st_blksize; + if (wbytes < WRITE_BLOCK_SIZE_DEFAULT) + wbytes = WRITE_BLOCK_SIZE_DEFAULT;
if (VIR_ALLOC_N(zerobuf, wbytes) < 0) { ret = -errno;
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
-- Minoru Usui <usui@mxm.nes.nec.co.jp>
participants (2)
-
Eric Blake -
Minoru Usui