[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] Fix performance problem of virStorageVolCreateXMLFrom()



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]