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

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

[Stefano Stabellini]

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>

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

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

[Paolo Bonzini]

Il 26/06/2013 19:48, Stefano Stabellini ha scritto:
> +    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;
> +    }

You can use bdrv_parse_cache_flags.  Note that BDRV_O_NATIVE_AIO
requires BDRV_O_NOCACHE too (but if you only specify BDRV_O_NATIVE_AIO
it's a no-op, not an error).

Paolo

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

[Stefano Stabellini]

On Wed, 26 Jun 2013, Paolo Bonzini wrote:
> Il 26/06/2013 19:48, Stefano Stabellini ha scritto:
> > +    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;
> > +    }
> 
> You can use bdrv_parse_cache_flags.

I didn't want to call to bdrv_parse_cache_flags because the options are
slightly different (they are a subset) and they are going to become
part of the block interface, so the parsing could diverge in the future.

However as of today I could call bdrv_parse_cache_flags and it would
work fine.


> Note that BDRV_O_NATIVE_AIO
> requires BDRV_O_NOCACHE too (but if you only specify BDRV_O_NATIVE_AIO
> it's a no-op, not an error).

Yeah, good point, I might as well remove it from there.

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

[Anthony Liguori]

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...

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

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

[Stefano Stabellini]

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.



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

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

[Anthony Liguori]

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