[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] xen_disk: support cache backend option
From: |
Anthony Liguori |
Subject: |
Re: [Qemu-devel] [PATCH] xen_disk: support cache backend option |
Date: |
Wed, 26 Jun 2013 17:10:20 -0500 |
User-agent: |
Notmuch/0.15.2+77~g661dcf8 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu) |
Stefano Stabellini <address@hidden> writes:
> On Wed, 26 Jun 2013, Anthony Liguori wrote:
>> Stefano Stabellini <address@hidden> writes:
>>
>> > Support a backend option "cache" that specifies the cache mode that
>> > should be used to open the disk file or device.
>> >
>> > See: http://marc.info/?l=xen-devel&m=137226872905057
>> >
>> > Signed-off-by: Stefano Stabellini <address@hidden>
>>
>> Is the guest setting this or a management tool? I thought we were
>> moving to having the Xen management tools use QMP and the command line
>> instead of putting this stuff in XenStore...
>
> And we are, in fact we have just introduced QMP based cpu hotplug in
> libxl for HVM guests, using the existing cpu-add command.
>
> However this option would be part of the existing block protocol, that
> like all the other PV protocols are entirely xenstore based.
> I think it makes sense to introduce the cache configuration via xenstore
> and make it part of the block interface so that other block backend
> (like blkback and blktap) might support it too in the future. Otherwise
> it would be a QEMU-only thing and moreover it would be the only block
> related configuration for the PV backend to go via QMP when everything
> else comes from xenstore. Pretty ugly.
Bleck. I really wish we didn't have this logic in QEMU in the first
place. I guess since it's already here though, extending it can't hurt.
But please try to remove the whole set-things-up-via-Xenstore in the
future. It will become a problem at some point as it's a pretty
significant layering violation. Maybe the thing to do is move the logic
out of the device and into a Xen-specific module that setups the device
model based on Xenstore...
Anyway, I guess:
Acked-by: Anthony Liguori <address@hidden>
I would not use bdrv_parse_cache_flags since we may add more down the
road.
Regards,
Anthony Liguori
>
>
>
>> Regards,
>>
>> Anthony Liguori
>>
>> >
>> > diff --git a/hw/xen_disk.c b/hw/xen_disk.c
>> > index f484404..092aa6b 100644
>> > --- a/hw/block/xen_disk.c
>> > +++ b/hw/block/xen_disk.c
>> > @@ -94,6 +94,7 @@ struct XenBlkDev {
>> > char *type;
>> > char *dev;
>> > char *devtype;
>> > + char *cache;
>> > const char *fileproto;
>> > const char *filename;
>> > int ring_ref;
>> > @@ -734,6 +735,12 @@ static int blk_init(struct XenDevice *xendev)
>> > if (blkdev->devtype == NULL) {
>> > blkdev->devtype = xenstore_read_be_str(&blkdev->xendev,
>> > "device-type");
>> > }
>> > + if (blkdev->cache == NULL) {
>> > + blkdev->cache = xenstore_read_be_str(&blkdev->xendev, "cache");
>> > + }
>> > + if (blkdev->cache == NULL) {
>> > + blkdev->cache = g_strdup("writeback");
>> > + }
>> >
>> > /* do we have all we need? */
>> > if (blkdev->params == NULL ||
>> > @@ -774,6 +781,8 @@ out_error:
>> > blkdev->dev = NULL;
>> > g_free(blkdev->devtype);
>> > blkdev->devtype = NULL;
>> > + g_free(blkdev->cache);
>> > + blkdev->cache = NULL;
>> > return -1;
>> > }
>> >
>> > @@ -782,8 +791,14 @@ static int blk_connect(struct XenDevice *xendev)
>> > struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev,
>> > xendev);
>> > int pers, index, qflags;
>> >
>> > - /* read-only ? */
>> > - qflags = BDRV_O_CACHE_WB | BDRV_O_NATIVE_AIO;
>> > + if (!strcmp(blkdev->cache, "none")) {
>> > + qflags = BDRV_O_NATIVE_AIO | BDRV_O_NOCACHE;
>> > + } else if (!strcmp(blkdev->cache, "writethrough")) {
>> > + qflags = 0;
>> > + } else {
>> > + /* default to writeback */
>> > + qflags = BDRV_O_NATIVE_AIO | BDRV_O_CACHE_WB;
>> > + }
>> > if (strcmp(blkdev->mode, "w") == 0) {
>> > qflags |= BDRV_O_RDWR;
>> > }
>> > @@ -950,6 +965,7 @@ static int blk_free(struct XenDevice *xendev)
>> > g_free(blkdev->type);
>> > g_free(blkdev->dev);
>> > g_free(blkdev->devtype);
>> > + g_free(blkdev->cache);
>> > qemu_bh_delete(blkdev->bh);
>> > return 0;
>> > }
>>