[pve-devel] [PATCH] Add patch to improve qmrestore to RBD, activating writeback cache.

[Eneko Lacunza]

Hi,

El 26/07/16 a las 17:28, Alexandre DERUMIER escribió:
>>> I wonder if this couldn't be fixed directly in rbd.c block driver
> I have send a patch, can you test ?
I got it, will be testing this morning and will report back, this really 
seems the correct thing to do (fix the rbd block "bug").
>
>
>
> ----- Mail original -----
> De: "aderumier" <aderumier at odiso.com>
> À: "pve-devel" <pve-devel at pve.proxmox.com>
> Envoyé: Mardi 26 Juillet 2016 16:46:58
> Objet: Re: [pve-devel] [PATCH] Add patch to improve qmrestore to RBD, activating writeback cache.
>
> I wonder if this couldn't be fixed directly in rbd.c block driver
>
>
> currently:
>
> block/rbd.c
>
> if (flags & BDRV_O_NOCACHE) {
> rados_conf_set(s->cluster, "rbd_cache", "false");
> } else {
> rados_conf_set(s->cluster, "rbd_cache", "true");
> }
>
>
> and in block.c
>
> int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough)
> {
> *flags &= ~BDRV_O_CACHE_MASK;
>
> if (!strcmp(mode, "off") || !strcmp(mode, "none")) {
> *writethrough = false;
> *flags |= BDRV_O_NOCACHE;
> } else if (!strcmp(mode, "directsync")) {
> *writethrough = true;
> *flags |= BDRV_O_NOCACHE;
> } else if (!strcmp(mode, "writeback")) {
> *writethrough = false;
> } else if (!strcmp(mode, "unsafe")) {
> *writethrough = false;
> *flags |= BDRV_O_NO_FLUSH;
> } else if (!strcmp(mode, "writethrough")) {
> *writethrough = true;
> } else {
> return -1;
> }
>
> return 0;
> }
>
>
>
> as in qemu-img convert we don't send flush and in vma restore too, we use BDRV_O_NO_FLUSH.
>
>
> I think we should add in rbd.c something like
>
> if (flags & BDRV_O_NOCACHE) {
> rados_conf_set(s->cluster, "rbd_cache", "false");
> } else {
> rados_conf_set(s->cluster, "rbd_cache", "true");
> }
>
> + if (flags & BDRV_O_NO_FLUSH) {
> + rados_conf_set(s->cluster, "rbd_cache_writethrough_until_flush", "false");
> + }
>
>
> I really think it's the right way (or it's impossible to use unsafe cache option) and could be pushed to qemu upsteam.
>
>
> ----- Mail original -----
> De: "Eneko Lacunza" <elacunza at binovo.es>
> À: "pve-devel" <pve-devel at pve.proxmox.com>
> Envoyé: Mardi 26 Juillet 2016 16:05:52
> Objet: Re: [pve-devel] [PATCH] Add patch to improve qmrestore to RBD, activating writeback cache.
>
> Hi,
>
> I'm unsure about wether it would be better to have one patch or two
> independent patches?
>
> El 26/07/16 a las 16:02, Alexandre DERUMIER escribió:
>> Hi,
>>
>> I think that qemu-img convert have the same problem
>>
>> I have found a commit from 2015
>> https://git.greensocs.com/fkonrad/mttcg/commit/80ccf93b884a2edab5ec62634758e942bba81b7c
>>
>> By default it don't send flush (cache=unsafe), and the commit add a flush on image closing, because some storage like sheepdog,
>> can't do the flush by themself.
>>
>> maybe can we add the same flush just after the opening in convert.
>>
>>
>>
>> ----- Mail original -----
>> De: "Eneko Lacunza" <elacunza at binovo.es>
>> À: "pve-devel" <pve-devel at pve.proxmox.com>
>> Cc: "Eneko Lacunza" <elacunza at pve-test.binovo.net>
>> Envoyé: Mardi 26 Juillet 2016 15:18:55
>> Objet: [pve-devel] [PATCH] Add patch to improve qmrestore to RBD, activating writeback cache.
>>
>> From: Eneko Lacunza <elacunza at pve-test.binovo.net>
>>
>> Signed-off-by: Eneko Lacunza <elacunza at pve-test.binovo.net>
>> ---
>> .../0054-vma-force-enable-rbd-cache-for-qmrestore.patch | 17 +++++++++++++++++
>> debian/patches/series | 1 +
>> 2 files changed, 18 insertions(+)
>> create mode 100644 debian/patches/pve/0054-vma-force-enable-rbd-cache-for-qmrestore.patch
>>
>> diff --git a/debian/patches/pve/0054-vma-force-enable-rbd-cache-for-qmrestore.patch b/debian/patches/pve/0054-vma-force-enable-rbd-cache-for-qmrestore.patch
>> new file mode 100644
>> index 0000000..d9722c7
>> --- /dev/null
>> +++ b/debian/patches/pve/0054-vma-force-enable-rbd-cache-for-qmrestore.patch
>> @@ -0,0 +1,17 @@
>> +Issue a bogus flush so that Ceph activates rbd cache, accelerating qmrestore to RBD.
>> +---
>> +Index: b/vma.c
>> +===================================================================
>> +--- a/vma.c
>> ++++ b/vma.c
>> +@@ -335,6 +335,9 @@ static int extract_content(int argc, cha
>> +
>> + BlockDriverState *bs = blk_bs(blk);
>> +
>> ++ /* This is needed to activate rbd cache (writeback/coalesce) */
>> ++ bdrv_flush(bs);
>> ++
>> + if (vma_reader_register_bs(vmar, i, bs, write_zero, &errp) < 0) {
>> + g_error("%s", error_get_pretty(errp));
>> + }
>> +
>> diff --git a/debian/patches/series b/debian/patches/series
>> index 3614309..c858a30 100644
>> --- a/debian/patches/series
>> +++ b/debian/patches/series
>> @@ -51,6 +51,7 @@ pve/0050-fix-possible-unitialised-return-value.patch
>> pve/0051-net-NET_CLIENT_OPTIONS_KIND_MAX-changed.patch
>> pve/0052-vnc-refactor-to-QIOChannelSocket.patch
>> pve/0053-vma-use-BlockBackend-on-extract.patch
>> +pve/0054-vma-force-enable-rbd-cache-for-qmrestore.patch
>> #see https://bugs.launchpad.net/qemu/+bug/1488363?comments=all
>> extra/0001-Revert-target-i386-disable-LINT0-after-reset.patch
>> extra/0001-i386-kvmvapic-initialise-imm32-variable.patch
>


-- 
Zuzendari Teknikoa / Director Técnico
Binovo IT Human Project, S.L.
Telf. 943493611
       943324914
Astigarraga bidea 2, planta 6 dcha., ofi. 3-2; 20180 Oiartzun (Gipuzkoa)
www.binovo.es