Re: [Qemu-devel] [PATCH] xen_disk: support cache backend option

[Anthony Liguori <anthony@codemonkey.ws>]

Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:

> On Wed, 26 Jun 2013, Anthony Liguori wrote:
>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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 <stefano.stabellini@eu.citrix.com>
>> 
>> 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 <aliguori@us.ibm.com>

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;
>> >  }
>>