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

[PATCH v2] 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.

v2:
rename xenstore property from discard_enable to discard-enable
update description in xl-disk-configuration.txt
use libxl_defbool as type for discard_enable
update check-xl-disk-parse to use "<default>"
add LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE to libxl.h

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

diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt
index 5bd456d..c9fd9bd 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -178,6 +178,21 @@ 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 if, available for that backend typ
+
+This option is an advisory setting for the backend driver, depending of the
+value, to advertise discard support (TRIM, UNMAP) to the frontend. The real
+benefit of this option is to be able to force it off rather than on. It allows
+to disable "hole punching" for file based backends which were intentionally
+created non-sparse to avoid fragmentation of the file.
+
+
 
 ============================================
 DEPRECATED PARAMETERS, PREFIXES AND SYNTAXES
diff --git a/tools/libxl/check-xl-disk-parse b/tools/libxl/check-xl-disk-parse
index 797277c..4d26ca3 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": "<default>"
 }
 
 END
@@ -82,7 +83,8 @@ disk: {
     "script": null,
     "removable": 1,
     "readwrite": 0,
-    "is_cdrom": 1
+    "is_cdrom": 1,
+    "discard_enable": "<default>"
 }
 
 END
@@ -104,7 +106,8 @@ disk: {
     "script": null,
     "removable": 0,
     "readwrite": 1,
-    "is_cdrom": 0
+    "is_cdrom": 0,
+    "discard_enable": "<default>"
 }
 
 EOF
@@ -121,7 +124,8 @@ disk: {
     "script": null,
     "removable": 1,
     "readwrite": 0,
-    "is_cdrom": 1
+    "is_cdrom": 1,
+    "discard_enable": "<default>"
 }
 
 EOF
@@ -142,7 +146,8 @@ disk: {
     "script": null,
     "removable": 1,
     "readwrite": 0,
-    "is_cdrom": 1
+    "is_cdrom": 1,
+    "discard_enable": "<default>"
 }
 
 EOF
@@ -160,7 +165,8 @@ disk: {
     "script": "block-iscsi",
     "removable": 0,
     "readwrite": 1,
-    "is_cdrom": 0
+    "is_cdrom": 0,
+    "discard_enable": "<default>"
 }
 
 EOF
@@ -180,7 +186,8 @@ disk: {
     "script": "block-drbd",
     "removable": 0,
     "readwrite": 1,
-    "is_cdrom": 0
+    "is_cdrom": 0,
+    "discard_enable": "<default>"
 }
 
 EOF
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2845ca4..64d081b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2196,6 +2196,9 @@ 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.h b/tools/libxl/libxl.h
index 12d6c31..f26ef3c 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -95,6 +95,11 @@
 #define LIBXL_HAVE_BUILDINFO_EVENT_CHANNELS 1
 
 /*
+ * The libxl_device_disk has the discard_enable field.
+ */
+#define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE 1
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
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 */
 
diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
index 542f123..4704fe6 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 v2] 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.

v2:
rename xenstore property from discard_enable to discard-enable
move discard_req to case BLKIF_OP_DISCARD

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 hw/block/xen_blkif.h | 12 ++++++++++++
 hw/block/xen_disk.c  | 30 ++++++++++++++++++++++++++++++
 2 files changed, 42 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..a1f1c7e 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);
@@ -521,6 +524,16 @@ 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:
+    {
+        struct blkif_request_discard *discard_req = (void *)&ioreq->req;
+        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 +712,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 +792,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 +829,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 v2] qemu-upstream: add discard support for xen_disk

[Stefano Stabellini]

On Thu, 30 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.
> 
> v2:
> rename xenstore property from discard_enable to discard-enable
> move discard_req to case BLKIF_OP_DISCARD
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

The patch is fine by me but it has few simple style issues.
Please run it through scripts/checkpatch.pl in QEMU and resend (CC'ing
qemu-devel too).
Thanks!


>  hw/block/xen_blkif.h | 12 ++++++++++++
>  hw/block/xen_disk.c  | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 42 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..a1f1c7e 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);
> @@ -521,6 +524,16 @@ 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:
> +    {
> +        struct blkif_request_discard *discard_req = (void *)&ioreq->req;
> +        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 +712,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 +792,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 +829,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 v2] libxl: add option for discard support to xl disk configuration

[Olaf Hering]

On Thu, Jan 30, Olaf Hering wrote:

> boolean property "discard_enable" is written to the backend node. An

This should have been "discard-enable". Will resend with updated commit
message once the tree is open again for new features.

Olaf

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

[Ian Campbell]

On Thu, 2014-01-30 at 16:32 +0100, Olaf Hering wrote:
> On Thu, Jan 30, Olaf Hering wrote:
> 
> > boolean property "discard_enable" is written to the backend node. An
> 
> This should have been "discard-enable". Will resend with updated commit
> message

I think I can probably remember to do this on commit...

>  once the tree is open again for new features.

...but since it would be worth pinging then anyway I guess you may as
well resend the patch then too.

Ian.

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

[Olaf Hering]

And just for reference, this is a version for our 4.4:

....

This change does not break ABI. Instead of adding a new member ->discard_enable
to struct libxl_device_disk the existing ->readwrite member is reused.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 docs/misc/xl-disk-configuration.txt | 15 +++++++++++++++
 tools/libxl/libxl.c                 |  2 ++
 tools/libxl/libxl.h                 | 11 +++++++++++
 tools/libxl/libxlu_disk.c           |  3 +++
 tools/libxl/libxlu_disk_i.h         |  2 +-
 tools/libxl/libxlu_disk_l.l         |  4 ++++
 6 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt
index 5bd456d..c9fd9bd 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -178,6 +178,21 @@ 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 if, available for that backend typ
+
+This option is an advisory setting for the backend driver, depending of the
+value, to advertise discard support (TRIM, UNMAP) to the frontend. The real
+benefit of this option is to be able to force it off rather than on. It allows
+to disable "hole punching" for file based backends which were intentionally
+created non-sparse to avoid fragmentation of the file.
+
+
 
 ============================================
 DEPRECATED PARAMETERS, PREFIXES AND SYNTAXES
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2845ca4..9ed5062 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 (disk->readwrite == LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_DISABLE_MAGIC)
+            flexarray_append_pair(back, "discard-enable", "0");
 
         flexarray_append(front, "backend-id");
         flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 12d6c31..021f7e4 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -95,6 +95,17 @@
 #define LIBXL_HAVE_BUILDINFO_EVENT_CHANNELS 1
 
 /*
+ * The libxl_device_disk lacks discard_enable field, disabling discard
+ * is supported without breaking the ABI. This is done by overloading
+ * struct libxl_device_disk->readwrite:
+ * readwrite == 0: disk is readonly, no discard
+ * readwrite == 1: disk is readwrite, backend driver may enable discard
+ * readwrite == MAGIC: disk is readwrite, backend driver should not offer
+ * discard to the frontend driver.
+ */
+#define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_DISABLE_MAGIC 0xdcadU
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
index 18fe386..e596cb6 100644
--- a/tools/libxl/libxlu_disk.c
+++ b/tools/libxl/libxlu_disk.c
@@ -80,6 +80,9 @@ int xlu_disk_parse(XLU_Config *cfg,
             disk->format = LIBXL_DISK_FORMAT_EMPTY;
     }
 
+    if (disk->readwrite && dpc.disable_discard)
+        disk->readwrite = LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_DISABLE_MAGIC;
+
     if (!disk->vdev) {
         xlu__disk_err(&dpc,0, "no vdev specified");
         goto x_err;
diff --git a/tools/libxl/libxlu_disk_i.h b/tools/libxl/libxlu_disk_i.h
index 4fccd4a..9db3002 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, disable_discard, 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..ecc30ae 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->disable_discard = 0; }
+discard=1,?	{ DPC->disable_discard = 0; }
+discard=off,?	{ DPC->disable_discard = 1; }
+discard=0,?	{ DPC->disable_discard = 1; }
 
  /* the target magic parameter, eats the rest of the string */

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

[Ian Campbell]

On Thu, 2014-01-30 at 17:25 +0100, Olaf Hering wrote:
> And just for reference, this is a version for our 4.4:
> 
> ....
> 
> This change does not break ABI. Instead of adding a new member ->discard_enable
> to struct libxl_device_disk the existing ->readwrite member is reused.

Looks like it changes the libxlu ABI though. Or maybe that's totally
internal?

TBH -- if you (==suse I guess?) are contemplating carrying this as a
backport even before 4.4 is out the door we should probably be at least
considering a freeze exception for 4.4. George CCd for input. (I
appreciate that "backport=>freeze exception" is a potentially slippery
slope/ripe for abuse...)

> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  docs/misc/xl-disk-configuration.txt | 15 +++++++++++++++
>  tools/libxl/libxl.c                 |  2 ++
>  tools/libxl/libxl.h                 | 11 +++++++++++
>  tools/libxl/libxlu_disk.c           |  3 +++
>  tools/libxl/libxlu_disk_i.h         |  2 +-
>  tools/libxl/libxlu_disk_l.l         |  4 ++++
>  6 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt
> index 5bd456d..c9fd9bd 100644
> --- a/docs/misc/xl-disk-configuration.txt
> +++ b/docs/misc/xl-disk-configuration.txt
> @@ -178,6 +178,21 @@ 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 if, available for that backend typ
> +
> +This option is an advisory setting for the backend driver, depending of the
> +value, to advertise discard support (TRIM, UNMAP) to the frontend. The real
> +benefit of this option is to be able to force it off rather than on. It allows
> +to disable "hole punching" for file based backends which were intentionally
> +created non-sparse to avoid fragmentation of the file.
> +
> +
>  
>  ============================================
>  DEPRECATED PARAMETERS, PREFIXES AND SYNTAXES
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2845ca4..9ed5062 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 (disk->readwrite == LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_DISABLE_MAGIC)
> +            flexarray_append_pair(back, "discard-enable", "0");
>  
>          flexarray_append(front, "backend-id");
>          flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 12d6c31..021f7e4 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -95,6 +95,17 @@
>  #define LIBXL_HAVE_BUILDINFO_EVENT_CHANNELS 1
>  
>  /*
> + * The libxl_device_disk lacks discard_enable field, disabling discard
> + * is supported without breaking the ABI. This is done by overloading
> + * struct libxl_device_disk->readwrite:
> + * readwrite == 0: disk is readonly, no discard
> + * readwrite == 1: disk is readwrite, backend driver may enable discard
> + * readwrite == MAGIC: disk is readwrite, backend driver should not offer
> + * discard to the frontend driver.
> + */
> +#define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_DISABLE_MAGIC 0xdcadU
> +
> +/*
>   * libxl ABI compatibility
>   *
>   * The only guarantee which libxl makes regarding ABI compatibility
> diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
> index 18fe386..e596cb6 100644
> --- a/tools/libxl/libxlu_disk.c
> +++ b/tools/libxl/libxlu_disk.c
> @@ -80,6 +80,9 @@ int xlu_disk_parse(XLU_Config *cfg,
>              disk->format = LIBXL_DISK_FORMAT_EMPTY;
>      }
>  
> +    if (disk->readwrite && dpc.disable_discard)
> +        disk->readwrite = LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_DISABLE_MAGIC;
> +
>      if (!disk->vdev) {
>          xlu__disk_err(&dpc,0, "no vdev specified");
>          goto x_err;
> diff --git a/tools/libxl/libxlu_disk_i.h b/tools/libxl/libxlu_disk_i.h
> index 4fccd4a..9db3002 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, disable_discard, 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..ecc30ae 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->disable_discard = 0; }
> +discard=1,?	{ DPC->disable_discard = 0; }
> +discard=off,?	{ DPC->disable_discard = 1; }
> +discard=0,?	{ DPC->disable_discard = 1; }
>  
>   /* the target magic parameter, eats the rest of the string */
>  
> 
> 

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

[Olaf Hering]

On Thu, Jan 30, Ian Campbell wrote:

> On Thu, 2014-01-30 at 17:25 +0100, Olaf Hering wrote:
> > This change does not break ABI. Instead of adding a new member ->discard_enable
> > to struct libxl_device_disk the existing ->readwrite member is reused.
> Looks like it changes the libxlu ABI though. Or maybe that's totally
> internal?

DiskParseContext is used internal, there is no public header for it.

> TBH -- if you (==suse I guess?) are contemplating carrying this as a
> backport even before 4.4 is out the door we should probably be at least
> considering a freeze exception for 4.4. George CCd for input. (I
> appreciate that "backport=>freeze exception" is a potentially slippery
> slope/ripe for abuse...)

It will make less work for SUSE if this change would be incorporated
into 4.4, and later replaced with the "final" version I sent out today.
However, its small and will be easy to port forward to 4.4.X.

The risk of including such change is small as it requires a patched qemu
which actually does discard (1.7?), a patched frontend driver (pvops
3.15?) before the codepaths it enables are actually executed.

Olaf

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

[Ian Campbell]

On Thu, 2014-01-30 at 18:30 +0100, Olaf Hering wrote:

George, any thoughts on:

> > TBH -- if you (==suse I guess?) are contemplating carrying this as a
> > backport even before 4.4 is out the door we should probably be at least
> > considering a freeze exception for 4.4. George CCd for input. (I
> > appreciate that "backport=>freeze exception" is a potentially slippery
> > slope/ripe for abuse...)
> 
> It will make less work for SUSE if this change would be incorporated
> into 4.4, and later replaced with the "final" version I sent out today.
> However, its small and will be easy to port forward to 4.4.X.
> 
> The risk of including such change is small as it requires a patched qemu
> which actually does discard (1.7?), a patched frontend driver (pvops
> 3.15?) before the codepaths it enables are actually executed.
> 
> Olaf
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[George Dunlap]

On 02/04/2014 04:22 PM, Ian Campbell wrote:
> On Thu, 2014-01-30 at 18:30 +0100, Olaf Hering wrote:
>
> George, any thoughts on:
>
>>> TBH -- if you (==suse I guess?) are contemplating carrying this as a
>>> backport even before 4.4 is out the door we should probably be at least
>>> considering a freeze exception for 4.4. George CCd for input. (I
>>> appreciate that "backport=>freeze exception" is a potentially slippery
>>> slope/ripe for abuse...)
>> It will make less work for SUSE if this change would be incorporated
>> into 4.4, and later replaced with the "final" version I sent out today.
>> However, its small and will be easy to port forward to 4.4.X.
>>
>> The risk of including such change is small as it requires a patched qemu
>> which actually does discard (1.7?), a patched frontend driver (pvops
>> 3.15?) before the codepaths it enables are actually executed.

Well it looks like in order to keep ABI compatibility (which I don't 
think we ever promised), you're introducing this weird hack with 
overloading a putative boolean value with a magic number?

I think the patch is really ugly.  I assume the reason you're attemping 
to avoid breaking ABI compatibility is because we're so close to the 
release? But if so, adding an ugly hack like this is worse, IMHO.

  -George

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

[Olaf Hering]

On Wed, Feb 05, George Dunlap wrote:

> Well it looks like in order to keep ABI compatibility (which I don't think
> we ever promised), you're introducing this weird hack with overloading a
> putative boolean value with a magic number?

Yes, thats the point. If libxl_device_disk changes then IMO the SONAME
has to change as well. And is 4.4-rc4 is the right time to do that?
Likely not. I'm fine with carry the 4.4 patch to achieve the result, and
put the other version into 4.5.

Olaf

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

[Ian Campbell]

On Wed, 2014-02-05 at 12:36 +0100, Olaf Hering wrote:
> On Wed, Feb 05, George Dunlap wrote:
> 
> > Well it looks like in order to keep ABI compatibility (which I don't think
> > we ever promised), you're introducing this weird hack with overloading a
> > putative boolean value with a magic number?
> 
> Yes, thats the point. If libxl_device_disk changes then IMO the SONAME
> has to change as well. And is 4.4-rc4 is the right time to do that?

The SONAME is currently 4.3, so it looks like we need to bump the SONAME
anyways (since it seems unlikely that we have not changed the ABI since
4.3). 

This normally happens fairly late on in the release cycle, although TBH
it should probably have happened by now.

> Likely not.

So perhaps not as unlikely ;-)

>  I'm fine with carry the 4.4 patch to achieve the result, and
> put the other version into 4.5.
> 
> Olaf
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Olaf Hering]

On Wed, Feb 05, Ian Campbell wrote:

> The SONAME is currently 4.3, so it looks like we need to bump the SONAME
> anyways (since it seems unlikely that we have not changed the ABI since
> 4.3). 

Looking at git diff RELEASE-4.3.0.. tools/libxl/libxl.h I see that at
least libxl_domain_create_restore got a new parameter.

Olaf

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

[Ian Campbell]

On Wed, 2014-02-05 at 13:27 +0100, Olaf Hering wrote:
> On Wed, Feb 05, Ian Campbell wrote:
> 
> > The SONAME is currently 4.3, so it looks like we need to bump the SONAME
> > anyways (since it seems unlikely that we have not changed the ABI since
> > 4.3). 
> 
> Looking at git diff RELEASE-4.3.0.. tools/libxl/libxl.h I see that at
> least libxl_domain_create_restore got a new parameter.

Right. Ian J said he was going to do a sweep for changes at some point
before the release.

Ian.

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

[George Dunlap]

On 02/05/2014 11:36 AM, Olaf Hering wrote:
> On Wed, Feb 05, George Dunlap wrote:
>
>> Well it looks like in order to keep ABI compatibility (which I don't think
>> we ever promised), you're introducing this weird hack with overloading a
>> putative boolean value with a magic number?
> Yes, thats the point. If libxl_device_disk changes then IMO the SONAME
> has to change as well. And is 4.4-rc4 is the right time to do that?
> Likely not. I'm fine with carry the 4.4 patch to achieve the result, and
> put the other version into 4.5.

Right, and so this exposes another risk of accepting a patch so late: 
that of making poor interface decisions in a rush which either libxl or 
programs written against it have to deal with for a long time.  (i.e., 
either we have to keep supporting the old interface, or the application 
developer has to special case the 4.4 interface if they want to be able 
to compile against it).

  -George

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

[Ian Jackson]

Olaf Hering writes ("Re: [Xen-devel] [PATCH for xen-4.4] libxl: add option for discard support to xl disk configuration"):
> On Wed, Feb 05, George Dunlap wrote:
> > Well it looks like in order to keep ABI compatibility (which I don't think
> > we ever promised), you're introducing this weird hack with overloading a
> > putative boolean value with a magic number?
> 
> Yes, thats the point. If libxl_device_disk changes then IMO the SONAME
> has to change as well. And is 4.4-rc4 is the right time to do that?
> Likely not. I'm fine with carry the 4.4 patch to achieve the result, and
> put the other version into 4.5.

I think there is no reason not to change the ABI now.  RCs are
provided for testing purposes, not production use.

The real question is whether the actual functional patch ought to be
accepted.

Ian.