QEMU RBD is slow with QCOW2 images

QEMU RBD is slow with QCOW2 images

[Stefano Garzarella]

Hi Jason,
as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD 
writing data is very slow compared to a raw file.

Comparing raw vs QCOW2 image creation with RBD I found that we use a 
different object size, for the raw file I see '4 MiB objects', for QCOW2 
I see '64 KiB objects' as reported on comment 14 [2].
This should be the main issue of slowness, indeed forcing in the code 4 
MiB object size also for QCOW2 increased the speed a lot.

Looking better I discovered that for raw files, we call rbd_create() 
with obj_order = 0 (if 'cluster_size' options is not defined), so the 
default object size is used.
Instead for QCOW2, we use obj_order = 16, since the default 
'cluster_size' defined for QCOW2, is 64 KiB.

Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster 
size, since in qcow2_co_create_opts() we remove the 'cluster_size' from 
QemuOpts calling qemu_opts_to_qdict_filtered().
For some reason that I have yet to understand, after this deletion, 
however remains in QemuOpts the default value of 'cluster_size' for 
qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts()

At this point my doubts are:
Does it make sense to use the same cluster_size as qcow2 as object_size 
in RBD?
If we want to keep the 2 options separated, how can it be done? Should 
we rename the option in block/rbd.c?

Thanks,
Stefano

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1744525
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1744525#c14

Re: QEMU RBD is slow with QCOW2 images

[Jason Dillaman]

On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> Hi Jason,
> as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
> writing data is very slow compared to a raw file.
>
> Comparing raw vs QCOW2 image creation with RBD I found that we use a
> different object size, for the raw file I see '4 MiB objects', for QCOW2
> I see '64 KiB objects' as reported on comment 14 [2].
> This should be the main issue of slowness, indeed forcing in the code 4
> MiB object size also for QCOW2 increased the speed a lot.
>
> Looking better I discovered that for raw files, we call rbd_create()
> with obj_order = 0 (if 'cluster_size' options is not defined), so the
> default object size is used.
> Instead for QCOW2, we use obj_order = 16, since the default
> 'cluster_size' defined for QCOW2, is 64 KiB.
>
> Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
> size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
> QemuOpts calling qemu_opts_to_qdict_filtered().
> For some reason that I have yet to understand, after this deletion,
> however remains in QemuOpts the default value of 'cluster_size' for
> qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts()
>
> At this point my doubts are:
> Does it make sense to use the same cluster_size as qcow2 as object_size
> in RBD?

No, not really. But it also doesn't really make any sense to put a
QCOW2 image within an RBD image. To clarify from the BZ, OpenStack
does not put QCOW2 images on RBD, it converts QCOW2 images into raw
images to store in RBD.

> If we want to keep the 2 options separated, how can it be done? Should
> we rename the option in block/rbd.c?

You can already pass overrides to the RBD block driver by just
appending them after the
"rbd:<filename>[:option1=value1[:option2=value2]]" portion, perhaps
that could be re-used.

> Thanks,
> Stefano
>
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1744525
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1744525#c14
>


-- 
Jason

Re: QEMU RBD is slow with QCOW2 images

[Peter Lieven]

Am 03.03.21 um 19:47 schrieb Jason Dillaman:
> On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> Hi Jason,
>> as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
>> writing data is very slow compared to a raw file.
>>
>> Comparing raw vs QCOW2 image creation with RBD I found that we use a
>> different object size, for the raw file I see '4 MiB objects', for QCOW2
>> I see '64 KiB objects' as reported on comment 14 [2].
>> This should be the main issue of slowness, indeed forcing in the code 4
>> MiB object size also for QCOW2 increased the speed a lot.
>>
>> Looking better I discovered that for raw files, we call rbd_create()
>> with obj_order = 0 (if 'cluster_size' options is not defined), so the
>> default object size is used.
>> Instead for QCOW2, we use obj_order = 16, since the default
>> 'cluster_size' defined for QCOW2, is 64 KiB.
>>
>> Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
>> size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
>> QemuOpts calling qemu_opts_to_qdict_filtered().
>> For some reason that I have yet to understand, after this deletion,
>> however remains in QemuOpts the default value of 'cluster_size' for
>> qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts()
>>
>> At this point my doubts are:
>> Does it make sense to use the same cluster_size as qcow2 as object_size
>> in RBD?
> No, not really. But it also doesn't really make any sense to put a
> QCOW2 image within an RBD image. To clarify from the BZ, OpenStack
> does not put QCOW2 images on RBD, it converts QCOW2 images into raw
> images to store in RBD.


As discussed earlier the only reasonable format for rbd image is raw.

What is the idea behind putting a qcow2 on an rbd pool?

Jason and I even discussed shortly durign the review of the rbd driver rewrite I posted

earlier if it was ok to drop support for writing past the end of file.


Anyway the reason why it is so slow is that write requests serialize if the

qcow2 file grows. If there is a sane reason why we need qcow2 on rbd

we need to implement at least preallocation mode = full to overcome

the serialization.


Peter

Re: QEMU RBD is slow with QCOW2 images

[Stefano Garzarella]

On Wed, Mar 03, 2021 at 01:47:06PM -0500, Jason Dillaman wrote:
>On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> Hi Jason,
>> as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
>> writing data is very slow compared to a raw file.
>>
>> Comparing raw vs QCOW2 image creation with RBD I found that we use a
>> different object size, for the raw file I see '4 MiB objects', for QCOW2
>> I see '64 KiB objects' as reported on comment 14 [2].
>> This should be the main issue of slowness, indeed forcing in the code 4
>> MiB object size also for QCOW2 increased the speed a lot.
>>
>> Looking better I discovered that for raw files, we call rbd_create()
>> with obj_order = 0 (if 'cluster_size' options is not defined), so the
>> default object size is used.
>> Instead for QCOW2, we use obj_order = 16, since the default
>> 'cluster_size' defined for QCOW2, is 64 KiB.
>>
>> Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
>> size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
>> QemuOpts calling qemu_opts_to_qdict_filtered().
>> For some reason that I have yet to understand, after this deletion,
>> however remains in QemuOpts the default value of 'cluster_size' for
>> qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts()
>>
>> At this point my doubts are:
>> Does it make sense to use the same cluster_size as qcow2 as object_size
>> in RBD?
>
>No, not really. But it also doesn't really make any sense to put a
>QCOW2 image within an RBD image. To clarify from the BZ, OpenStack
>does not put QCOW2 images on RBD, it converts QCOW2 images into raw
>images to store in RBD.

Yes, that was my doubt, thanks for the confirmation.

Also Daniel (+CC) confirmed me the same thing, but just to be complete 
he added that there is a case where OpenStack could use qcow2 on RBD, 
but in this case using in-kernel RBD, so the QEMU RBD is not involved.

>
>> If we want to keep the 2 options separated, how can it be done? Should
>> we rename the option in block/rbd.c?
>
>You can already pass overrides to the RBD block driver by just
>appending them after the
>"rbd:<filename>[:option1=value1[:option2=value2]]" portion, perhaps
>that could be re-used.

I see, we should extend qemu_rbd_parse_filename() to suppurt it.

Maybe if we don't want to support this configuration, we should print 
some warning messages.

Thanks,
Stefano

Re: QEMU RBD is slow with QCOW2 images

[Stefano Garzarella]

On Wed, Mar 03, 2021 at 10:26:12PM +0100, Peter Lieven wrote:
>Am 03.03.21 um 19:47 schrieb Jason Dillaman:
>> On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>> Hi Jason,
>>> as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
>>> writing data is very slow compared to a raw file.
>>>
>>> Comparing raw vs QCOW2 image creation with RBD I found that we use a
>>> different object size, for the raw file I see '4 MiB objects', for QCOW2
>>> I see '64 KiB objects' as reported on comment 14 [2].
>>> This should be the main issue of slowness, indeed forcing in the code 4
>>> MiB object size also for QCOW2 increased the speed a lot.
>>>
>>> Looking better I discovered that for raw files, we call rbd_create()
>>> with obj_order = 0 (if 'cluster_size' options is not defined), so the
>>> default object size is used.
>>> Instead for QCOW2, we use obj_order = 16, since the default
>>> 'cluster_size' defined for QCOW2, is 64 KiB.
>>>
>>> Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
>>> size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
>>> QemuOpts calling qemu_opts_to_qdict_filtered().
>>> For some reason that I have yet to understand, after this deletion,
>>> however remains in QemuOpts the default value of 'cluster_size' for
>>> qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts()
>>>
>>> At this point my doubts are:
>>> Does it make sense to use the same cluster_size as qcow2 as object_size
>>> in RBD?
>> No, not really. But it also doesn't really make any sense to put a
>> QCOW2 image within an RBD image. To clarify from the BZ, OpenStack
>> does not put QCOW2 images on RBD, it converts QCOW2 images into raw
>> images to store in RBD.
>
>
>As discussed earlier the only reasonable format for rbd image is raw.
>What is the idea behind putting a qcow2 on an rbd pool?
>Jason and I even discussed shortly durign the review of the rbd driver 
>rewrite I posted
>earlier if it was ok to drop support for writing past the end of file.
>
>Anyway the reason why it is so slow is that write requests serialize if the
>qcow2 file grows. If there is a sane reason why we need qcow2 on rbd
>we need to implement at least preallocation mode = full to overcome
>the serialization.

Agree, at most we could deprecate it by printing a message and then 
remove the support in future releases.

Thanks,
Stefano

Re: QEMU RBD is slow with QCOW2 images

[Daniel P. Berrangé]

On Thu, Mar 04, 2021 at 09:55:40AM +0100, Stefano Garzarella wrote:
> On Wed, Mar 03, 2021 at 01:47:06PM -0500, Jason Dillaman wrote:
> > On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > 
> > > Hi Jason,
> > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
> > > writing data is very slow compared to a raw file.
> > > 
> > > Comparing raw vs QCOW2 image creation with RBD I found that we use a
> > > different object size, for the raw file I see '4 MiB objects', for QCOW2
> > > I see '64 KiB objects' as reported on comment 14 [2].
> > > This should be the main issue of slowness, indeed forcing in the code 4
> > > MiB object size also for QCOW2 increased the speed a lot.
> > > 
> > > Looking better I discovered that for raw files, we call rbd_create()
> > > with obj_order = 0 (if 'cluster_size' options is not defined), so the
> > > default object size is used.
> > > Instead for QCOW2, we use obj_order = 16, since the default
> > > 'cluster_size' defined for QCOW2, is 64 KiB.
> > > 
> > > Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
> > > size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
> > > QemuOpts calling qemu_opts_to_qdict_filtered().
> > > For some reason that I have yet to understand, after this deletion,
> > > however remains in QemuOpts the default value of 'cluster_size' for
> > > qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts()
> > > 
> > > At this point my doubts are:
> > > Does it make sense to use the same cluster_size as qcow2 as object_size
> > > in RBD?
> > 
> > No, not really. But it also doesn't really make any sense to put a
> > QCOW2 image within an RBD image. To clarify from the BZ, OpenStack
> > does not put QCOW2 images on RBD, it converts QCOW2 images into raw
> > images to store in RBD.
> 
> Yes, that was my doubt, thanks for the confirmation.
> 
> Also Daniel (+CC) confirmed me the same thing, but just to be complete he
> added that there is a case where OpenStack could use qcow2 on RBD, but in
> this case using in-kernel RBD, so the QEMU RBD is not involved.
> 
> > 
> > > If we want to keep the 2 options separated, how can it be done? Should
> > > we rename the option in block/rbd.c?
> > 
> > You can already pass overrides to the RBD block driver by just
> > appending them after the
> > "rbd:<filename>[:option1=value1[:option2=value2]]" portion, perhaps
> > that could be re-used.
> 
> I see, we should extend qemu_rbd_parse_filename() to suppurt it.

We shouldn't really be extending the legacy filename syntax.
If we need extra options we want them in the QAPI schema for
blockdev.

> Maybe if we don't want to support this configuration, we should print some
> warning messages.

Note these are separate layers in QEMU block layer. qcow2 is a format
driver, while RBD is a protocol driver. QEMU lets any format driver be
run on top of any protocol driver in general. In practice there are
certain combinations that are more sane and commonly used than others,
but QEMU doesn't document this or assign support level to pairing
right now.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: QEMU RBD is slow with QCOW2 images

[Stefano Garzarella]

On Thu, Mar 04, 2021 at 10:25:33AM +0000, Daniel P. Berrangé wrote:
>On Thu, Mar 04, 2021 at 09:55:40AM +0100, Stefano Garzarella wrote:
>> On Wed, Mar 03, 2021 at 01:47:06PM -0500, Jason Dillaman wrote:
>> > On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> > >
>> > > Hi Jason,
>> > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
>> > > writing data is very slow compared to a raw file.
>> > >
>> > > Comparing raw vs QCOW2 image creation with RBD I found that we use a
>> > > different object size, for the raw file I see '4 MiB objects', for QCOW2
>> > > I see '64 KiB objects' as reported on comment 14 [2].
>> > > This should be the main issue of slowness, indeed forcing in the code 4
>> > > MiB object size also for QCOW2 increased the speed a lot.
>> > >
>> > > Looking better I discovered that for raw files, we call rbd_create()
>> > > with obj_order = 0 (if 'cluster_size' options is not defined), so the
>> > > default object size is used.
>> > > Instead for QCOW2, we use obj_order = 16, since the default
>> > > 'cluster_size' defined for QCOW2, is 64 KiB.
>> > >
>> > > Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
>> > > size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
>> > > QemuOpts calling qemu_opts_to_qdict_filtered().
>> > > For some reason that I have yet to understand, after this deletion,
>> > > however remains in QemuOpts the default value of 'cluster_size' for
>> > > qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts()
>> > >
>> > > At this point my doubts are:
>> > > Does it make sense to use the same cluster_size as qcow2 as object_size
>> > > in RBD?
>> >
>> > No, not really. But it also doesn't really make any sense to put a
>> > QCOW2 image within an RBD image. To clarify from the BZ, OpenStack
>> > does not put QCOW2 images on RBD, it converts QCOW2 images into raw
>> > images to store in RBD.
>>
>> Yes, that was my doubt, thanks for the confirmation.
>>
>> Also Daniel (+CC) confirmed me the same thing, but just to be complete he
>> added that there is a case where OpenStack could use qcow2 on RBD, but in
>> this case using in-kernel RBD, so the QEMU RBD is not involved.
>>
>> >
>> > > If we want to keep the 2 options separated, how can it be done? Should
>> > > we rename the option in block/rbd.c?
>> >
>> > You can already pass overrides to the RBD block driver by just
>> > appending them after the
>> > "rbd:<filename>[:option1=value1[:option2=value2]]" portion, perhaps
>> > that could be re-used.
>>
>> I see, we should extend qemu_rbd_parse_filename() to suppurt it.
>
>We shouldn't really be extending the legacy filename syntax.
>If we need extra options we want them in the QAPI schema for
>blockdev.

Got it.

I'm still a bit confused about how QemuOpts are handled between format 
and protocol drivers.

It seems that in this case the protocol tries to access some information 
from the format (BLOCK_OPT_CLUSTER_SIZE).

Since the format removes this information from the QemuOpts passed to 
the protocol, this takes the default value of the format, even if a 
different value is specified.

Is it correct for a protocol to access BLOCK_OPT_CLUSTER_SIZE?

>
>> Maybe if we don't want to support this configuration, we should print some
>> warning messages.
>
>Note these are separate layers in QEMU block layer. qcow2 is a format
>driver, while RBD is a protocol driver. QEMU lets any format driver be
>run on top of any protocol driver in general. In practice there are
>certain combinations that are more sane and commonly used than others,
>but QEMU doesn't document this or assign support level to pairing
>right now.

Thanks for the clarification,
Stefano

Re: QEMU RBD is slow with QCOW2 images

[Daniel P. Berrangé]

On Thu, Mar 04, 2021 at 12:12:51PM +0100, Stefano Garzarella wrote:
> On Thu, Mar 04, 2021 at 10:25:33AM +0000, Daniel P. Berrangé wrote:
> > On Thu, Mar 04, 2021 at 09:55:40AM +0100, Stefano Garzarella wrote:
> > > On Wed, Mar 03, 2021 at 01:47:06PM -0500, Jason Dillaman wrote:
> > > > On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > > >
> > > > > Hi Jason,
> > > > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
> > > > > writing data is very slow compared to a raw file.
> > > > >
> > > > > Comparing raw vs QCOW2 image creation with RBD I found that we use a
> > > > > different object size, for the raw file I see '4 MiB objects', for QCOW2
> > > > > I see '64 KiB objects' as reported on comment 14 [2].
> > > > > This should be the main issue of slowness, indeed forcing in the code 4
> > > > > MiB object size also for QCOW2 increased the speed a lot.
> > > > >
> > > > > Looking better I discovered that for raw files, we call rbd_create()
> > > > > with obj_order = 0 (if 'cluster_size' options is not defined), so the
> > > > > default object size is used.
> > > > > Instead for QCOW2, we use obj_order = 16, since the default
> > > > > 'cluster_size' defined for QCOW2, is 64 KiB.
> > > > >
> > > > > Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
> > > > > size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
> > > > > QemuOpts calling qemu_opts_to_qdict_filtered().
> > > > > For some reason that I have yet to understand, after this deletion,
> > > > > however remains in QemuOpts the default value of 'cluster_size' for
> > > > > qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts()
> > > > >
> > > > > At this point my doubts are:
> > > > > Does it make sense to use the same cluster_size as qcow2 as object_size
> > > > > in RBD?
> > > >
> > > > No, not really. But it also doesn't really make any sense to put a
> > > > QCOW2 image within an RBD image. To clarify from the BZ, OpenStack
> > > > does not put QCOW2 images on RBD, it converts QCOW2 images into raw
> > > > images to store in RBD.
> > > 
> > > Yes, that was my doubt, thanks for the confirmation.
> > > 
> > > Also Daniel (+CC) confirmed me the same thing, but just to be complete he
> > > added that there is a case where OpenStack could use qcow2 on RBD, but in
> > > this case using in-kernel RBD, so the QEMU RBD is not involved.
> > > 
> > > >
> > > > > If we want to keep the 2 options separated, how can it be done? Should
> > > > > we rename the option in block/rbd.c?
> > > >
> > > > You can already pass overrides to the RBD block driver by just
> > > > appending them after the
> > > > "rbd:<filename>[:option1=value1[:option2=value2]]" portion, perhaps
> > > > that could be re-used.
> > > 
> > > I see, we should extend qemu_rbd_parse_filename() to suppurt it.
> > 
> > We shouldn't really be extending the legacy filename syntax.
> > If we need extra options we want them in the QAPI schema for
> > blockdev.
> 
> Got it.
> 
> I'm still a bit confused about how QemuOpts are handled between format and
> protocol drivers.
> 
> It seems that in this case the protocol tries to access some information
> from the format (BLOCK_OPT_CLUSTER_SIZE).
> 
> Since the format removes this information from the QemuOpts passed to the
> protocol, this takes the default value of the format, even if a different
> value is specified.
> 
> Is it correct for a protocol to access BLOCK_OPT_CLUSTER_SIZE?

In a -blockdev world, the caller would be expected to set the values
explicitly at all layers that need it.

You're talking about a scenario that is non-blockdev though, and
I'm not sure what the right answer is here. Will need Kevin/Max
to answer that one.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: QEMU RBD is slow with QCOW2 images

[Kevin Wolf]

Am 03.03.2021 um 18:40 hat Stefano Garzarella geschrieben:
> Hi Jason,
> as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
> writing data is very slow compared to a raw file.
> 
> Comparing raw vs QCOW2 image creation with RBD I found that we use a
> different object size, for the raw file I see '4 MiB objects', for QCOW2 I
> see '64 KiB objects' as reported on comment 14 [2].
> This should be the main issue of slowness, indeed forcing in the code 4 MiB
> object size also for QCOW2 increased the speed a lot.
> 
> Looking better I discovered that for raw files, we call rbd_create() with
> obj_order = 0 (if 'cluster_size' options is not defined), so the default
> object size is used.
> Instead for QCOW2, we use obj_order = 16, since the default 'cluster_size'
> defined for QCOW2, is 64 KiB.

Hm, the QemuOpts-based image creation is messy, but why does the rbd
driver even see the cluster_size option?

The first thing qcow2_co_create_opts() does is splitting the passed
QemuOpts into options it will process on the qcow2 layer and options
that are passed to the protocol layer. So if you pass a cluster_size
option, qcow2 should take it for itself and not pass it to rbd.

If it is passed to rbd, I think that's a bug in the qcow2 driver.

> Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
> size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
> QemuOpts calling qemu_opts_to_qdict_filtered().
> For some reason that I have yet to understand, after this deletion, however
> remains in QemuOpts the default value of 'cluster_size' for qcow2 (64 KiB),
> that it's used in qemu_rbd_co_create_opts()

So it seems you came to a similar conclusion. We need to find out where
the 64k come from and just fix that so that rbd uses its default.

> At this point my doubts are:
> Does it make sense to use the same cluster_size as qcow2 as object_size in
> RBD?
> If we want to keep the 2 options separated, how can it be done? Should we
> rename the option in block/rbd.c?

My lazy answer is that you could just use QMP blockdev-create, where you
create layer by layer separately.

What could possibly be done for the QemuOpts is using the dotted syntax
like for opening, so you could specify file.cluster_size=... for the
protocol layer (or data_file.cluster_size=... for the external data
file etc.)

Kevin

Re: QEMU RBD is slow with QCOW2 images

[Stefano Garzarella]

On Thu, Mar 04, 2021 at 01:05:02PM +0100, Kevin Wolf wrote:
>Am 03.03.2021 um 18:40 hat Stefano Garzarella geschrieben:
>> Hi Jason,
>> as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
>> writing data is very slow compared to a raw file.
>>
>> Comparing raw vs QCOW2 image creation with RBD I found that we use a
>> different object size, for the raw file I see '4 MiB objects', for 
>> QCOW2 I
>> see '64 KiB objects' as reported on comment 14 [2].
>> This should be the main issue of slowness, indeed forcing in the code 4 MiB
>> object size also for QCOW2 increased the speed a lot.
>>
>> Looking better I discovered that for raw files, we call rbd_create() with
>> obj_order = 0 (if 'cluster_size' options is not defined), so the default
>> object size is used.
>> Instead for QCOW2, we use obj_order = 16, since the default 'cluster_size'
>> defined for QCOW2, is 64 KiB.
>
>Hm, the QemuOpts-based image creation is messy, but why does the rbd
>driver even see the cluster_size option?
>
>The first thing qcow2_co_create_opts() does is splitting the passed
>QemuOpts into options it will process on the qcow2 layer and options
>that are passed to the protocol layer. So if you pass a cluster_size
>option, qcow2 should take it for itself and not pass it to rbd.
>
>If it is passed to rbd, I think that's a bug in the qcow2 driver.

IIUC qcow2 properyl remove it, but when rbd uses 
qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE, 0) the default value 
of qcow2 format is returned.

Going in depth in qemu_opt_get_size_helper(), I found that 
qemu_opt_find() properly returns a NULL pointer, but then we call 
find_default_by_name() that returns the default value of qcow2 format 
(64k).

>
>> Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
>> size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
>> QemuOpts calling qemu_opts_to_qdict_filtered().
>> For some reason that I have yet to understand, after this deletion, however
>> remains in QemuOpts the default value of 'cluster_size' for qcow2 (64 KiB),
>> that it's used in qemu_rbd_co_create_opts()
>
>So it seems you came to a similar conclusion. We need to find out where
>the 64k come from and just fix that so that rbd uses its default.

Yes, I tried debugging above, but I'm not sure how to fix it.

Maybe a new parameter in qemu_opt_get_size_helper() to prevent it from 
looking for the default value.
Or we should prevent the default value from being added to the 
opts->list->desc, but that part is still not very clear to me.

>
>> At this point my doubts are:
>> Does it make sense to use the same cluster_size as qcow2 as object_size in
>> RBD?
>> If we want to keep the 2 options separated, how can it be done? Should we
>> rename the option in block/rbd.c?
>
>My lazy answer is that you could just use QMP blockdev-create, where you
>create layer by layer separately.
>
>What could possibly be done for the QemuOpts is using the dotted syntax
>like for opening, so you could specify file.cluster_size=... for the
>protocol layer (or data_file.cluster_size=... for the external data
>file etc.)
>

This would be cool :-)

Thanks,
Stefano

Re: QEMU RBD is slow with QCOW2 images

[Kevin Wolf]

Am 04.03.2021 um 15:08 hat Stefano Garzarella geschrieben:
> On Thu, Mar 04, 2021 at 01:05:02PM +0100, Kevin Wolf wrote:
> > Am 03.03.2021 um 18:40 hat Stefano Garzarella geschrieben:
> > > Hi Jason,
> > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
> > > writing data is very slow compared to a raw file.
> > > 
> > > Comparing raw vs QCOW2 image creation with RBD I found that we use a
> > > different object size, for the raw file I see '4 MiB objects', for
> > > QCOW2 I
> > > see '64 KiB objects' as reported on comment 14 [2].
> > > This should be the main issue of slowness, indeed forcing in the code 4 MiB
> > > object size also for QCOW2 increased the speed a lot.
> > > 
> > > Looking better I discovered that for raw files, we call rbd_create() with
> > > obj_order = 0 (if 'cluster_size' options is not defined), so the default
> > > object size is used.
> > > Instead for QCOW2, we use obj_order = 16, since the default 'cluster_size'
> > > defined for QCOW2, is 64 KiB.
> > 
> > Hm, the QemuOpts-based image creation is messy, but why does the rbd
> > driver even see the cluster_size option?
> > 
> > The first thing qcow2_co_create_opts() does is splitting the passed
> > QemuOpts into options it will process on the qcow2 layer and options
> > that are passed to the protocol layer. So if you pass a cluster_size
> > option, qcow2 should take it for itself and not pass it to rbd.
> > 
> > If it is passed to rbd, I think that's a bug in the qcow2 driver.
> 
> IIUC qcow2 properyl remove it, but when rbd uses qemu_opt_get_size_del(opts,
> BLOCK_OPT_CLUSTER_SIZE, 0) the default value of qcow2 format is returned.
> 
> Going in depth in qemu_opt_get_size_helper(), I found that qemu_opt_find()
> properly returns a NULL pointer, but then we call find_default_by_name()
> that returns the default value of qcow2 format (64k).

Ugh, I see why. We're passing the protocol driver a QemuOpts that was
created for a QemuOptsList with the qcow2 default, not for its own
QemuOptsList. This is wrong.

Note that the QemuOptsList is not qcow2_create_opts itself, but a list
that is created with qemu_opts_append() to combine qcow2 and rbd options
into a new QemuOptsList. For overlapping options, the format wins.

I don't think you can change the QemuOptsList of an existing QemuOpts,
nor is there a clone operation that could just copy all options into a
new QemuOpts created for the rbd QemuOptsList, so maybe the easiest
hack^Wsolution would be converting to QDict and back...

> > > Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
> > > size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
> > > QemuOpts calling qemu_opts_to_qdict_filtered().
> > > For some reason that I have yet to understand, after this deletion, however
> > > remains in QemuOpts the default value of 'cluster_size' for qcow2 (64 KiB),
> > > that it's used in qemu_rbd_co_create_opts()
> > 
> > So it seems you came to a similar conclusion. We need to find out where
> > the 64k come from and just fix that so that rbd uses its default.
> 
> Yes, I tried debugging above, but I'm not sure how to fix it.
> 
> Maybe a new parameter in qemu_opt_get_size_helper() to prevent it from
> looking for the default value.
> Or we should prevent the default value from being added to the
> opts->list->desc, but that part is still not very clear to me.

opts->list is already wrong, I think this is what we need to fix.

> > > At this point my doubts are:
> > > Does it make sense to use the same cluster_size as qcow2 as object_size in
> > > RBD?
> > > If we want to keep the 2 options separated, how can it be done? Should we
> > > rename the option in block/rbd.c?
> > 
> > My lazy answer is that you could just use QMP blockdev-create, where you
> > create layer by layer separately.
> > 
> > What could possibly be done for the QemuOpts is using the dotted syntax
> > like for opening, so you could specify file.cluster_size=... for the
> > protocol layer (or data_file.cluster_size=... for the external data
> > file etc.)
> 
> This would be cool :-)

I'm almost sure that compatibility will make this more complicated than
it sounds, but we could have a try.

Kevin

Re: QEMU RBD is slow with QCOW2 images

[Stefano Garzarella]

On Thu, Mar 04, 2021 at 03:59:17PM +0100, Kevin Wolf wrote:
>Am 04.03.2021 um 15:08 hat Stefano Garzarella geschrieben:
>> On Thu, Mar 04, 2021 at 01:05:02PM +0100, Kevin Wolf wrote:
>> > Am 03.03.2021 um 18:40 hat Stefano Garzarella geschrieben:
>> > > Hi Jason,
>> > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
>> > > writing data is very slow compared to a raw file.
>> > >
>> > > Comparing raw vs QCOW2 image creation with RBD I found that we use a
>> > > different object size, for the raw file I see '4 MiB objects', for
>> > > QCOW2 I
>> > > see '64 KiB objects' as reported on comment 14 [2].
>> > > This should be the main issue of slowness, indeed forcing in the code 4 MiB
>> > > object size also for QCOW2 increased the speed a lot.
>> > >
>> > > Looking better I discovered that for raw files, we call rbd_create() with
>> > > obj_order = 0 (if 'cluster_size' options is not defined), so the default
>> > > object size is used.
>> > > Instead for QCOW2, we use obj_order = 16, since the default 'cluster_size'
>> > > defined for QCOW2, is 64 KiB.
>> >
>> > Hm, the QemuOpts-based image creation is messy, but why does the rbd
>> > driver even see the cluster_size option?
>> >
>> > The first thing qcow2_co_create_opts() does is splitting the passed
>> > QemuOpts into options it will process on the qcow2 layer and options
>> > that are passed to the protocol layer. So if you pass a cluster_size
>> > option, qcow2 should take it for itself and not pass it to rbd.
>> >
>> > If it is passed to rbd, I think that's a bug in the qcow2 driver.
>>
>> IIUC qcow2 properyl remove it, but when rbd uses qemu_opt_get_size_del(opts,
>> BLOCK_OPT_CLUSTER_SIZE, 0) the default value of qcow2 format is returned.
>>
>> Going in depth in qemu_opt_get_size_helper(), I found that qemu_opt_find()
>> properly returns a NULL pointer, but then we call find_default_by_name()
>> that returns the default value of qcow2 format (64k).
>
>Ugh, I see why. We're passing the protocol driver a QemuOpts that was
>created for a QemuOptsList with the qcow2 default, not for its own
>QemuOptsList. This is wrong.
>
>Note that the QemuOptsList is not qcow2_create_opts itself, but a list
>that is created with qemu_opts_append() to combine qcow2 and rbd options
>into a new QemuOptsList. For overlapping options, the format wins.
>
>I don't think you can change the QemuOptsList of an existing QemuOpts,
>nor is there a clone operation that could just copy all options into a
>new QemuOpts created for the rbd QemuOptsList, so maybe the easiest
>hack^Wsolution would be converting to QDict and back...

Do you mean something like this? (I'll send a proper patch when 
everything is a little clearer to me :-)

diff --git a/block.c b/block.c
index a1f3cecd75..74b02b32dc 100644
--- a/block.c
+++ b/block.c
@@ -671,13 +671,33 @@ out:
  int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
  {
      BlockDriver *drv;
+    QemuOpts *new_opts;
+    QDict *qdict;
+    int ret;

      drv = bdrv_find_protocol(filename, true, errp);
      if (drv == NULL) {
          return -ENOENT;
      }

-    return bdrv_create(drv, filename, opts, errp);
+    if (!drv->create_opts) {
+        error_setg(errp, "Driver '%s' does not support image creation",
+                   drv->format_name);
+        return -ENOTSUP;
+    }
+
+    qdict = qemu_opts_to_qdict(opts, NULL);
+    new_opts = qemu_opts_from_qdict(drv->create_opts, qdict, errp);
+    if (new_opts == NULL) {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    ret = bdrv_create(drv, filename, new_opts, errp);
+out:
+    qemu_opts_del(new_opts);
+    qobject_unref(qdict);
+    return ret;
  }

>
>> > > Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
>> > > size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
>> > > QemuOpts calling qemu_opts_to_qdict_filtered().
>> > > For some reason that I have yet to understand, after this deletion, however
>> > > remains in QemuOpts the default value of 'cluster_size' for qcow2 (64 KiB),
>> > > that it's used in qemu_rbd_co_create_opts()
>> >
>> > So it seems you came to a similar conclusion. We need to find out where
>> > the 64k come from and just fix that so that rbd uses its default.
>>
>> Yes, I tried debugging above, but I'm not sure how to fix it.
>>
>> Maybe a new parameter in qemu_opt_get_size_helper() to prevent it from
>> looking for the default value.
>> Or we should prevent the default value from being added to the
>> opts->list->desc, but that part is still not very clear to me.
>
>opts->list is already wrong, I think this is what we need to fix.
>
>> > > At this point my doubts are:
>> > > Does it make sense to use the same cluster_size as qcow2 as object_size in
>> > > RBD?
>> > > If we want to keep the 2 options separated, how can it be done?  
>> > > Should we
>> > > rename the option in block/rbd.c?
>> >
>> > My lazy answer is that you could just use QMP blockdev-create, where you
>> > create layer by layer separately.
>> >
>> > What could possibly be done for the QemuOpts is using the dotted syntax
>> > like for opening, so you could specify file.cluster_size=... for the
>> > protocol layer (or data_file.cluster_size=... for the external data
>> > file etc.)
>>
>> This would be cool :-)
>
>I'm almost sure that compatibility will make this more complicated than
>it sounds, but we could have a try.

Eheh I see your point.

Thanks,
Stefano

Re: QEMU RBD is slow with QCOW2 images

[Kevin Wolf]

Am 04.03.2021 um 18:32 hat Stefano Garzarella geschrieben:
> On Thu, Mar 04, 2021 at 03:59:17PM +0100, Kevin Wolf wrote:
> > Am 04.03.2021 um 15:08 hat Stefano Garzarella geschrieben:
> > > On Thu, Mar 04, 2021 at 01:05:02PM +0100, Kevin Wolf wrote:
> > > > Am 03.03.2021 um 18:40 hat Stefano Garzarella geschrieben:
> > > > > Hi Jason,
> > > > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
> > > > > writing data is very slow compared to a raw file.
> > > > >
> > > > > Comparing raw vs QCOW2 image creation with RBD I found that we use a
> > > > > different object size, for the raw file I see '4 MiB objects', for
> > > > > QCOW2 I
> > > > > see '64 KiB objects' as reported on comment 14 [2].
> > > > > This should be the main issue of slowness, indeed forcing in the code 4 MiB
> > > > > object size also for QCOW2 increased the speed a lot.
> > > > >
> > > > > Looking better I discovered that for raw files, we call rbd_create() with
> > > > > obj_order = 0 (if 'cluster_size' options is not defined), so the default
> > > > > object size is used.
> > > > > Instead for QCOW2, we use obj_order = 16, since the default 'cluster_size'
> > > > > defined for QCOW2, is 64 KiB.
> > > >
> > > > Hm, the QemuOpts-based image creation is messy, but why does the rbd
> > > > driver even see the cluster_size option?
> > > >
> > > > The first thing qcow2_co_create_opts() does is splitting the passed
> > > > QemuOpts into options it will process on the qcow2 layer and options
> > > > that are passed to the protocol layer. So if you pass a cluster_size
> > > > option, qcow2 should take it for itself and not pass it to rbd.
> > > >
> > > > If it is passed to rbd, I think that's a bug in the qcow2 driver.
> > > 
> > > IIUC qcow2 properyl remove it, but when rbd uses qemu_opt_get_size_del(opts,
> > > BLOCK_OPT_CLUSTER_SIZE, 0) the default value of qcow2 format is returned.
> > > 
> > > Going in depth in qemu_opt_get_size_helper(), I found that qemu_opt_find()
> > > properly returns a NULL pointer, but then we call find_default_by_name()
> > > that returns the default value of qcow2 format (64k).
> > 
> > Ugh, I see why. We're passing the protocol driver a QemuOpts that was
> > created for a QemuOptsList with the qcow2 default, not for its own
> > QemuOptsList. This is wrong.
> > 
> > Note that the QemuOptsList is not qcow2_create_opts itself, but a list
> > that is created with qemu_opts_append() to combine qcow2 and rbd options
> > into a new QemuOptsList. For overlapping options, the format wins.
> > 
> > I don't think you can change the QemuOptsList of an existing QemuOpts,
> > nor is there a clone operation that could just copy all options into a
> > new QemuOpts created for the rbd QemuOptsList, so maybe the easiest
> > hack^Wsolution would be converting to QDict and back...
> 
> Do you mean something like this? (I'll send a proper patch when everything
> is a little clearer to me :-)
> 
> diff --git a/block.c b/block.c
> index a1f3cecd75..74b02b32dc 100644
> --- a/block.c
> +++ b/block.c
> @@ -671,13 +671,33 @@ out:
>  int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>  {
>      BlockDriver *drv;
> +    QemuOpts *new_opts;
> +    QDict *qdict;
> +    int ret;
> 
>      drv = bdrv_find_protocol(filename, true, errp);
>      if (drv == NULL) {
>          return -ENOENT;
>      }
> 
> -    return bdrv_create(drv, filename, opts, errp);
> +    if (!drv->create_opts) {
> +        error_setg(errp, "Driver '%s' does not support image creation",
> +                   drv->format_name);
> +        return -ENOTSUP;
> +    }
> +
> +    qdict = qemu_opts_to_qdict(opts, NULL);
> +    new_opts = qemu_opts_from_qdict(drv->create_opts, qdict, errp);
> +    if (new_opts == NULL) {
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    ret = bdrv_create(drv, filename, new_opts, errp);
> +out:
> +    qemu_opts_del(new_opts);
> +    qobject_unref(qdict);
> +    return ret;
>  }

Something like this, yes. Does it work for you?

Of course, in the real patch it could use a comment why we're doing
these seemingly redundant conversions.

Kevin

Re: QEMU RBD is slow with QCOW2 images

[Stefano Garzarella]

On Fri, Mar 05, 2021 at 10:16:41AM +0100, Kevin Wolf wrote:
>Am 04.03.2021 um 18:32 hat Stefano Garzarella geschrieben:
>> On Thu, Mar 04, 2021 at 03:59:17PM +0100, Kevin Wolf wrote:
>> > Am 04.03.2021 um 15:08 hat Stefano Garzarella geschrieben:
>> > > On Thu, Mar 04, 2021 at 01:05:02PM +0100, Kevin Wolf wrote:
>> > > > Am 03.03.2021 um 18:40 hat Stefano Garzarella geschrieben:
>> > > > > Hi Jason,
>> > > > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
>> > > > > writing data is very slow compared to a raw file.
>> > > > >
>> > > > > Comparing raw vs QCOW2 image creation with RBD I found that we use a
>> > > > > different object size, for the raw file I see '4 MiB objects', for
>> > > > > QCOW2 I
>> > > > > see '64 KiB objects' as reported on comment 14 [2].
>> > > > > This should be the main issue of slowness, indeed forcing in the code 4 MiB
>> > > > > object size also for QCOW2 increased the speed a lot.
>> > > > >
>> > > > > Looking better I discovered that for raw files, we call rbd_create() with
>> > > > > obj_order = 0 (if 'cluster_size' options is not defined), so the default
>> > > > > object size is used.
>> > > > > Instead for QCOW2, we use obj_order = 16, since the default 'cluster_size'
>> > > > > defined for QCOW2, is 64 KiB.
>> > > >
>> > > > Hm, the QemuOpts-based image creation is messy, but why does the rbd
>> > > > driver even see the cluster_size option?
>> > > >
>> > > > The first thing qcow2_co_create_opts() does is splitting the passed
>> > > > QemuOpts into options it will process on the qcow2 layer and options
>> > > > that are passed to the protocol layer. So if you pass a cluster_size
>> > > > option, qcow2 should take it for itself and not pass it to rbd.
>> > > >
>> > > > If it is passed to rbd, I think that's a bug in the qcow2 driver.
>> > >
>> > > IIUC qcow2 properyl remove it, but when rbd uses qemu_opt_get_size_del(opts,
>> > > BLOCK_OPT_CLUSTER_SIZE, 0) the default value of qcow2 format is returned.
>> > >
>> > > Going in depth in qemu_opt_get_size_helper(), I found that qemu_opt_find()
>> > > properly returns a NULL pointer, but then we call find_default_by_name()
>> > > that returns the default value of qcow2 format (64k).
>> >
>> > Ugh, I see why. We're passing the protocol driver a QemuOpts that was
>> > created for a QemuOptsList with the qcow2 default, not for its own
>> > QemuOptsList. This is wrong.
>> >
>> > Note that the QemuOptsList is not qcow2_create_opts itself, but a list
>> > that is created with qemu_opts_append() to combine qcow2 and rbd options
>> > into a new QemuOptsList. For overlapping options, the format wins.
>> >
>> > I don't think you can change the QemuOptsList of an existing QemuOpts,
>> > nor is there a clone operation that could just copy all options into a
>> > new QemuOpts created for the rbd QemuOptsList, so maybe the easiest
>> > hack^Wsolution would be converting to QDict and back...
>>
>> Do you mean something like this? (I'll send a proper patch when everything
>> is a little clearer to me :-)
>>
>> diff --git a/block.c b/block.c
>> index a1f3cecd75..74b02b32dc 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -671,13 +671,33 @@ out:
>>  int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>>  {
>>      BlockDriver *drv;
>> +    QemuOpts *new_opts;
>> +    QDict *qdict;
>> +    int ret;
>>
>>      drv = bdrv_find_protocol(filename, true, errp);
>>      if (drv == NULL) {
>>          return -ENOENT;
>>      }
>>
>> -    return bdrv_create(drv, filename, opts, errp);
>> +    if (!drv->create_opts) {
>> +        error_setg(errp, "Driver '%s' does not support image creation",
>> +                   drv->format_name);
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    qdict = qemu_opts_to_qdict(opts, NULL);
>> +    new_opts = qemu_opts_from_qdict(drv->create_opts, qdict, errp);
>> +    if (new_opts == NULL) {
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    ret = bdrv_create(drv, filename, new_opts, errp);
>> +out:
>> +    qemu_opts_del(new_opts);
>> +    qobject_unref(qdict);
>> +    return ret;
>>  }
>
>Something like this, yes. Does it work for you?

Yes, I did some testing and only the protocol parameters are passed and 
its defaults.

Thanks for the suggestion!

>
>Of course, in the real patch it could use a comment why we're doing
>these seemingly redundant conversions.

Sure, I'm running some tests to make sure I haven't broken anything. :-)

Thanks,
Stefano