Re: [Qemu-devel] [PATCH v2] xen_disk: support "direct-io-safe" backend option

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Thu, 27 Jun 2013, Stefano Stabellini wrote:
> Support backend option "direct-io-safe".  This is documented as
> follows in the Xen backend specification:
> 
>  * direct-io-safe
>  *      Values:         0/1 (boolean)
>  *      Default Value:  0
>  *
>  *      The underlying storage is not affected by the direct IO memory
>  *      lifetime bug.  See:
>  *        http://lists.xen.org/archives/html/xen-devel/2012-12/msg01154.html
>  *
>  *      Therefore this option gives the backend permission to use
>  *      O_DIRECT, notwithstanding that bug.
>  *
>  *      That is, if this option is enabled, use of O_DIRECT is safe,
>  *      in circumstances where we would normally have avoided it as a
>  *      workaround for that bug.  This option is not relevant for all
>  *      backends, and even not necessarily supported for those for
>  *      which it is relevant.  A backend which knows that it is not
>  *      affected by the bug can ignore this option.
>  *
>  *      This option doesn't require a backend to use O_DIRECT, so it
>  *      should not be used to try to control the caching behaviour.
> 
> Also, BDRV_O_NATIVE_AIO is ignored if BDRV_O_NOCACHE, so clarify the
> default flags passed to the qemu block layer.
> 
> The original proposal for a "cache" backend option has been dropped
> because it was believed too wide, especially considering that at the
> moment the backend doesn't have a way to tell the toolstack that it is
> capable of supporting it.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

George, should I go ahead and commit to the qemu-xen tree?


> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 247f32f..727f433 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(directiosafe));
>  
>      /* 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,8 @@ out_error:
>      blkdev->dev = NULL;
>      g_free(blkdev->devtype);
>      blkdev->devtype = NULL;
> +    g_free(directiosafe);
> +    blkdev->directiosafe = false;
>      return -1;
>  }
>  
> @@ -783,7 +791,11 @@ static int blk_connect(struct XenDevice *xendev)
>      bool readonly = true;
>  
>      /* read-only ? */
> -    qflags = BDRV_O_CACHE_WB | BDRV_O_NATIVE_AIO;
> +    if (blkdev->directiosafe) {
> +        qflags = BDRV_O_NOCACHE | BDRV_O_NATIVE_AIO;
> +    } else {
> +        qflags = BDRV_O_CACHE_WB;
> +    }
>      if (strcmp(blkdev->mode, "w") == 0) {
>          qflags |= BDRV_O_RDWR;
>          readonly = false;
>