Re: [Xen-devel] [PATCH v2] introduce a cache options for PV disks

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 27 Jun 2013, Stefano Stabellini wrote:
> On Thu, 27 Jun 2013, Ian Jackson wrote:
> > Stefano Stabellini writes ("[PATCH v2] introduce a cache options for PV 
> > disks"):
> > > Document a per-disk cache option in the xl config file to allow users to
> > > select the cache mode that the backend should use to open the disk file
> > > or device.
> > 
> > Here's an RFD revised version of the qemu patch.  I haven't compiled
> > it yet.
> > 
> 
> It's mostly OK, just few minor corrections for code readability

BTW are you going to follow up or do you want me to?

> 
> > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> > index 247f32f..9b42e7f 100644
> > --- a/hw/block/xen_disk.c
> > +++ b/hw/block/xen_disk.c
> > @@ -93,6 +93,7 @@ struct XenBlkDev {
> >      char                *type;
> >      char                *dev;
> >      char                *devtype;
> > +    bool                directiosafe;
> >      const char          *fileproto;
> >      const char          *filename;
> >      int                 ring_ref;
> > @@ -701,6 +702,7 @@ static int blk_init(struct XenDevice *xendev)
> >  {
> >      struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, 
> > xendev);
> >      int info = 0;
> > +    char *directiosafe == NULL;
>                            ^ =
> 
> >      /* read xenstore entries */
> >      if (blkdev->params == NULL) {
> > @@ -733,6 +735,8 @@ static int blk_init(struct XenDevice *xendev)
> >      if (blkdev->devtype == NULL) {
> >          blkdev->devtype = xenstore_read_be_str(&blkdev->xendev, 
> > "device-type");
> >      }
> > +    directiosafe = xenstore_read_be_str(&blkdev->xendev, "direct-io-safe");
> > +    blkdev->directiosafe = (directiosafe && atoi(directiofsafe));
> >  
> >      /* do we have all we need? */
> >      if (blkdev->params == NULL ||
> > @@ -760,6 +764,8 @@ static int blk_init(struct XenDevice *xendev)
> >      xenstore_write_be_int(&blkdev->xendev, "feature-flush-cache", 1);
> >      xenstore_write_be_int(&blkdev->xendev, "feature-persistent", 1);
> >      xenstore_write_be_int(&blkdev->xendev, "info", info);
> > +
> > +    g_free(directiosafe);
> >      return 0;
> >  
> >  out_error:
> > @@ -773,6 +779,7 @@ out_error:
> >      blkdev->dev = NULL;
> >      g_free(blkdev->devtype);
> >      blkdev->devtype = NULL;
> > +    g_free(directiosafe);
> 
> maybe add
> 
> blkdev->directiosafe = false;
> 
> 
> >      return -1;
> >  }
> >  
> > @@ -784,6 +791,9 @@ static int blk_connect(struct XenDevice *xendev)
> >  
> >      /* read-only ? */
> >      qflags = BDRV_O_CACHE_WB | BDRV_O_NATIVE_AIO;
> > +    if (blkdev->directiosafe) {
> > +        qflags = BDRV_O_NOCACHE | BDRV_O_NATIVE_AIO;
> > +    }
> 
> Please change this into:
> 
> if (blkdev->directiosafe) {
>     qflags = BDRV_O_NOCACHE | BDRV_O_NATIVE_AIO;
> } else {
>     qflags = BDRV_O_CACHE_WB;
> }
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel