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

[Stefano Stabellini]

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.



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