Re: [Qemu-devel] [PATCH] xen_disk: add discard support

[Kevin Wolf]

Am 30.01.2014 um 16:02 hat Olaf Hering geschrieben:
> 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 <address@hidden>
> ---
> 
> The counter part for libxl is here, will go into xen-4.5:
> http://lists.xenproject.org/archives/html/xen-devel/2014-01/msg02632.html
> 
> 
> checkpatch comaplains about tabs in hw/block/xen_blkif.h.
> The whole file is full of tabs, like the whole source tree...
> 
> If desired I will send a follow up patch to wipe tabs in hw/block/xen_blkif.h
> 
> 
>  hw/block/xen_blkif.h | 12 ++++++++++++
>  hw/block/xen_disk.c  | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 46 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 098f6c6..e74efc7 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,17 @@ 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);

Neither SCSI nor IDE account for discards. I think we should keep the
behaviour consistent across devices.

If we do want to introduce accounting for discards, I'm not sure whether
counting them as writes or giving them their own category makes more
sense.

> +        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;

Kevin