[PATCH] libxl: add option for discard support to xl disk configuration

[PATCH] libxl: add option for discard support to xl disk configuration

[Olaf Hering]

Handle new option discard=on|off for disk configuration. It is supposed
to disable discard support if file based backing storage was
intentionally created non-sparse to avoid fragmentation of the file.

The option is a boolean and intended for the backend driver. A new
boolean property "discard_enable" is written to the backend node. An
upcoming patch for qemu will make use of this property. The kernel
blkback driver may be updated as well to disable discard for phy based
backing storage.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 docs/misc/xl-disk-configuration.txt | 13 +++++++++++++
 tools/libxl/check-xl-disk-parse     | 21 ++++++++++++++-------
 tools/libxl/libxl.c                 |  2 ++
 tools/libxl/libxl_types.idl         |  1 +
 tools/libxl/libxlu_disk.c           |  1 +
 tools/libxl/libxlu_disk_i.h         |  2 +-
 tools/libxl/libxlu_disk_l.l         |  8 ++++++++
 xen/include/public/io/blkif.h       |  8 ++++++++
 8 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt
index 5bd456d..4f81394 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -178,6 +178,19 @@ information to be interpreted by the executable program <script>,
 These scripts are normally called "block-<script>".
 
 
+discard=<boolean>
+---------------
+
+Description:           Instruct backend to advertise discard support to frontend
+Supported values:      on, off, 0, 1
+Mandatory:             No
+Default value:         on
+
+This option instructs the backend driver, depending of the value, to advertise
+discard support (TRIM, UNMAP) to the frontend. It allows to disable "hole
+punching" for file based backends which were intentionally created non-sparse.
+
+
 
 ============================================
 DEPRECATED PARAMETERS, PREFIXES AND SYNTAXES
diff --git a/tools/libxl/check-xl-disk-parse b/tools/libxl/check-xl-disk-parse
index 797277c..485b8c6 100755
--- a/tools/libxl/check-xl-disk-parse
+++ b/tools/libxl/check-xl-disk-parse
@@ -61,7 +61,8 @@ disk: {
     "script": null,
     "removable": 0,
     "readwrite": 1,
-    "is_cdrom": 0
+    "is_cdrom": 0,
+    "discard_enable": 1
 }
 
 END
@@ -82,7 +83,8 @@ disk: {
     "script": null,
     "removable": 1,
     "readwrite": 0,
-    "is_cdrom": 1
+    "is_cdrom": 1,
+    "discard_enable": 1
 }
 
 END
@@ -104,7 +106,8 @@ disk: {
     "script": null,
     "removable": 0,
     "readwrite": 1,
-    "is_cdrom": 0
+    "is_cdrom": 0,
+    "discard_enable": 1
 }
 
 EOF
@@ -121,7 +124,8 @@ disk: {
     "script": null,
     "removable": 1,
     "readwrite": 0,
-    "is_cdrom": 1
+    "is_cdrom": 1,
+    "discard_enable": 1
 }
 
 EOF
@@ -142,7 +146,8 @@ disk: {
     "script": null,
     "removable": 1,
     "readwrite": 0,
-    "is_cdrom": 1
+    "is_cdrom": 1,
+    "discard_enable": 1
 }
 
 EOF
@@ -160,7 +165,8 @@ disk: {
     "script": "block-iscsi",
     "removable": 0,
     "readwrite": 1,
-    "is_cdrom": 0
+    "is_cdrom": 0,
+    "discard_enable": 1
 }
 
 EOF
@@ -180,7 +186,8 @@ disk: {
     "script": "block-drbd",
     "removable": 0,
     "readwrite": 1,
-    "is_cdrom": 0
+    "is_cdrom": 0,
+    "discard_enable": 1
 }
 
 EOF
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2845ca4..3633a7d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2196,6 +2196,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
         flexarray_append(back, disk->readwrite ? "w" : "r");
         flexarray_append(back, "device-type");
         flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
+        flexarray_append(back, "discard_enable");
+        flexarray_append(back, libxl__sprintf(gc, "%d", (disk->discard_enable) ? 1 : 0));
 
         flexarray_append(front, "backend-id");
         flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 649ce50..b58b198 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -415,6 +415,7 @@ libxl_device_disk = Struct("device_disk", [
     ("removable", integer),
     ("readwrite", integer),
     ("is_cdrom", integer),
+    ("discard_enable", integer),
     ])
 
 libxl_device_nic = Struct("device_nic", [
diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
index 18fe386..ee82a8d 100644
--- a/tools/libxl/libxlu_disk.c
+++ b/tools/libxl/libxlu_disk.c
@@ -58,6 +58,7 @@ int xlu_disk_parse(XLU_Config *cfg,
     dpc.disk = disk;
 
     disk->readwrite = 1;
+    disk->discard_enable = 1; /* Doing it twice?! */
 
     for (i=0; i<nspecs; i++) {
         e = dpc_prep(&dpc, specs[i]);
diff --git a/tools/libxl/libxlu_disk_i.h b/tools/libxl/libxlu_disk_i.h
index 4fccd4a..c002d02 100644
--- a/tools/libxl/libxlu_disk_i.h
+++ b/tools/libxl/libxlu_disk_i.h
@@ -10,7 +10,7 @@ typedef struct {
     void *scanner;
     YY_BUFFER_STATE buf;
     libxl_device_disk *disk;
-    int access_set, had_depr_prefix;
+    int access_set, discard_set, had_depr_prefix;
     const char *spec;
 } DiskParseContext;
 
diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index 7c4e7f1..2afd5e7 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -173,6 +173,10 @@ backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
 
 vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
 script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }
+discard=on,?	{ DPC->disk->discard_enable = 1; DPC->discard_set = 1; }
+discard=1,?	{ DPC->disk->discard_enable = 1; DPC->discard_set = 1; }
+discard=off,?	{ DPC->disk->discard_enable = 0; DPC->discard_set = 1; }
+discard=0,?	{ DPC->disk->discard_enable = 0; DPC->discard_set = 1; }
 
  /* the target magic parameter, eats the rest of the string */
 
@@ -244,6 +248,10 @@ phy:/.*		{ DPC->had_depr_prefix=1; DEPRECATE(0); }
         xlu__disk_err(DPC,yytext,"too many positional parameters");
         return 0; /* don't print any more errors */
     }
+    if (!DPC->discard_set) {
+        DPC->discard_set = 1;
+	DPC->disk->discard_enable = 1;
+    }
 }
 
 . {
diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
index 542f123..0121e19 100644
--- a/xen/include/public/io/blkif.h
+++ b/xen/include/public/io/blkif.h
@@ -175,6 +175,14 @@
  *
  *------------------------- Backend Device Properties -------------------------
  *
+ * discard_enable
+ *      Values:         0/1 (boolean)
+ *      Default Value:  1
+ *
+ *      This optional property, set by the toolstack, instructs the backend to
+ *      offer discard to the frontend. If the property is missing the backend
+ *      should offer discard if the backing storage actually supports it.
+ *
  * discard-alignment
  *      Values:         <uint32_t>
  *      Default Value:  0

[PATCH] qemu-upstream: add discard support for xen_disk

[Olaf Hering]

Implement discard support for xen_disk. It makes use of the existing
discard code in qemu.

The discard support is enabled unconditionally. The tool stack may provide a
property "discard_enable" in the backend node to optionally disable discard
support.  This is helpful in case the backing file was intentionally created
non-sparse to avoid fragmentation.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 hw/block/xen_blkif.h | 12 ++++++++++++
 hw/block/xen_disk.c  | 28 ++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/hw/block/xen_blkif.h b/hw/block/xen_blkif.h
index c0f4136..711b692 100644
--- a/hw/block/xen_blkif.h
+++ b/hw/block/xen_blkif.h
@@ -79,6 +79,12 @@ static inline void blkif_get_x86_32_req(blkif_request_t *dst, blkif_x86_32_reque
 	dst->handle = src->handle;
 	dst->id = src->id;
 	dst->sector_number = src->sector_number;
+	if (src->operation == BLKIF_OP_DISCARD) {
+		struct blkif_request_discard *s = (void *)src;
+		struct blkif_request_discard *d = (void *)dst;
+		d->nr_sectors = s->nr_sectors;
+		return;
+	}
 	if (n > src->nr_segments)
 		n = src->nr_segments;
 	for (i = 0; i < n; i++)
@@ -94,6 +100,12 @@ static inline void blkif_get_x86_64_req(blkif_request_t *dst, blkif_x86_64_reque
 	dst->handle = src->handle;
 	dst->id = src->id;
 	dst->sector_number = src->sector_number;
+	if (src->operation == BLKIF_OP_DISCARD) {
+		struct blkif_request_discard *s = (void *)src;
+		struct blkif_request_discard *d = (void *)dst;
+		d->nr_sectors = s->nr_sectors;
+		return;
+	}
 	if (n > src->nr_segments)
 		n = src->nr_segments;
 	for (i = 0; i < n; i++)
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 03e30d7..539f2ed 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -114,6 +114,7 @@ struct XenBlkDev {
     int                 requests_finished;
 
     /* Persistent grants extension */
+    gboolean            feature_discard;
     gboolean            feature_persistent;
     GTree               *persistent_gnts;
     unsigned int        persistent_gnt_count;
@@ -253,6 +254,8 @@ static int ioreq_parse(struct ioreq *ioreq)
     case BLKIF_OP_WRITE:
         ioreq->prot = PROT_READ; /* from memory */
         break;
+    case BLKIF_OP_DISCARD:
+        return 0;
     default:
         xen_be_printf(&blkdev->xendev, 0, "error: unknown operation (%d)\n",
                       ioreq->req.operation);
@@ -490,6 +493,7 @@ static void qemu_aio_complete(void *opaque, int ret)
 static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
 {
     struct XenBlkDev *blkdev = ioreq->blkdev;
+    struct blkif_request_discard *discard_req = (void *)&ioreq->req;
 
     if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
         goto err_no_map;
@@ -521,6 +525,13 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
                         &ioreq->v, ioreq->v.size / BLOCK_SIZE,
                         qemu_aio_complete, ioreq);
         break;
+    case BLKIF_OP_DISCARD:
+        bdrv_acct_start(blkdev->bs, &ioreq->acct, discard_req->nr_sectors * BLOCK_SIZE, BDRV_ACCT_WRITE);
+        ioreq->aio_inflight++;
+        bdrv_aio_discard(blkdev->bs,
+                        discard_req->sector_number, discard_req->nr_sectors,
+                        qemu_aio_complete, ioreq);
+        break;
     default:
         /* unknown operation (shouldn't happen -- parse catches this) */
         goto err;
@@ -699,6 +710,19 @@ static void blk_alloc(struct XenDevice *xendev)
     }
 }
 
+static void blk_parse_discard(struct XenBlkDev *blkdev)
+{
+    int enable;
+
+    blkdev->feature_discard = true;
+
+    if (xenstore_read_be_int(&blkdev->xendev, "discard_enable", &enable) == 0)
+	    blkdev->feature_discard = !!enable;
+
+    if (blkdev->feature_discard)
+	    xenstore_write_be_int(&blkdev->xendev, "feature-discard", 1);
+}
+
 static int blk_init(struct XenDevice *xendev)
 {
     struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, xendev);
@@ -766,6 +790,8 @@ static int blk_init(struct XenDevice *xendev)
     xenstore_write_be_int(&blkdev->xendev, "feature-persistent", 1);
     xenstore_write_be_int(&blkdev->xendev, "info", info);
 
+    blk_parse_discard(blkdev);
+
     g_free(directiosafe);
     return 0;
 
@@ -801,6 +827,8 @@ static int blk_connect(struct XenDevice *xendev)
         qflags |= BDRV_O_RDWR;
         readonly = false;
     }
+    if (blkdev->feature_discard)
+        qflags |= BDRV_O_UNMAP;
 
     /* init qemu block driver */
     index = (blkdev->xendev.dev - 202 * 256) / 16;

Re: [PATCH] qemu-upstream: add discard support for xen_disk

[Stefano Stabellini]

On Tue, 28 Jan 2014, Olaf Hering wrote:
> Implement discard support for xen_disk. It makes use of the existing
> discard code in qemu.
> 
> The discard support is enabled unconditionally. The tool stack may provide a
> property "discard_enable" in the backend node to optionally disable discard
> support.  This is helpful in case the backing file was intentionally created
> non-sparse to avoid fragmentation.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

I think that the patch is fine, thank you.
Just a small comment below but it is just a matter of taste.


>  hw/block/xen_blkif.h | 12 ++++++++++++
>  hw/block/xen_disk.c  | 28 ++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/hw/block/xen_blkif.h b/hw/block/xen_blkif.h
> index c0f4136..711b692 100644
> --- a/hw/block/xen_blkif.h
> +++ b/hw/block/xen_blkif.h
> @@ -79,6 +79,12 @@ static inline void blkif_get_x86_32_req(blkif_request_t *dst, blkif_x86_32_reque
>  	dst->handle = src->handle;
>  	dst->id = src->id;
>  	dst->sector_number = src->sector_number;
> +	if (src->operation == BLKIF_OP_DISCARD) {
> +		struct blkif_request_discard *s = (void *)src;
> +		struct blkif_request_discard *d = (void *)dst;
> +		d->nr_sectors = s->nr_sectors;
> +		return;
> +	}
>  	if (n > src->nr_segments)
>  		n = src->nr_segments;
>  	for (i = 0; i < n; i++)
> @@ -94,6 +100,12 @@ static inline void blkif_get_x86_64_req(blkif_request_t *dst, blkif_x86_64_reque
>  	dst->handle = src->handle;
>  	dst->id = src->id;
>  	dst->sector_number = src->sector_number;
> +	if (src->operation == BLKIF_OP_DISCARD) {
> +		struct blkif_request_discard *s = (void *)src;
> +		struct blkif_request_discard *d = (void *)dst;
> +		d->nr_sectors = s->nr_sectors;
> +		return;
> +	}
>  	if (n > src->nr_segments)
>  		n = src->nr_segments;
>  	for (i = 0; i < n; i++)
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 03e30d7..539f2ed 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -114,6 +114,7 @@ struct XenBlkDev {
>      int                 requests_finished;
>  
>      /* Persistent grants extension */
> +    gboolean            feature_discard;
>      gboolean            feature_persistent;
>      GTree               *persistent_gnts;
>      unsigned int        persistent_gnt_count;
> @@ -253,6 +254,8 @@ static int ioreq_parse(struct ioreq *ioreq)
>      case BLKIF_OP_WRITE:
>          ioreq->prot = PROT_READ; /* from memory */
>          break;
> +    case BLKIF_OP_DISCARD:
> +        return 0;
>      default:
>          xen_be_printf(&blkdev->xendev, 0, "error: unknown operation (%d)\n",
>                        ioreq->req.operation);
> @@ -490,6 +493,7 @@ static void qemu_aio_complete(void *opaque, int ret)
>  static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>  {
>      struct XenBlkDev *blkdev = ioreq->blkdev;
> +    struct blkif_request_discard *discard_req = (void *)&ioreq->req;

Given that ioreq->req might not be a struct blkif_request_discard*, I
would rather make the assignment under the case BLKIF_OP_DISCARD below.


>      if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
>          goto err_no_map;
> @@ -521,6 +525,13 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>                          &ioreq->v, ioreq->v.size / BLOCK_SIZE,
>                          qemu_aio_complete, ioreq);
>          break;
> +    case BLKIF_OP_DISCARD:
> +        bdrv_acct_start(blkdev->bs, &ioreq->acct, discard_req->nr_sectors * BLOCK_SIZE, BDRV_ACCT_WRITE);
> +        ioreq->aio_inflight++;
> +        bdrv_aio_discard(blkdev->bs,
> +                        discard_req->sector_number, discard_req->nr_sectors,
> +                        qemu_aio_complete, ioreq);
> +        break;
>      default:
>          /* unknown operation (shouldn't happen -- parse catches this) */
>          goto err;
> @@ -699,6 +710,19 @@ static void blk_alloc(struct XenDevice *xendev)
>      }
>  }
>  
> +static void blk_parse_discard(struct XenBlkDev *blkdev)
> +{
> +    int enable;
> +
> +    blkdev->feature_discard = true;
> +
> +    if (xenstore_read_be_int(&blkdev->xendev, "discard_enable", &enable) == 0)
> +	    blkdev->feature_discard = !!enable;
> +
> +    if (blkdev->feature_discard)
> +	    xenstore_write_be_int(&blkdev->xendev, "feature-discard", 1);
> +}
> +
>  static int blk_init(struct XenDevice *xendev)
>  {
>      struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, xendev);
> @@ -766,6 +790,8 @@ static int blk_init(struct XenDevice *xendev)
>      xenstore_write_be_int(&blkdev->xendev, "feature-persistent", 1);
>      xenstore_write_be_int(&blkdev->xendev, "info", info);
>  
> +    blk_parse_discard(blkdev);
> +
>      g_free(directiosafe);
>      return 0;
>  
> @@ -801,6 +827,8 @@ static int blk_connect(struct XenDevice *xendev)
>          qflags |= BDRV_O_RDWR;
>          readonly = false;
>      }
> +    if (blkdev->feature_discard)
> +        qflags |= BDRV_O_UNMAP;
>  
>      /* init qemu block driver */
>      index = (blkdev->xendev.dev - 202 * 256) / 16;
> 

Re: [PATCH] qemu-upstream: add discard support for xen_disk

[Ian Campbell]

On Tue, 2014-01-28 at 20:10 +0000, Stefano Stabellini wrote:

> > +    if (xenstore_read_be_int(&blkdev->xendev, "discard_enable", &enable) == 0)

If this is the first implementation (i.e. there are no others in the
wild yet) then please can we use "discard-enable" for consistency with
all the other properties.

Ian.

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Ian Campbell]

On Tue, 2014-01-28 at 19:24 +0100, Olaf Hering wrote:
> Handle new option discard=on|off for disk configuration. It is supposed
> to disable discard support if file based backing storage was
> intentionally created non-sparse to avoid fragmentation of the file.
> 
> The option is a boolean and intended for the backend driver. A new
> boolean property "discard_enable" is written to the backend node. An
> upcoming patch for qemu will make use of this property. The kernel
> blkback driver may be updated as well to disable discard for phy based
> backing storage.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  docs/misc/xl-disk-configuration.txt | 13 +++++++++++++
>  tools/libxl/check-xl-disk-parse     | 21 ++++++++++++++-------
>  tools/libxl/libxl.c                 |  2 ++
>  tools/libxl/libxl_types.idl         |  1 +
>  tools/libxl/libxlu_disk.c           |  1 +
>  tools/libxl/libxlu_disk_i.h         |  2 +-
>  tools/libxl/libxlu_disk_l.l         |  8 ++++++++
>  xen/include/public/io/blkif.h       |  8 ++++++++
>  8 files changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt
> index 5bd456d..4f81394 100644
> --- a/docs/misc/xl-disk-configuration.txt
> +++ b/docs/misc/xl-disk-configuration.txt
> @@ -178,6 +178,19 @@ information to be interpreted by the executable program <script>,
>  These scripts are normally called "block-<script>".
>  
> 
> +discard=<boolean>
> +---------------
> +
> +Description:           Instruct backend to advertise discard support to frontend
> +Supported values:      on, off, 0, 1
> +Mandatory:             No
> +Default value:         on

I think this default should be "on if, available for that backend type".

What happens if the backed does not support discard?

> +This option instructs the backend driver, depending of the value, to advertise
> +discard support (TRIM, UNMAP) to the frontend. It allows to disable "hole
> +punching" for file based backends which were intentionally created non-sparse.
> +
> +
>  
>  ============================================
>  DEPRECATED PARAMETERS, PREFIXES AND SYNTAXES

[...]
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 649ce50..b58b198 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -415,6 +415,7 @@ libxl_device_disk = Struct("device_disk", [
>      ("removable", integer),
>      ("readwrite", integer),
>      ("is_cdrom", integer),
> +    ("discard_enable", integer),

I have a feeling this should be a libxl_defbool, to allow for the
possibility of "libxl does what is best/lets the backend decide".

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2845ca4..3633a7d 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2196,6 +2196,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
>          flexarray_append(back, disk->readwrite ? "w" : "r");
>          flexarray_append(back, "device-type");
>          flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
> +        flexarray_append(back, "discard_enable");
> +        flexarray_append(back, libxl__sprintf(gc, "%d", (disk->discard_enable) ? 1 : 0));


And if this were a defbool then you'd want to use libxl_defbool_is_default: i.e.
	if (!libxl_defbool_is_default(disk->discard_enable))
		flexarray_append(back, ..., libxl_defbool_val(...) ? "1" : "0"))

(note the lack of libxl_sprintf here too).

>  
>          flexarray_append(front, "backend-id");
>          flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));

>      ])
>  
>  libxl_device_nic = Struct("device_nic", [
> diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
> index 18fe386..ee82a8d 100644
> --- a/tools/libxl/libxlu_disk.c
> +++ b/tools/libxl/libxlu_disk.c
> @@ -58,6 +58,7 @@ int xlu_disk_parse(XLU_Config *cfg,
>      dpc.disk = disk;
>  
>      disk->readwrite = 1;
> +    disk->discard_enable = 1; /* Doing it twice?! */

Why?

> diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
> index 7c4e7f1..2afd5e7 100644
> --- a/tools/libxl/libxlu_disk_l.l
> +++ b/tools/libxl/libxlu_disk_l.l
> @@ -173,6 +173,10 @@ backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
>  
>  vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
>  script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }
> +discard=on,?	{ DPC->disk->discard_enable = 1; DPC->discard_set = 1; }
> +discard=1,?	{ DPC->disk->discard_enable = 1; DPC->discard_set = 1; }
> +discard=off,?	{ DPC->disk->discard_enable = 0; DPC->discard_set = 1; }
> +discard=0,?	{ DPC->disk->discard_enable = 0; DPC->discard_set = 1; }
>  
>   /* the target magic parameter, eats the rest of the string */
>  
> @@ -244,6 +248,10 @@ phy:/.*		{ DPC->had_depr_prefix=1; DEPRECATE(0); }
>          xlu__disk_err(DPC,yytext,"too many positional parameters");
>          return 0; /* don't print any more errors */
>      }
> +    if (!DPC->discard_set) {
> +        DPC->discard_set = 1;
> +	DPC->disk->discard_enable = 1;

Indentation is messed up here.

> +    }
>  }
>  
>  . {
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 542f123..0121e19 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -175,6 +175,14 @@
>   *
>   *------------------------- Backend Device Properties -------------------------
>   *
> + * discard_enable

All of the existing properties use - not _.

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Olaf Hering]

On Wed, Jan 29, Ian Campbell wrote:

> > +Default value:         on
> 
> I think this default should be "on if, available for that backend type".

Ok, will make this change.

> What happens if the backed does not support discard?

The toolstack just does not know if a phy device supports it, or if file
backed storage can do hole punching. If feature-discard is set and the
frontend sends a discard request, the backend would return an error
(like ENOTSUPPORTED) and the frontend internally disables the discard
flag. Thats how it is done in pvops and the forward ported xenlinux
tree.

Up to now I have not prepared a change for the backend drivers. They
could either force feature-discard to be true so that the error paths
will be executed. Or they could ignore the discard-enable if the backing
storage does not support discard.

> >      disk->readwrite = 1;
> > +    disk->discard_enable = 1; /* Doing it twice?! */
> 
> Why?

Thats what I'm asking you. Why is readwrite set here, and later on also
in the .l file? At least just setting it here did not unconditionally
enable it if no discard= was specified. I have not traced the code why
that happens.

> > + * discard_enable
> 
> All of the existing properties use - not _.

I will make this change.

Thanks.

Olaf

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Ian Campbell]

On Wed, 2014-01-29 at 12:19 +0100, Olaf Hering wrote:
> On Wed, Jan 29, Ian Campbell wrote:
> 
> > > +Default value:         on
> > 
> > I think this default should be "on if, available for that backend type".
> 
> Ok, will make this change.
> 
> > What happens if the backed does not support discard?
> 
> The toolstack just does not know if a phy device supports it, or if file
> backed storage can do hole punching. If feature-discard is set and the
> frontend sends a discard request, the backend would return an error
> (like ENOTSUPPORTED) and the frontend internally disables the discard
> flag. Thats how it is done in pvops and the forward ported xenlinux
> tree.

That sounds good.

Is it worth noting that enable-discard=1 is only advisory and will be
ignored if the underlying storage and/or backend doesn't understand it?
The real benefit of this option is to be able to force it off rather
than on I think.

> Up to now I have not prepared a change for the backend drivers. They
> could either force feature-discard to be true so that the error paths
> will be executed. Or they could ignore the discard-enable if the backing
> storage does not support discard.
> 
> > >      disk->readwrite = 1;
> > > +    disk->discard_enable = 1; /* Doing it twice?! */
> > 
> > Why?
> 
> Thats what I'm asking you. Why is readwrite set here, and later on also
> in the .l file? At least just setting it here did not unconditionally
> enable it if no discard= was specified. I have not traced the code why
> that happens.

One for Ian J I think. Perhaps it is just setting the default?

readwrite is on unless you say "ro" in the config, so I suppose that
makes sense. I don't know about discard_enable -- if this were a defbool
it would probably go away anyway.

Ian.

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Olaf Hering]

On Wed, Jan 29, Ian Campbell wrote:

> On Wed, 2014-01-29 at 12:19 +0100, Olaf Hering wrote:
> > The toolstack just does not know if a phy device supports it, or if file
> > backed storage can do hole punching. If feature-discard is set and the
> > frontend sends a discard request, the backend would return an error
> > (like ENOTSUPPORTED) and the frontend internally disables the discard
> > flag. Thats how it is done in pvops and the forward ported xenlinux
> > tree.
> 
> That sounds good.
> 
> Is it worth noting that enable-discard=1 is only advisory and will be
> ignored if the underlying storage and/or backend doesn't understand it?
> The real benefit of this option is to be able to force it off rather
> than on I think.

Yes, the purpose is to turn it off. I will extend the description.

> > Thats what I'm asking you. Why is readwrite set here, and later on also
> > in the .l file? At least just setting it here did not unconditionally
> > enable it if no discard= was specified. I have not traced the code why
> > that happens.
> 
> One for Ian J I think. Perhaps it is just setting the default?

For some reason this setting is lost, at least for my discard flag.  The
readwrite part may suffer from the same issue, otherwise access_set
could be removed.  I will trace the code to understand how it works.

Olaf

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Olaf Hering]

On Wed, Jan 29, Olaf Hering wrote:

> On Wed, Jan 29, Ian Campbell wrote:
> > On Wed, 2014-01-29 at 12:19 +0100, Olaf Hering wrote:
> > > Thats what I'm asking you. Why is readwrite set here, and later on also
> > > in the .l file? At least just setting it here did not unconditionally
> > > enable it if no discard= was specified. I have not traced the code why
> > > that happens.
> > 
> > One for Ian J I think. Perhaps it is just setting the default?
> 
> For some reason this setting is lost, at least for my discard flag.  The
> readwrite part may suffer from the same issue, otherwise access_set
> could be removed.  I will trace the code to understand how it works.

I think the failure was caused by incompatibility between libxl.so and xl
because I did just copy libs around.

Its working fine with a fresh rpm package. I will remove the discard_set
variable.


Olaf

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Olaf Hering]

On Wed, Jan 29, Ian Campbell wrote:

> On Tue, 2014-01-28 at 19:24 +0100, Olaf Hering wrote:
> > +    ("discard_enable", integer),
> I have a feeling this should be a libxl_defbool, to allow for the
> possibility of "libxl does what is best/lets the backend decide".
> 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 2845ca4..3633a7d 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -2196,6 +2196,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
> >          flexarray_append(back, disk->readwrite ? "w" : "r");
> >          flexarray_append(back, "device-type");
> >          flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
> > +        flexarray_append(back, "discard_enable");
> > +        flexarray_append(back, libxl__sprintf(gc, "%d", (disk->discard_enable) ? 1 : 0));
> And if this were a defbool then you'd want to use libxl_defbool_is_default: i.e.
> 	if (!libxl_defbool_is_default(disk->discard_enable))
> 		flexarray_append(back, ..., libxl_defbool_val(...) ? "1" : "0"))
> 
> (note the lack of libxl_sprintf here too).

Did you have something like this in mind? Its all it takes.

Olaf


diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2845ca4..bbaf450 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2196,6 +2196,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
         flexarray_append(back, disk->readwrite ? "w" : "r");
         flexarray_append(back, "device-type");
         flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
+        if (!libxl_defbool_is_default(disk->discard_enable))
+            flexarray_append_pair(back, "discard-enable", libxl_defbool_val(disk->discard_enable) ? "1" : "0");
 
         flexarray_append(front, "backend-id");
         flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 649ce50..6575515 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -415,6 +415,7 @@ libxl_device_disk = Struct("device_disk", [
     ("removable", integer),
     ("readwrite", integer),
     ("is_cdrom", integer),
+    ("discard_enable", libxl_defbool),
     ])
 
 libxl_device_nic = Struct("device_nic", [
diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index 7c4e7f1..2585bee 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -173,6 +173,10 @@ backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
 
 vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
 script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }
+discard=on,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
+discard=1,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
+discard=off,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }
+discard=0,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }
 
  /* the target magic parameter, eats the rest of the string */

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Ian Campbell]

On Thu, 2014-01-30 at 11:56 +0100, Olaf Hering wrote:
> On Wed, Jan 29, Ian Campbell wrote:
> 
> > On Tue, 2014-01-28 at 19:24 +0100, Olaf Hering wrote:
> > > +    ("discard_enable", integer),
> > I have a feeling this should be a libxl_defbool, to allow for the
> > possibility of "libxl does what is best/lets the backend decide".
> > 
> > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > index 2845ca4..3633a7d 100644
> > > --- a/tools/libxl/libxl.c
> > > +++ b/tools/libxl/libxl.c
> > > @@ -2196,6 +2196,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
> > >          flexarray_append(back, disk->readwrite ? "w" : "r");
> > >          flexarray_append(back, "device-type");
> > >          flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
> > > +        flexarray_append(back, "discard_enable");
> > > +        flexarray_append(back, libxl__sprintf(gc, "%d", (disk->discard_enable) ? 1 : 0));
> > And if this were a defbool then you'd want to use libxl_defbool_is_default: i.e.
> > 	if (!libxl_defbool_is_default(disk->discard_enable))
> > 		flexarray_append(back, ..., libxl_defbool_val(...) ? "1" : "0"))
> > 
> > (note the lack of libxl_sprintf here too).
> 
> Did you have something like this in mind? Its all it takes.

Looks about right, yes (modulo the over long line)

Thanks.
Ian.

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Olaf Hering]

On Tue, Jan 28, Olaf Hering wrote:

> Handle new option discard=on|off for disk configuration. It is supposed
> to disable discard support if file based backing storage was
> intentionally created non-sparse to avoid fragmentation of the file.

> +++ b/tools/libxl/libxl_types.idl
> @@ -415,6 +415,7 @@ libxl_device_disk = Struct("device_disk", [
>      ("removable", integer),
>      ("readwrite", integer),
>      ("is_cdrom", integer),
> +    ("discard_enable", integer),

This new field changes the API, _libxl_types.h:struct libxl_device_disk
gets a new member. How should code using this new flag recognize if its
present? If it is supposed to be part of a new libxl-4.5 API then
out-of-tree code could put the code into #ifdef LIBXL_API_VERSION >= X.
If not, how should it be done?

For my own purpose I will overload ->readwrite to carry the discard flag
and to preserve the ABI.

Olaf

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Ian Campbell]

On Wed, 2014-01-29 at 16:06 +0100, Olaf Hering wrote:
> On Tue, Jan 28, Olaf Hering wrote:
> 
> > Handle new option discard=on|off for disk configuration. It is supposed
> > to disable discard support if file based backing storage was
> > intentionally created non-sparse to avoid fragmentation of the file.
> 
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -415,6 +415,7 @@ libxl_device_disk = Struct("device_disk", [
> >      ("removable", integer),
> >      ("readwrite", integer),
> >      ("is_cdrom", integer),
> > +    ("discard_enable", integer),
> 
> This new field changes the API, _libxl_types.h:struct libxl_device_disk
> gets a new member. How should code using this new flag recognize if its
> present? If it is supposed to be part of a new libxl-4.5 API then
> out-of-tree code could put the code into #ifdef LIBXL_API_VERSION >= X.
> If not, how should it be done?

You should add a #define LIBXL_HAVE_FOO to libxl.h, there are a few
examples in there already.

There is no need to make the actual field conditional -- that would
actually be wrong since it would modify the ABI depending on what the
application asked for, meaning it would differ from how libxl was
actually built. An application which us using an ABI before 4.5 simply
won't think to touch this field.

> 
> For my own purpose I will overload ->readwrite to carry the discard flag
> and to preserve the ABI.
> 
> Olaf

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Olaf Hering]

On Wed, Jan 29, Ian Campbell wrote:

> On Wed, 2014-01-29 at 16:06 +0100, Olaf Hering wrote:
> > This new field changes the API, _libxl_types.h:struct libxl_device_disk
> > gets a new member. How should code using this new flag recognize if its
> > present? If it is supposed to be part of a new libxl-4.5 API then
> > out-of-tree code could put the code into #ifdef LIBXL_API_VERSION >= X.
> > If not, how should it be done?
> You should add a #define LIBXL_HAVE_FOO to libxl.h, there are a few
> examples in there already.

I will add such a define.

> There is no need to make the actual field conditional -- that would
> actually be wrong since it would modify the ABI depending on what the
> application asked for, meaning it would differ from how libxl was
> actually built. An application which us using an ABI before 4.5 simply
> won't think to touch this field.

I meant the access of the field in libvirt, like "p->discard_enable = val;".
Putting such code into #ifdef LIBXL_HAVE_FOO is fine.

Olaf

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Ian Campbell]

On Wed, 2014-01-29 at 17:07 +0100, Olaf Hering wrote:
> On Wed, Jan 29, Ian Campbell wrote:
> 
> > On Wed, 2014-01-29 at 16:06 +0100, Olaf Hering wrote:
> > > This new field changes the API, _libxl_types.h:struct libxl_device_disk
> > > gets a new member. How should code using this new flag recognize if its
> > > present? If it is supposed to be part of a new libxl-4.5 API then
> > > out-of-tree code could put the code into #ifdef LIBXL_API_VERSION >= X.
> > > If not, how should it be done?
> > You should add a #define LIBXL_HAVE_FOO to libxl.h, there are a few
> > examples in there already.
> 
> I will add such a define.

THanks.

> > There is no need to make the actual field conditional -- that would
> > actually be wrong since it would modify the ABI depending on what the
> > application asked for, meaning it would differ from how libxl was
> > actually built. An application which us using an ABI before 4.5 simply
> > won't think to touch this field.
> 
> I meant the access of the field in libvirt, like "p->discard_enable = val;".
> Putting such code into #ifdef LIBXL_HAVE_FOO is fine.

Yes, I misunderstood what you meant.

The applications choices are to #define LIBXL_API_VERSION to a big
enough number or to make things conditional on the appropriate
LIBXL_HAVE_FOO. AIUI libvirt has chosen to use the LIBXL_HAVE option.

Ian.

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] libxl: add option for discard support to xl disk configuration"):
> There is no need to make the actual field conditional -- that would
> actually be wrong since it would modify the ABI depending on what the
> application asked for, meaning it would differ from how libxl was
> actually built. An application which us using an ABI before 4.5 simply
> won't think to touch this field.

You mean an application which is using an API (P not B) before 4.5.

I.e. the API is forward-compatible but the ABI is compatible only
within a Xen release.

Ian.

Re: [PATCH] libxl: add option for discard support to xl disk configuration

[Ian Campbell]

On Thu, 2014-01-30 at 11:32 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH] libxl: add option for discard support to xl disk configuration"):
> > There is no need to make the actual field conditional -- that would
> > actually be wrong since it would modify the ABI depending on what the
> > application asked for, meaning it would differ from how libxl was
> > actually built. An application which us using an ABI before 4.5 simply
> > won't think to touch this field.
> 
> You mean an application which is using an API (P not B) before 4.5.

Yes, sorry.

> I.e. the API is forward-compatible but the ABI is compatible only
> within a Xen release.

Correct.

Ian.