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

[PATCH] [RFC] 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. But it would be worth to
have a knob to disable it in case the backing file was intentionally
created non-sparse to avoid fragmentation.
How could this be knob be passed from domU.cfg:disk=[] to the actual
qemu process?

blkfront_setup_discard should check for "qdisk" instead of (, or in
addition to?) "file" to actually make use of this new feature.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 hw/block/xen_blkif.h | 12 ++++++++++++
 hw/block/xen_disk.c  | 16 ++++++++++++++++
 2 files changed, 28 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..555c2d6 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -68,6 +68,8 @@ struct ioreq {
     int                 presync;
     int                 postsync;
     uint8_t             mapped;
+    int64_t             sector_num;
+    int                 nb_sectors;
 
     /* grant mapping */
     uint32_t            domids[BLKIF_MAX_SEGMENTS_PER_REQUEST];
@@ -232,6 +234,7 @@ static void ioreq_release(struct ioreq *ioreq, bool finish)
 static int ioreq_parse(struct ioreq *ioreq)
 {
     struct XenBlkDev *blkdev = ioreq->blkdev;
+    struct blkif_request_discard *discard_req = (void *)&ioreq->req;
     uintptr_t mem;
     size_t len;
     int i;
@@ -244,6 +247,10 @@ static int ioreq_parse(struct ioreq *ioreq)
     case BLKIF_OP_READ:
         ioreq->prot = PROT_WRITE; /* to memory */
         break;
+    case BLKIF_OP_DISCARD:
+        ioreq->sector_num = discard_req->sector_number;
+        ioreq->nb_sectors = discard_req->nr_sectors;
+        return 0;
     case BLKIF_OP_FLUSH_DISKCACHE:
         ioreq->presync = 1;
         if (!ioreq->req.nr_segments) {
@@ -521,6 +528,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, ioreq->nb_sectors * BLOCK_SIZE, BDRV_ACCT_WRITE);
+        ioreq->aio_inflight++;
+        bdrv_aio_discard(blkdev->bs,
+                        ioreq->sector_num, ioreq->nb_sectors,
+                        qemu_aio_complete, ioreq);
+        break;
     default:
         /* unknown operation (shouldn't happen -- parse catches this) */
         goto err;
@@ -764,6 +778,7 @@ 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, "feature-discard", 1);
     xenstore_write_be_int(&blkdev->xendev, "info", info);
 
     g_free(directiosafe);
@@ -801,6 +816,7 @@ static int blk_connect(struct XenDevice *xendev)
         qflags |= BDRV_O_RDWR;
         readonly = false;
     }
+    qflags |= BDRV_O_UNMAP;
 
     /* init qemu block driver */
     index = (blkdev->xendev.dev - 202 * 256) / 16;

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

[Ian Campbell]

On Thu, 2014-01-09 at 23:01 +0100, Olaf Hering wrote:
> Implement discard support for xen_disk. It makes use of the existing
> discard code in qemu.

Please always CC the maintainers on patches. In this case that's
Stefano.

Stefano, I wonder if the MAINTAINERS entry for upstream qemu ought to
list Anthony and/or the upstream qemu list as well?

Ian.

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

[Stefano Stabellini]

On Thu, 9 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. But it would be worth to
> have a knob to disable it in case the backing file was intentionally
> created non-sparse to avoid fragmentation.
> How could this be knob be passed from domU.cfg:disk=[] to the actual
> qemu process?

It would need to be on xenstore, because that is the only per-disk
interface xen_disk is listening to.


> blkfront_setup_discard should check for "qdisk" instead of (, or in
> addition to?) "file" to actually make use of this new feature.

Why? I don't think that would be correct: if the feature is advertised
on xenstore by the backend (feature-discard) then blkfront can/should
use it. If it is not present then it is not going to use it.
Let's not complicate things further.


> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  hw/block/xen_blkif.h | 12 ++++++++++++
>  hw/block/xen_disk.c  | 16 ++++++++++++++++
>  2 files changed, 28 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..555c2d6 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -68,6 +68,8 @@ struct ioreq {
>      int                 presync;
>      int                 postsync;
>      uint8_t             mapped;
> +    int64_t             sector_num;
> +    int                 nb_sectors;

You have access to the original request via req, I don't think you need
these two fields, do you?


>      /* grant mapping */
>      uint32_t            domids[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> @@ -232,6 +234,7 @@ static void ioreq_release(struct ioreq *ioreq, bool finish)
>  static int ioreq_parse(struct ioreq *ioreq)
>  {
>      struct XenBlkDev *blkdev = ioreq->blkdev;
> +    struct blkif_request_discard *discard_req = (void *)&ioreq->req;
>      uintptr_t mem;
>      size_t len;
>      int i;
> @@ -244,6 +247,10 @@ static int ioreq_parse(struct ioreq *ioreq)
>      case BLKIF_OP_READ:
>          ioreq->prot = PROT_WRITE; /* to memory */
>          break;
> +    case BLKIF_OP_DISCARD:
> +        ioreq->sector_num = discard_req->sector_number;
> +        ioreq->nb_sectors = discard_req->nr_sectors;
> +        return 0;
>      case BLKIF_OP_FLUSH_DISKCACHE:
>          ioreq->presync = 1;
>          if (!ioreq->req.nr_segments) {
> @@ -521,6 +528,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, ioreq->nb_sectors * BLOCK_SIZE, BDRV_ACCT_WRITE);
> +        ioreq->aio_inflight++;
> +        bdrv_aio_discard(blkdev->bs,
> +                        ioreq->sector_num, ioreq->nb_sectors,
> +                        qemu_aio_complete, ioreq);
> +        break;
>      default:
>          /* unknown operation (shouldn't happen -- parse catches this) */
>          goto err;
> @@ -764,6 +778,7 @@ 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, "feature-discard", 1);
>      xenstore_write_be_int(&blkdev->xendev, "info", info);
>  
>      g_free(directiosafe);
> @@ -801,6 +816,7 @@ static int blk_connect(struct XenDevice *xendev)
>          qflags |= BDRV_O_RDWR;
>          readonly = false;
>      }
> +    qflags |= BDRV_O_UNMAP;
>  
>      /* init qemu block driver */
>      index = (blkdev->xendev.dev - 202 * 256) / 16;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

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

[Olaf Hering]

On Fri, Jan 17, Stefano Stabellini wrote:

> On Thu, 9 Jan 2014, Olaf Hering wrote:
> > The discard support is enabled unconditionally. But it would be worth to
> > have a knob to disable it in case the backing file was intentionally
> > created non-sparse to avoid fragmentation.
> > How could this be knob be passed from domU.cfg:disk=[] to the actual
> > qemu process?
> 
> It would need to be on xenstore, because that is the only per-disk
> interface xen_disk is listening to.

I figured that out. There are already script=, backend= and other knobs.
I will see how to add a discard=on|off to libxl and write that to the
xenstore backend node so qemu can get it from there.
What property name do you suggest? I have something like
"toolstack-option-discard" in mind.

> > blkfront_setup_discard should check for "qdisk" instead of (, or in
> > addition to?) "file" to actually make use of this new feature.
> 
> Why? I don't think that would be correct: if the feature is advertised
> on xenstore by the backend (feature-discard) then blkfront can/should
> use it. If it is not present then it is not going to use it.
> Let's not complicate things further.

blockfront is broken:
http://lists.xenproject.org/archives/html/xen-devel/2014-01/msg00988.html


> > +++ b/hw/block/xen_disk.c
> > @@ -68,6 +68,8 @@ struct ioreq {
> >      int                 presync;
> >      int                 postsync;
> >      uint8_t             mapped;
> > +    int64_t             sector_num;
> > +    int                 nb_sectors;
> 
> You have access to the original request via req, I don't think you need
> these two fields, do you?

I will double check if thats doable.

Thanks,

Olaf

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

[Stefano Stabellini]

On Fri, 17 Jan 2014, Olaf Hering wrote:
> On Fri, Jan 17, Stefano Stabellini wrote:
> 
> > On Thu, 9 Jan 2014, Olaf Hering wrote:
> > > The discard support is enabled unconditionally. But it would be worth to
> > > have a knob to disable it in case the backing file was intentionally
> > > created non-sparse to avoid fragmentation.
> > > How could this be knob be passed from domU.cfg:disk=[] to the actual
> > > qemu process?
> > 
> > It would need to be on xenstore, because that is the only per-disk
> > interface xen_disk is listening to.
> 
> I figured that out. There are already script=, backend= and other knobs.
> I will see how to add a discard=on|off to libxl and write that to the
> xenstore backend node so qemu can get it from there.
> What property name do you suggest? I have something like
> "toolstack-option-discard" in mind.

discard_enabled?


> > > blkfront_setup_discard should check for "qdisk" instead of (, or in
> > > addition to?) "file" to actually make use of this new feature.
> > 
> > Why? I don't think that would be correct: if the feature is advertised
> > on xenstore by the backend (feature-discard) then blkfront can/should
> > use it. If it is not present then it is not going to use it.
> > Let's not complicate things further.
> 
> blockfront is broken:
> http://lists.xenproject.org/archives/html/xen-devel/2014-01/msg00988.html

blkfront should be fixed then :-)

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

[Olaf Hering]

On Fri, Jan 17, Stefano Stabellini wrote:

> On Fri, 17 Jan 2014, Olaf Hering wrote:
> > On Fri, Jan 17, Stefano Stabellini wrote:
> > 
> > > On Thu, 9 Jan 2014, Olaf Hering wrote:
> > > > The discard support is enabled unconditionally. But it would be worth to
> > > > have a knob to disable it in case the backing file was intentionally
> > > > created non-sparse to avoid fragmentation.
> > > > How could this be knob be passed from domU.cfg:disk=[] to the actual
> > > > qemu process?
> > > 
> > > It would need to be on xenstore, because that is the only per-disk
> > > interface xen_disk is listening to.
> > 
> > I figured that out. There are already script=, backend= and other knobs.
> > I will see how to add a discard=on|off to libxl and write that to the
> > xenstore backend node so qemu can get it from there.
> > What property name do you suggest? I have something like
> > "toolstack-option-discard" in mind.
> 
> discard_enabled?

Isnt that name too generic? In the end that node is used also by backend
and frontend.

Olaf

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

[Stefano Stabellini]

On Fri, 17 Jan 2014, Olaf Hering wrote:
> On Fri, Jan 17, Stefano Stabellini wrote:
> 
> > On Fri, 17 Jan 2014, Olaf Hering wrote:
> > > On Fri, Jan 17, Stefano Stabellini wrote:
> > > 
> > > > On Thu, 9 Jan 2014, Olaf Hering wrote:
> > > > > The discard support is enabled unconditionally. But it would be worth to
> > > > > have a knob to disable it in case the backing file was intentionally
> > > > > created non-sparse to avoid fragmentation.
> > > > > How could this be knob be passed from domU.cfg:disk=[] to the actual
> > > > > qemu process?
> > > > 
> > > > It would need to be on xenstore, because that is the only per-disk
> > > > interface xen_disk is listening to.
> > > 
> > > I figured that out. There are already script=, backend= and other knobs.
> > > I will see how to add a discard=on|off to libxl and write that to the
> > > xenstore backend node so qemu can get it from there.
> > > What property name do you suggest? I have something like
> > > "toolstack-option-discard" in mind.
> > 
> > discard_enabled?
> 
> Isnt that name too generic? In the end that node is used also by backend
> and frontend.

The problem is that it is confusing to have two options in the same
place, one written by the toolstack for the backend and the other
written by the backend for the frontend.

Can't we just assume that if the backend can do discard on that file, it
is simply going to enable feature-discard? Do we really need the
toolstack driven option too?

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

[Ian Campbell]

On Fri, 2014-01-17 at 16:38 +0100, Olaf Hering wrote:
> On Fri, Jan 17, Stefano Stabellini wrote:
> 
> > On Fri, 17 Jan 2014, Olaf Hering wrote:
> > > On Fri, Jan 17, Stefano Stabellini wrote:
> > > 
> > > > On Thu, 9 Jan 2014, Olaf Hering wrote:
> > > > > The discard support is enabled unconditionally. But it would be worth to
> > > > > have a knob to disable it in case the backing file was intentionally
> > > > > created non-sparse to avoid fragmentation.
> > > > > How could this be knob be passed from domU.cfg:disk=[] to the actual
> > > > > qemu process?
> > > > 
> > > > It would need to be on xenstore, because that is the only per-disk
> > > > interface xen_disk is listening to.
> > > 
> > > I figured that out. There are already script=, backend= and other knobs.
> > > I will see how to add a discard=on|off to libxl and write that to the
> > > xenstore backend node so qemu can get it from there.
> > > What property name do you suggest? I have something like
> > > "toolstack-option-discard" in mind.
> > 
> > discard_enabled?
> 
> Isnt that name too generic? In the end that node is used also by backend
> and frontend.

Surely this node is for toolstack to qdisk communication. It is then up
to itself qdisk to decide whether to expose this feature to the
frontend, using the existing defined feature flag for that purpose.

Ian.

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

[Olaf Hering]

On Fri, Jan 17, Stefano Stabellini wrote:

> Can't we just assume that if the backend can do discard on that file, it
> is simply going to enable feature-discard? Do we really need the
> toolstack driven option too?

If the backing file is fully ppopulated on purpose to avoid
fragmentation of that file, then silently enabling discard means the
unwanted fragmentation will occour over time. If anyone is really doing
that in their setup I certainly dont know. At least kvm/qemu has such
knob, and also an hyperv host offers sparse, fully populated and overlay
as possible option when creating a new virtual disk image.

Now I think that if libxl already writes feature-discard=0, qemu can use
that to clear BDRV_O_UNMAP. Also blkbk could be extended to first check
if the property already exists before blindly enabling discard. No idea
if that makes sense for "phy", but you never know.

Olaf

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

[Stefano Stabellini]

On Fri, 17 Jan 2014, Olaf Hering wrote:
> On Fri, Jan 17, Stefano Stabellini wrote:
> 
> > Can't we just assume that if the backend can do discard on that file, it
> > is simply going to enable feature-discard? Do we really need the
> > toolstack driven option too?
> 
> If the backing file is fully ppopulated on purpose to avoid
> fragmentation of that file, then silently enabling discard means the
> unwanted fragmentation will occour over time. If anyone is really doing
> that in their setup I certainly dont know. At least kvm/qemu has such
> knob, and also an hyperv host offers sparse, fully populated and overlay
> as possible option when creating a new virtual disk image.
> 
> Now I think that if libxl already writes feature-discard=0, qemu can use
> that to clear BDRV_O_UNMAP. Also blkbk could be extended to first check
> if the property already exists before blindly enabling discard. No idea
> if that makes sense for "phy", but you never know.

OK, but it is important that we don't collapse the two options into one:
the backend might or might not support feature-discard so we can't have
the toolstack write feature-discard directly.

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

[Olaf Hering]

On Fri, Jan 17, Stefano Stabellini wrote:

> OK, but it is important that we don't collapse the two options into one:
> the backend might or might not support feature-discard so we can't have
> the toolstack write feature-discard directly.

Thats true. So I will use "discard_enabled" in the libxl change.

Olaf