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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
> 

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