| Message ID | alpine.DEB.2.02.1306261846300.4782@kaball.uk.xensource.com |
|---|---|
| State | New |
| Headers | show |
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
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; > }
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.
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; > > } >
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; >> > } >>
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; }
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>