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

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



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>


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