[Qemu-devel] [PATCH 0/2] improve qemu-img conversion performance

[Qemu-devel] [PATCH 0/2] improve qemu-img conversion performance

[Yehuda Sadeh]

The following set of patches improve the qemu-img conversion process
performance. When using a higher latency backend, small writes have a
severe impact on the time it takes to do image conversion. 
We switch to using async writes, and we avoid splitting writes due to
holes when the holes are small enough.

Yehuda Sadeh (2):
  qemu-img: async write to block device when converting image
  qemu-img: don't skip writing small holes

 qemu-img.c |   34 +++++++++++++++++++++++++++-------
 1 files changed, 27 insertions(+), 7 deletions(-)

-- 
1.7.5.1

[Qemu-devel] [PATCH 1/2] qemu-img: async write to block device when converting image

[Yehuda Sadeh]

In order to improve image conversion process, instead of synchronously
writing the destingation image, we keep a window of async writes.

Signed-off-by: Yehuda Sadeh <yehuda@hq.newdream.net>
---
 qemu-img.c |   28 +++++++++++++++++++++++-----
 1 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index b205e98..0552746 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -622,6 +622,17 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
 }
 
 #define IO_BUF_SIZE (2 * 1024 * 1024)
+#define IO_WRITE_WINDOW_THRESHOLD (32 * 1024 * 1024)
+
+static int write_window = 0;
+
+static void img_write_cb(void *opaque, int ret)
+{
+    QEMUIOVector *qiov = (QEMUIOVector *)opaque;
+    write_window -=  qiov->iov->iov_len / 512;
+    qemu_iovec_destroy(qiov);    
+    qemu_free(qiov);
+}
 
 static int img_convert(int argc, char **argv)
 {
@@ -980,6 +991,9 @@ static int img_convert(int argc, char **argv)
                should add a specific call to have the info to go faster */
             buf1 = buf;
             while (n > 0) {
+                while (write_window > IO_WRITE_WINDOW_THRESHOLD / 512) {
+                    qemu_aio_wait();
+                }
                 /* If the output image is being created as a copy on write image,
                    copy all sectors even the ones containing only NUL bytes,
                    because they may differ from the sectors in the base image.
@@ -989,11 +1003,11 @@ static int img_convert(int argc, char **argv)
                    already there is garbage, not 0s. */
                 if (!has_zero_init || out_baseimg ||
                     is_allocated_sectors(buf1, n, &n1)) {
-                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
-                    if (ret < 0) {
-                        error_report("error while writing");
-                        goto out;
-                    }
+                    QEMUIOVector *qiov = qemu_mallocz(sizeof(QEMUIOVector));
+		    qemu_iovec_init(qiov, 1);
+		    qemu_iovec_add(qiov, (void *)buf1, n1 * 512);
+                    bdrv_aio_writev(out_bs, sector_num, qiov, n1, img_write_cb, qiov);
+                    write_window += n1;
                 }
                 sector_num += n1;
                 n -= n1;
@@ -1001,11 +1015,15 @@ static int img_convert(int argc, char **argv)
             }
             qemu_progress_print(local_progress, 100);
         }
+        while (write_window > 0) {
+            qemu_aio_wait();
+        }
     }
 out:
     qemu_progress_end();
     free_option_parameters(create_options);
     free_option_parameters(param);
+    bdrv_flush(out_bs);
     qemu_free(buf);
     if (out_bs) {
         bdrv_delete(out_bs);
-- 
1.7.5.1

Re: [Qemu-devel] [PATCH 1/2] qemu-img: async write to block device when converting image

[Sage Weil]

On Wed, 7 Sep 2011, Yehuda Sadeh wrote:
> In order to improve image conversion process, instead of synchronously
> writing the destingation image, we keep a window of async writes.
> 
> Signed-off-by: Yehuda Sadeh <yehuda@hq.newdream.net>

Small fix below:

> ---
>  qemu-img.c |   28 +++++++++++++++++++++++-----
>  1 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index b205e98..0552746 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -622,6 +622,17 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
>  }
>  
>  #define IO_BUF_SIZE (2 * 1024 * 1024)
> +#define IO_WRITE_WINDOW_THRESHOLD (32 * 1024 * 1024)
> +
> +static int write_window = 0;
> +
> +static void img_write_cb(void *opaque, int ret)
> +{
> +    QEMUIOVector *qiov = (QEMUIOVector *)opaque;
> +    write_window -=  qiov->iov->iov_len / 512;
> +    qemu_iovec_destroy(qiov);    
> +    qemu_free(qiov);
> +}
>  
>  static int img_convert(int argc, char **argv)
>  {
> @@ -980,6 +991,9 @@ static int img_convert(int argc, char **argv)
>                 should add a specific call to have the info to go faster */
>              buf1 = buf;
>              while (n > 0) {
> +                while (write_window > IO_WRITE_WINDOW_THRESHOLD / 512) {
> +                    qemu_aio_wait();
> +                }
>                  /* If the output image is being created as a copy on write image,
>                     copy all sectors even the ones containing only NUL bytes,
>                     because they may differ from the sectors in the base image.
> @@ -989,11 +1003,11 @@ static int img_convert(int argc, char **argv)
>                     already there is garbage, not 0s. */
>                  if (!has_zero_init || out_baseimg ||
>                      is_allocated_sectors(buf1, n, &n1)) {
> -                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
> -                    if (ret < 0) {
> -                        error_report("error while writing");
> -                        goto out;
> -                    }
> +                    QEMUIOVector *qiov = qemu_mallocz(sizeof(QEMUIOVector));
> +		    qemu_iovec_init(qiov, 1);
> +		    qemu_iovec_add(qiov, (void *)buf1, n1 * 512);
> +                    bdrv_aio_writev(out_bs, sector_num, qiov, n1, img_write_cb, qiov);
> +                    write_window += n1;
>                  }
>                  sector_num += n1;
>                  n -= n1;
> @@ -1001,11 +1015,15 @@ static int img_convert(int argc, char **argv)
>              }
>              qemu_progress_print(local_progress, 100);
>          }
> +        while (write_window > 0) {
> +            qemu_aio_wait();
> +        }
>      }
>  out:
>      qemu_progress_end();
>      free_option_parameters(create_options);
>      free_option_parameters(param);
> +    bdrv_flush(out_bs);
>      qemu_free(buf);
>      if (out_bs) {
>          bdrv_delete(out_bs);

The bdrv_flush() needs to go inside the if or else we get a null 
dereference on error (e.g. from a bad image name).

sage


> -- 
> 1.7.5.1
> 
> 
> 

[Qemu-devel] [PATCH 2/2] qemu-img: don't skip writing small holes

[Yehuda Sadeh]

When doing convert, we check that the sectors that are written
are not empty. When holes are small, and interleaved with data
it can lead to a significant performance issue.

Signed-off-by: Yehuda Sadeh <yehuda@hq.newdream.net>
---
 qemu-img.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 0552746..757fc3a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -623,6 +623,7 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
 
 #define IO_BUF_SIZE (2 * 1024 * 1024)
 #define IO_WRITE_WINDOW_THRESHOLD (32 * 1024 * 1024)
+#define IO_WRITE_MIN_SIZE (128 * 1024)
 
 static int write_window = 0;
 
@@ -991,6 +992,7 @@ static int img_convert(int argc, char **argv)
                should add a specific call to have the info to go faster */
             buf1 = buf;
             while (n > 0) {
+                int is_allocated = is_allocated_sectors(buf1, n, &n1);
                 while (write_window > IO_WRITE_WINDOW_THRESHOLD / 512) {
                     qemu_aio_wait();
                 }
@@ -1001,8 +1003,8 @@ static int img_convert(int argc, char **argv)
                    If the output is to a host device, we also write out
                    sectors that are entirely 0, since whatever data was
                    already there is garbage, not 0s. */
-                if (!has_zero_init || out_baseimg ||
-                    is_allocated_sectors(buf1, n, &n1)) {
+                if (is_allocated || n != n1 || !has_zero_init || out_baseimg) {
+                    n1 = MAX(n1, MIN(n, IO_WRITE_MIN_SIZE / 512));
                     QEMUIOVector *qiov = qemu_mallocz(sizeof(QEMUIOVector));
 		    qemu_iovec_init(qiov, 1);
 		    qemu_iovec_add(qiov, (void *)buf1, n1 * 512);
-- 
1.7.5.1

Re: [Qemu-devel] [PATCH 0/2] improve qemu-img conversion performance

[Stefan Hajnoczi]

On Wed, Sep 07, 2011 at 04:06:51PM -0700, Yehuda Sadeh wrote:
> The following set of patches improve the qemu-img conversion process
> performance. When using a higher latency backend, small writes have a
> severe impact on the time it takes to do image conversion. 
> We switch to using async writes, and we avoid splitting writes due to
> holes when the holes are small enough.
> 
> Yehuda Sadeh (2):
>   qemu-img: async write to block device when converting image
>   qemu-img: don't skip writing small holes
> 
>  qemu-img.c |   34 +++++++++++++++++++++++++++-------
>  1 files changed, 27 insertions(+), 7 deletions(-)
> 
> -- 
> 2.7.5.1

This has nothing to do with the patch itself, but I've been curious
about the existence of both a QEMU and a Linux kernel rbd block driver.

The I/O latency with qemu-img has been an issue for rbd users.  But they
have the option of using the Linux kernel rbd block driver, where
qemu-img can take advantage of the page cache instead of performing
direct I/O.

Does this mean you intend to support both QEMU block/rbd.c and Linux
drivers/block/rbd.c?  As a user I would go with the Linux kernel driver
instead of the QEMU block driver because it offers page cache and host
block device features.  On the other hand a userspace driver is nice
because it does not require privileges.

Stefan

Re: [Qemu-devel] [PATCH 0/2] improve qemu-img conversion performance

[Sage Weil]

On Thu, 8 Sep 2011, Stefan Hajnoczi wrote:
> On Wed, Sep 07, 2011 at 04:06:51PM -0700, Yehuda Sadeh wrote:
> > The following set of patches improve the qemu-img conversion process
> > performance. When using a higher latency backend, small writes have a
> > severe impact on the time it takes to do image conversion. 
> > We switch to using async writes, and we avoid splitting writes due to
> > holes when the holes are small enough.
> > 
> > Yehuda Sadeh (2):
> >   qemu-img: async write to block device when converting image
> >   qemu-img: don't skip writing small holes
> > 
> >  qemu-img.c |   34 +++++++++++++++++++++++++++-------
> >  1 files changed, 27 insertions(+), 7 deletions(-)
> > 
> > -- 
> > 2.7.5.1
> 
> This has nothing to do with the patch itself, but I've been curious
> about the existence of both a QEMU and a Linux kernel rbd block driver.
> 
> The I/O latency with qemu-img has been an issue for rbd users.  But they
> have the option of using the Linux kernel rbd block driver, where
> qemu-img can take advantage of the page cache instead of performing
> direct I/O.
>
> Does this mean you intend to support both QEMU block/rbd.c and Linux
> drivers/block/rbd.c?  As a user I would go with the Linux kernel driver
> instead of the QEMU block driver because it offers page cache and host
> block device features.  On the other hand a userspace driver is nice
> because it does not require privileges.

We intend to support both drivers, yes.  The native qemu driver is 
generally more convenient because there is no kernel dependency, so we 
want to make qemu-img perform reasonably one way or another.

There are plans to implement some limited buffering (and flush) in librbd 
to make the device behave a bit more like a disk with a cache.  That will 
mask the sync write latency, but I suspect that doing these writes using 
the aio interface (and ignoring small holes) will help everyone...

sage

Re: [Qemu-devel] [PATCH 0/2] improve qemu-img conversion performance

[Kevin Wolf]

Am 08.09.2011 01:06, schrieb Yehuda Sadeh:
> The following set of patches improve the qemu-img conversion process
> performance. When using a higher latency backend, small writes have a
> severe impact on the time it takes to do image conversion. 
> We switch to using async writes, and we avoid splitting writes due to
> holes when the holes are small enough.
> 
> Yehuda Sadeh (2):
>   qemu-img: async write to block device when converting image
>   qemu-img: don't skip writing small holes
> 
>  qemu-img.c |   34 +++++++++++++++++++++++++++-------
>  1 files changed, 27 insertions(+), 7 deletions(-)
> 

This doesn't seem to be against git master or the block tree. Please rebase.

I think that commit a22f123c may obsolete your patch 2/2.

Kevin

Re: [Qemu-devel] [PATCH 0/2] improve qemu-img conversion performance

[Sage Weil]

On Thu, 8 Sep 2011, Kevin Wolf wrote:
> Am 08.09.2011 01:06, schrieb Yehuda Sadeh:
> > The following set of patches improve the qemu-img conversion process
> > performance. When using a higher latency backend, small writes have a
> > severe impact on the time it takes to do image conversion. 
> > We switch to using async writes, and we avoid splitting writes due to
> > holes when the holes are small enough.
> > 
> > Yehuda Sadeh (2):
> >   qemu-img: async write to block device when converting image
> >   qemu-img: don't skip writing small holes
> > 
> >  qemu-img.c |   34 +++++++++++++++++++++++++++-------
> >  1 files changed, 27 insertions(+), 7 deletions(-)
> > 
> 
> This doesn't seem to be against git master or the block tree. Please rebase.
> 
> I think that commit a22f123c may obsolete your patch 2/2.

With git.kernel.org down, where should I be looking for the latest 
upstream?

Thanks!
sage

Re: [Qemu-devel] [PATCH 0/2] improve qemu-img conversion performance

[Kevin Wolf]

Am 08.09.2011 18:36, schrieb Sage Weil:
> On Thu, 8 Sep 2011, Kevin Wolf wrote:
>> Am 08.09.2011 01:06, schrieb Yehuda Sadeh:
>>> The following set of patches improve the qemu-img conversion process
>>> performance. When using a higher latency backend, small writes have a
>>> severe impact on the time it takes to do image conversion. 
>>> We switch to using async writes, and we avoid splitting writes due to
>>> holes when the holes are small enough.
>>>
>>> Yehuda Sadeh (2):
>>>   qemu-img: async write to block device when converting image
>>>   qemu-img: don't skip writing small holes
>>>
>>>  qemu-img.c |   34 +++++++++++++++++++++++++++-------
>>>  1 files changed, 27 insertions(+), 7 deletions(-)
>>>
>>
>> This doesn't seem to be against git master or the block tree. Please rebase.
>>
>> I think that commit a22f123c may obsolete your patch 2/2.
> 
> With git.kernel.org down, where should I be looking for the latest 
> upstream?

qemu has never been on kernel.org. The interesting repositories for you are:

* Upstream: git://git.qemu.org/qemu.git master
* Block development branch: git://repo.or.cz/qemu/kevin.git block

Kevin

Re: [Qemu-devel] [PATCH 0/2] improve qemu-img conversion performance

[Sage Weil]

On Fri, 9 Sep 2011, Kevin Wolf wrote:
> Am 08.09.2011 18:36, schrieb Sage Weil:
> > On Thu, 8 Sep 2011, Kevin Wolf wrote:
> >> Am 08.09.2011 01:06, schrieb Yehuda Sadeh:
> >>> The following set of patches improve the qemu-img conversion process
> >>> performance. When using a higher latency backend, small writes have a
> >>> severe impact on the time it takes to do image conversion. 
> >>> We switch to using async writes, and we avoid splitting writes due to
> >>> holes when the holes are small enough.
> >>>
> >>> Yehuda Sadeh (2):
> >>>   qemu-img: async write to block device when converting image
> >>>   qemu-img: don't skip writing small holes
> >>>
> >>>  qemu-img.c |   34 +++++++++++++++++++++++++++-------
> >>>  1 files changed, 27 insertions(+), 7 deletions(-)
> >>>
> >>
> >> This doesn't seem to be against git master or the block tree. Please rebase.
> >>
> >> I think that commit a22f123c may obsolete your patch 2/2.
> > 
> > With git.kernel.org down, where should I be looking for the latest 
> > upstream?
> 
> qemu has never been on kernel.org. The interesting repositories for you are:
> 
> * Upstream: git://git.qemu.org/qemu.git master
> * Block development branch: git://repo.or.cz/qemu/kevin.git block

Oh right.  I've been working from qemu-kvm.git.  

I've done some (still minimal) testing, and it looks like the combination 
of a22f123c and the new writeback/flush stuff in librbd gets the same 
result as doing async io explicitly from qemu-img.c.  Want to take a look, 
Yehuda?  It still defaults to off, so you'll need to add 
rbd_writeback_window=8000000 or similar to the rbd device string.

Thanks!
sage

Re: [Qemu-devel] [PATCH 0/2] improve qemu-img conversion performance

[Yehuda Sadeh Weinraub]

On Sun, Sep 11, 2011 at 8:14 PM, Sage Weil <sage@newdream.net> wrote:
> On Fri, 9 Sep 2011, Kevin Wolf wrote:
>> Am 08.09.2011 18:36, schrieb Sage Weil:
>> > On Thu, 8 Sep 2011, Kevin Wolf wrote:
>> >> Am 08.09.2011 01:06, schrieb Yehuda Sadeh:
>> >>> The following set of patches improve the qemu-img conversion process
>> >>> performance. When using a higher latency backend, small writes have a
>> >>> severe impact on the time it takes to do image conversion.
>> >>> We switch to using async writes, and we avoid splitting writes due to
>> >>> holes when the holes are small enough.
>> >>>
>> >>> Yehuda Sadeh (2):
>> >>>   qemu-img: async write to block device when converting image
>> >>>   qemu-img: don't skip writing small holes
>> >>>
>> >>>  qemu-img.c |   34 +++++++++++++++++++++++++++-------
>> >>>  1 files changed, 27 insertions(+), 7 deletions(-)
>> >>>
>> >>
>> >> This doesn't seem to be against git master or the block tree. Please rebase.
>> >>
>> >> I think that commit a22f123c may obsolete your patch 2/2.
>> >
>> > With git.kernel.org down, where should I be looking for the latest
>> > upstream?
>>
>> qemu has never been on kernel.org. The interesting repositories for you are:
>>
>> * Upstream: git://git.qemu.org/qemu.git master
>> * Block development branch: git://repo.or.cz/qemu/kevin.git block
>
> Oh right.  I've been working from qemu-kvm.git.
>
> I've done some (still minimal) testing, and it looks like the combination
> of a22f123c and the new writeback/flush stuff in librbd gets the same
> result as doing async io explicitly from qemu-img.c.  Want to take a look,
> Yehuda?  It still defaults to off, so you'll need to add
> rbd_writeback_window=8000000 or similar to the rbd device string.
>

I'll take a look. I do have a rebased version for the qemu-img async
patch, and I think qemu can benefit from that anyway.

Yehuda

Re: [Qemu-devel] [PATCH 0/2] improve qemu-img conversion performance

[Yehuda Sadeh Weinraub]

On Sun, Sep 11, 2011 at 8:17 PM, Yehuda Sadeh Weinraub
<yehudasa@gmail.com> wrote:
> On Sun, Sep 11, 2011 at 8:14 PM, Sage Weil <sage@newdream.net> wrote:
>> On Fri, 9 Sep 2011, Kevin Wolf wrote:
>>> Am 08.09.2011 18:36, schrieb Sage Weil:
>>> > On Thu, 8 Sep 2011, Kevin Wolf wrote:
>>> >> Am 08.09.2011 01:06, schrieb Yehuda Sadeh:
>>> >>> The following set of patches improve the qemu-img conversion process
>>> >>> performance. When using a higher latency backend, small writes have a
>>> >>> severe impact on the time it takes to do image conversion.
>>> >>> We switch to using async writes, and we avoid splitting writes due to
>>> >>> holes when the holes are small enough.
>>> >>>
>>> >>> Yehuda Sadeh (2):
>>> >>>   qemu-img: async write to block device when converting image
>>> >>>   qemu-img: don't skip writing small holes
>>> >>>
>>> >>>  qemu-img.c |   34 +++++++++++++++++++++++++++-------
>>> >>>  1 files changed, 27 insertions(+), 7 deletions(-)
>>> >>>
>>> >>
>>> >> This doesn't seem to be against git master or the block tree. Please rebase.
>>> >>
>>> >> I think that commit a22f123c may obsolete your patch 2/2.
>>> >
>>> > With git.kernel.org down, where should I be looking for the latest
>>> > upstream?
>>>
>>> qemu has never been on kernel.org. The interesting repositories for you are:
>>>
>>> * Upstream: git://git.qemu.org/qemu.git master
>>> * Block development branch: git://repo.or.cz/qemu/kevin.git block
>>
>> Oh right.  I've been working from qemu-kvm.git.
>>
>> I've done some (still minimal) testing, and it looks like the combination
>> of a22f123c and the new writeback/flush stuff in librbd gets the same
>> result as doing async io explicitly from qemu-img.c.  Want to take a look,
>> Yehuda?  It still defaults to off, so you'll need to add
>> rbd_writeback_window=8000000 or similar to the rbd device string.
>>
>
> I'll take a look. I do have a rebased version for the qemu-img async
> patch, and I think qemu can benefit from that anyway.
>
I tested latest librbd with 8k rbd_writeback_window against Kevin's
block branch and it seems that the conversion performance surpasses
what I had seen with my qemu-img changes.

Yehuda

Re: [Qemu-devel] [PATCH 0/2] improve qemu-img conversion performance

[Kevin Wolf]

Am 12.09.2011 05:17, schrieb Yehuda Sadeh Weinraub:
> On Sun, Sep 11, 2011 at 8:14 PM, Sage Weil <sage@newdream.net> wrote:
>> On Fri, 9 Sep 2011, Kevin Wolf wrote:
>>> Am 08.09.2011 18:36, schrieb Sage Weil:
>>>> On Thu, 8 Sep 2011, Kevin Wolf wrote:
>>>>> Am 08.09.2011 01:06, schrieb Yehuda Sadeh:
>>>>>> The following set of patches improve the qemu-img conversion process
>>>>>> performance. When using a higher latency backend, small writes have a
>>>>>> severe impact on the time it takes to do image conversion.
>>>>>> We switch to using async writes, and we avoid splitting writes due to
>>>>>> holes when the holes are small enough.
>>>>>>
>>>>>> Yehuda Sadeh (2):
>>>>>>   qemu-img: async write to block device when converting image
>>>>>>   qemu-img: don't skip writing small holes
>>>>>>
>>>>>>  qemu-img.c |   34 +++++++++++++++++++++++++++-------
>>>>>>  1 files changed, 27 insertions(+), 7 deletions(-)
>>>>>>
>>>>>
>>>>> This doesn't seem to be against git master or the block tree. Please rebase.
>>>>>
>>>>> I think that commit a22f123c may obsolete your patch 2/2.
>>>>
>>>> With git.kernel.org down, where should I be looking for the latest
>>>> upstream?
>>>
>>> qemu has never been on kernel.org. The interesting repositories for you are:
>>>
>>> * Upstream: git://git.qemu.org/qemu.git master
>>> * Block development branch: git://repo.or.cz/qemu/kevin.git block
>>
>> Oh right.  I've been working from qemu-kvm.git.
>>
>> I've done some (still minimal) testing, and it looks like the combination
>> of a22f123c and the new writeback/flush stuff in librbd gets the same
>> result as doing async io explicitly from qemu-img.c.  Want to take a look,
>> Yehuda?  It still defaults to off, so you'll need to add
>> rbd_writeback_window=8000000 or similar to the rbd device string.
>>
> 
> I'll take a look. I do have a rebased version for the qemu-img async
> patch, and I think qemu can benefit from that anyway.

Yes, I agree that the change makes sense anyway.

Kevin