[libvirt] [PATCH v2] Fix performance problem of virStorageVolCreateXMLFrom()
Minoru Usui
usui at mxm.nes.nec.co.jp
Tue Mar 15 04:30:00 UTC 2011
Hi, Eric.
Thank you for merging my patch.
On Mon, 14 Mar 2011 21:04:26 -0600
Eric Blake <eblake at 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 at 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 at redhat.com +1-801-349-2682
> Libvirt virtualization library http://libvirt.org
>
--
Minoru Usui <usui at mxm.nes.nec.co.jp>
More information about the libvir-list
mailing list