[Qemu-devel] [PATCH v3 00/19] block & scsi: write_zeroes support through the whole stack

[Qemu-devel] [PATCH v3 00/19] block & scsi: write_zeroes support through the whole stack

[Paolo Bonzini]

This series fixes the implementation of WRITE SAME in the SCSI emulation
layer.  On top of this, it ensures that zero writes from the guest can
be offloaded to the host device or filesystem if that's supported.
This is a relatively important part of the thin provisioning feature,
and builds on the unmap flag to write_zeroes, recently added by Peter
Lieven.  The feature works with iscsi, block device and filesystem
targets.

v2->v3
        Drop "block/iscsi: use UNMAP to write zeroes if LBPRZ=1" [Peter]

        Patches 3/4: change tracepoint flags to %#x [Stefan]

        Patch 6: clean up conditionals for misaligned num [Stefan]
        Patch 6: fix freeing of bounce buffer [Peter]

        Patch 7: fix bdi->cluster_size for vhdx [Stefan]

        Patch 8: do not check bs->backing_hd to set
        bdi->unallocated_blocks_are_zero [Peter]

        Patch 19: use qemu_blockalign/qemu_vfree [Stefan]

v1->v2
        I cleaned up the series, putting all the block layer changes
        together and all the SCSI changes at the end.

        qcow2 and iscsi are now better covered, which caused the
        introduction of patches 6-11.  I also downloaded the latest
        which contributed some changes in patches 10-11.  It turns
        out we were too cautious in treating LBPRZ as advisory when
        using the UNMAP command.  On the other hand, the API we
        introduced is useful for qcow2/qed/vmdk WRITE SAME, so no
        damage done.


Paolo Bonzini (17):
  block: generalize BlockLimits handling to cover bdrv_aio_discard too
  block: add flags to BlockRequest
  block: add flags argument to bdrv_co_write_zeroes tracepoint
  block: add bdrv_aio_write_zeroes
  block: handle ENOTSUP from discard in generic code
  block: make bdrv_co_do_write_zeroes stricter in producing aligned
    requests
  vpc, vhdx: add get_info
  block drivers: add discard/write_zeroes properties to bdrv_get_info
    implementation
  block drivers: expose requirement for write same alignment from
    formats
  block/iscsi: check WRITE SAME support differently depending on
    MAY_UNMAP
  raw-posix: implement write_zeroes with MAY_UNMAP for files
  raw-posix: implement write_zeroes with MAY_UNMAP for block devices
  raw-posix: add support for write_zeroes on XFS and block devices
  qemu-iotests: 033 is fast
  scsi-disk: catch write protection errors in UNMAP
  scsi-disk: reject ANCHOR=1 for UNMAP and WRITE SAME commands
  scsi-disk: correctly implement WRITE SAME

Peter Lieven (2):
  block/iscsi: remove .bdrv_has_zero_init
  block/iscsi: updated copyright

 block.c                  | 145 +++++++++++++++++++++++----------------
 block/iscsi.c            |  27 +++++---
 block/qcow2.c            |   3 +
 block/qed.c              |   3 +
 block/raw-aio.h          |   3 +-
 block/raw-posix.c        | 175 ++++++++++++++++++++++++++++++++++++++++++++---
 block/vdi.c              |   1 +
 block/vhdx.c             |  13 ++++
 block/vmdk.c             |   4 ++
 block/vpc.c              |  15 ++++
 hw/scsi/scsi-disk.c      | 154 ++++++++++++++++++++++++++++++++++-------
 include/block/block.h    |   4 ++
 tests/qemu-iotests/group |   2 +-
 trace-events             |   4 +-
 14 files changed, 452 insertions(+), 101 deletions(-)

-- 
1.8.4.2

[Qemu-devel] [PATCH v3 01/19] block: generalize BlockLimits handling to cover bdrv_aio_discard too

[Paolo Bonzini]

bdrv_co_discard is only covering drivers which have a .bdrv_co_discard()
implementation, but not those with .bdrv_aio_discard(). Not very nice,
and easy to avoid.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 80 +++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 41 insertions(+), 39 deletions(-)

diff --git a/block.c b/block.c
index e22b55f..1b3e8b2 100644
--- a/block.c
+++ b/block.c
@@ -4288,6 +4288,8 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
 int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
                                  int nb_sectors)
 {
+    int max_discard;
+
     if (!bs->drv) {
         return -ENOMEDIUM;
     } else if (bdrv_check_request(bs, sector_num, nb_sectors)) {
@@ -4305,55 +4307,55 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
         return 0;
     }
 
-    if (bs->drv->bdrv_co_discard) {
-        int max_discard = bs->bl.max_discard ?
-                          bs->bl.max_discard : MAX_DISCARD_DEFAULT;
+    if (!bs->drv->bdrv_co_discard && !bs->drv->bdrv_aio_discard) {
+        return 0;
+    }
 
-        while (nb_sectors > 0) {
-            int ret;
-            int num = nb_sectors;
+    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : MAX_DISCARD_DEFAULT;
+    while (nb_sectors > 0) {
+        int ret;
+        int num = nb_sectors;
 
-            /* align request */
-            if (bs->bl.discard_alignment &&
-                num >= bs->bl.discard_alignment &&
-                sector_num % bs->bl.discard_alignment) {
-                if (num > bs->bl.discard_alignment) {
-                    num = bs->bl.discard_alignment;
-                }
-                num -= sector_num % bs->bl.discard_alignment;
+        /* align request */
+        if (bs->bl.discard_alignment &&
+            num >= bs->bl.discard_alignment &&
+            sector_num % bs->bl.discard_alignment) {
+            if (num > bs->bl.discard_alignment) {
+                num = bs->bl.discard_alignment;
             }
+            num -= sector_num % bs->bl.discard_alignment;
+        }
 
-            /* limit request size */
-            if (num > max_discard) {
-                num = max_discard;
-            }
+        /* limit request size */
+        if (num > max_discard) {
+            num = max_discard;
+        }
 
+        if (bs->drv->bdrv_co_discard) {
             ret = bs->drv->bdrv_co_discard(bs, sector_num, num);
-            if (ret) {
-                return ret;
+        } else {
+            BlockDriverAIOCB *acb;
+            CoroutineIOCompletion co = {
+                .coroutine = qemu_coroutine_self(),
+            };
+
+            acb = bs->drv->bdrv_aio_discard(bs, sector_num, nb_sectors,
+                                            bdrv_co_io_em_complete, &co);
+            if (acb == NULL) {
+                return -EIO;
+            } else {
+                qemu_coroutine_yield();
+                ret = co.ret;
             }
-
-            sector_num += num;
-            nb_sectors -= num;
         }
-        return 0;
-    } else if (bs->drv->bdrv_aio_discard) {
-        BlockDriverAIOCB *acb;
-        CoroutineIOCompletion co = {
-            .coroutine = qemu_coroutine_self(),
-        };
-
-        acb = bs->drv->bdrv_aio_discard(bs, sector_num, nb_sectors,
-                                        bdrv_co_io_em_complete, &co);
-        if (acb == NULL) {
-            return -EIO;
-        } else {
-            qemu_coroutine_yield();
-            return co.ret;
+        if (ret) {
+            return ret;
         }
-    } else {
-        return 0;
+
+        sector_num += num;
+        nb_sectors -= num;
     }
+    return 0;
 }
 
 int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 01/19] block: generalize BlockLimits handling to cover bdrv_aio_discard too

[Peter Lieven]

On 22.11.2013 13:39, Paolo Bonzini wrote:
> bdrv_co_discard is only covering drivers which have a .bdrv_co_discard()
> implementation, but not those with .bdrv_aio_discard(). Not very nice,
> and easy to avoid.
>
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block.c | 80 +++++++++++++++++++++++++++++++++--------------------------------
>   1 file changed, 41 insertions(+), 39 deletions(-)
>
> diff --git a/block.c b/block.c
> index e22b55f..1b3e8b2 100644
> --- a/block.c
> +++ b/block.c
> @@ -4288,6 +4288,8 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
>   int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>                                    int nb_sectors)
>   {
> +    int max_discard;
> +
>       if (!bs->drv) {
>           return -ENOMEDIUM;
>       } else if (bdrv_check_request(bs, sector_num, nb_sectors)) {
> @@ -4305,55 +4307,55 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>           return 0;
>       }
>   
> -    if (bs->drv->bdrv_co_discard) {
> -        int max_discard = bs->bl.max_discard ?
> -                          bs->bl.max_discard : MAX_DISCARD_DEFAULT;
> +    if (!bs->drv->bdrv_co_discard && !bs->drv->bdrv_aio_discard) {
> +        return 0;
> +    }
>   
> -        while (nb_sectors > 0) {
> -            int ret;
> -            int num = nb_sectors;
> +    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : MAX_DISCARD_DEFAULT;
> +    while (nb_sectors > 0) {
> +        int ret;
> +        int num = nb_sectors;
>   
> -            /* align request */
> -            if (bs->bl.discard_alignment &&
> -                num >= bs->bl.discard_alignment &&
> -                sector_num % bs->bl.discard_alignment) {
> -                if (num > bs->bl.discard_alignment) {
> -                    num = bs->bl.discard_alignment;
> -                }
> -                num -= sector_num % bs->bl.discard_alignment;
> +        /* align request */
> +        if (bs->bl.discard_alignment &&
> +            num >= bs->bl.discard_alignment &&
> +            sector_num % bs->bl.discard_alignment) {
> +            if (num > bs->bl.discard_alignment) {
> +                num = bs->bl.discard_alignment;
>               }
> +            num -= sector_num % bs->bl.discard_alignment;
> +        }
>   
> -            /* limit request size */
> -            if (num > max_discard) {
> -                num = max_discard;
> -            }
> +        /* limit request size */
> +        if (num > max_discard) {
> +            num = max_discard;
> +        }
>   
> +        if (bs->drv->bdrv_co_discard) {
>               ret = bs->drv->bdrv_co_discard(bs, sector_num, num);
> -            if (ret) {
> -                return ret;
> +        } else {
> +            BlockDriverAIOCB *acb;
> +            CoroutineIOCompletion co = {
> +                .coroutine = qemu_coroutine_self(),
> +            };
> +
> +            acb = bs->drv->bdrv_aio_discard(bs, sector_num, nb_sectors,
> +                                            bdrv_co_io_em_complete, &co);
> +            if (acb == NULL) {
> +                return -EIO;
> +            } else {
> +                qemu_coroutine_yield();
> +                ret = co.ret;
>               }
> -
> -            sector_num += num;
> -            nb_sectors -= num;
>           }
> -        return 0;
> -    } else if (bs->drv->bdrv_aio_discard) {
> -        BlockDriverAIOCB *acb;
> -        CoroutineIOCompletion co = {
> -            .coroutine = qemu_coroutine_self(),
> -        };
> -
> -        acb = bs->drv->bdrv_aio_discard(bs, sector_num, nb_sectors,
> -                                        bdrv_co_io_em_complete, &co);
> -        if (acb == NULL) {
> -            return -EIO;
> -        } else {
> -            qemu_coroutine_yield();
> -            return co.ret;
> +        if (ret) {
> +            return ret;
>           }
> -    } else {
> -        return 0;
> +
> +        sector_num += num;
> +        nb_sectors -= num;
>       }
> +    return 0;
>   }
>   
>   int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
Reviewed-by: Peter Lieven <pl@kamp.de>

[Qemu-devel] [PATCH v3 02/19] block: add flags to BlockRequest

[Paolo Bonzini]

This lets bdrv_co_do_rw receive flags, so that it can be used for
zero writes.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c               | 17 +++++++++++------
 include/block/block.h |  1 +
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 1b3e8b2..0665f35 100644
--- a/block.c
+++ b/block.c
@@ -74,6 +74,7 @@ static BlockDriverAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
                                                int64_t sector_num,
                                                QEMUIOVector *qiov,
                                                int nb_sectors,
+                                               BdrvRequestFlags flags,
                                                BlockDriverCompletionFunc *cb,
                                                void *opaque,
                                                bool is_write);
@@ -3655,7 +3656,7 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
 {
     trace_bdrv_aio_readv(bs, sector_num, nb_sectors, opaque);
 
-    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors,
+    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors, 0,
                                  cb, opaque, false);
 }
 
@@ -3665,7 +3666,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
 {
     trace_bdrv_aio_writev(bs, sector_num, nb_sectors, opaque);
 
-    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors,
+    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors, 0,
                                  cb, opaque, true);
 }
 
@@ -3837,8 +3838,10 @@ int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs)
     /* Run the aio requests. */
     mcb->num_requests = num_reqs;
     for (i = 0; i < num_reqs; i++) {
-        bdrv_aio_writev(bs, reqs[i].sector, reqs[i].qiov,
-            reqs[i].nb_sectors, multiwrite_cb, mcb);
+        bdrv_co_aio_rw_vector(bs, reqs[i].sector, reqs[i].qiov,
+                              reqs[i].nb_sectors, reqs[i].flags,
+                              multiwrite_cb, mcb,
+                              true);
     }
 
     return 0;
@@ -3980,10 +3983,10 @@ static void coroutine_fn bdrv_co_do_rw(void *opaque)
 
     if (!acb->is_write) {
         acb->req.error = bdrv_co_do_readv(bs, acb->req.sector,
-            acb->req.nb_sectors, acb->req.qiov, 0);
+            acb->req.nb_sectors, acb->req.qiov, acb->req.flags);
     } else {
         acb->req.error = bdrv_co_do_writev(bs, acb->req.sector,
-            acb->req.nb_sectors, acb->req.qiov, 0);
+            acb->req.nb_sectors, acb->req.qiov, acb->req.flags);
     }
 
     acb->bh = qemu_bh_new(bdrv_co_em_bh, acb);
@@ -3994,6 +3997,7 @@ static BlockDriverAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
                                                int64_t sector_num,
                                                QEMUIOVector *qiov,
                                                int nb_sectors,
+                                               BdrvRequestFlags flags,
                                                BlockDriverCompletionFunc *cb,
                                                void *opaque,
                                                bool is_write)
@@ -4005,6 +4009,7 @@ static BlockDriverAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
     acb->req.sector = sector_num;
     acb->req.nb_sectors = nb_sectors;
     acb->req.qiov = qiov;
+    acb->req.flags = flags;
     acb->is_write = is_write;
     acb->done = NULL;
 
diff --git a/include/block/block.h b/include/block/block.h
index 4d9e67c..703d875 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -311,6 +311,7 @@ typedef struct BlockRequest {
     /* Fields to be filled by multiwrite caller */
     int64_t sector;
     int nb_sectors;
+    int flags;
     QEMUIOVector *qiov;
     BlockDriverCompletionFunc *cb;
     void *opaque;
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 02/19] block: add flags to BlockRequest

[Peter Lieven]

On 22.11.2013 13:39, Paolo Bonzini wrote:
> This lets bdrv_co_do_rw receive flags, so that it can be used for
> zero writes.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block.c               | 17 +++++++++++------
>   include/block/block.h |  1 +
>   2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/block.c b/block.c
> index 1b3e8b2..0665f35 100644
> --- a/block.c
> +++ b/block.c
> @@ -74,6 +74,7 @@ static BlockDriverAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
>                                                  int64_t sector_num,
>                                                  QEMUIOVector *qiov,
>                                                  int nb_sectors,
> +                                               BdrvRequestFlags flags,
>                                                  BlockDriverCompletionFunc *cb,
>                                                  void *opaque,
>                                                  bool is_write);
> @@ -3655,7 +3656,7 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
>   {
>       trace_bdrv_aio_readv(bs, sector_num, nb_sectors, opaque);
>   
> -    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors,
> +    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors, 0,
>                                    cb, opaque, false);
>   }
>   
> @@ -3665,7 +3666,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>   {
>       trace_bdrv_aio_writev(bs, sector_num, nb_sectors, opaque);
>   
> -    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors,
> +    return bdrv_co_aio_rw_vector(bs, sector_num, qiov, nb_sectors, 0,
>                                    cb, opaque, true);
>   }
>   
> @@ -3837,8 +3838,10 @@ int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs)
>       /* Run the aio requests. */
>       mcb->num_requests = num_reqs;
>       for (i = 0; i < num_reqs; i++) {
> -        bdrv_aio_writev(bs, reqs[i].sector, reqs[i].qiov,
> -            reqs[i].nb_sectors, multiwrite_cb, mcb);
> +        bdrv_co_aio_rw_vector(bs, reqs[i].sector, reqs[i].qiov,
> +                              reqs[i].nb_sectors, reqs[i].flags,
> +                              multiwrite_cb, mcb,
> +                              true);
>       }
>   
>       return 0;
> @@ -3980,10 +3983,10 @@ static void coroutine_fn bdrv_co_do_rw(void *opaque)
>   
>       if (!acb->is_write) {
>           acb->req.error = bdrv_co_do_readv(bs, acb->req.sector,
> -            acb->req.nb_sectors, acb->req.qiov, 0);
> +            acb->req.nb_sectors, acb->req.qiov, acb->req.flags);
>       } else {
>           acb->req.error = bdrv_co_do_writev(bs, acb->req.sector,
> -            acb->req.nb_sectors, acb->req.qiov, 0);
> +            acb->req.nb_sectors, acb->req.qiov, acb->req.flags);
>       }
>   
>       acb->bh = qemu_bh_new(bdrv_co_em_bh, acb);
> @@ -3994,6 +3997,7 @@ static BlockDriverAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
>                                                  int64_t sector_num,
>                                                  QEMUIOVector *qiov,
>                                                  int nb_sectors,
> +                                               BdrvRequestFlags flags,
>                                                  BlockDriverCompletionFunc *cb,
>                                                  void *opaque,
>                                                  bool is_write)
> @@ -4005,6 +4009,7 @@ static BlockDriverAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
>       acb->req.sector = sector_num;
>       acb->req.nb_sectors = nb_sectors;
>       acb->req.qiov = qiov;
> +    acb->req.flags = flags;
>       acb->is_write = is_write;
>       acb->done = NULL;
>   
> diff --git a/include/block/block.h b/include/block/block.h
> index 4d9e67c..703d875 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -311,6 +311,7 @@ typedef struct BlockRequest {
>       /* Fields to be filled by multiwrite caller */
>       int64_t sector;
>       int nb_sectors;
> +    int flags;
>       QEMUIOVector *qiov;
>       BlockDriverCompletionFunc *cb;
>       void *opaque;
Reviewed-by: Peter Lieven <pl@kamp.de>

[Qemu-devel] [PATCH v3 03/19] block: add flags argument to bdrv_co_write_zeroes tracepoint

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c      | 2 +-
 trace-events | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 0665f35..cb07e57 100644
--- a/block.c
+++ b/block.c
@@ -2887,7 +2887,7 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
                                       int64_t sector_num, int nb_sectors,
                                       BdrvRequestFlags flags)
 {
-    trace_bdrv_co_write_zeroes(bs, sector_num, nb_sectors);
+    trace_bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
 
     if (!(bs->open_flags & BDRV_O_UNMAP)) {
         flags &= ~BDRV_REQ_MAY_UNMAP;
diff --git a/trace-events b/trace-events
index 8695e9e..f159ac9 100644
--- a/trace-events
+++ b/trace-events
@@ -64,7 +64,7 @@ bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
 bdrv_co_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
 bdrv_co_copy_on_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
 bdrv_co_writev(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
-bdrv_co_write_zeroes(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
+bdrv_co_write_zeroes(void *bs, int64_t sector_num, int nb_sector, int flags) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x"
 bdrv_co_io_em(void *bs, int64_t sector_num, int nb_sectors, int is_write, void *acb) "bs %p sector_num %"PRId64" nb_sectors %d is_write %d acb %p"
 bdrv_co_do_copy_on_readv(void *bs, int64_t sector_num, int nb_sectors, int64_t cluster_sector_num, int cluster_nb_sectors) "bs %p sector_num %"PRId64" nb_sectors %d cluster_sector_num %"PRId64" cluster_nb_sectors %d"
 
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 03/19] block: add flags argument to bdrv_co_write_zeroes tracepoint

[Peter Lieven]

On 22.11.2013 13:39, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block.c      | 2 +-
>   trace-events | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index 0665f35..cb07e57 100644
> --- a/block.c
> +++ b/block.c
> @@ -2887,7 +2887,7 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
>                                         int64_t sector_num, int nb_sectors,
>                                         BdrvRequestFlags flags)
>   {
> -    trace_bdrv_co_write_zeroes(bs, sector_num, nb_sectors);
> +    trace_bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
>   
>       if (!(bs->open_flags & BDRV_O_UNMAP)) {
>           flags &= ~BDRV_REQ_MAY_UNMAP;
> diff --git a/trace-events b/trace-events
> index 8695e9e..f159ac9 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -64,7 +64,7 @@ bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
>   bdrv_co_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
>   bdrv_co_copy_on_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
>   bdrv_co_writev(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
> -bdrv_co_write_zeroes(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
> +bdrv_co_write_zeroes(void *bs, int64_t sector_num, int nb_sector, int flags) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x"
>   bdrv_co_io_em(void *bs, int64_t sector_num, int nb_sectors, int is_write, void *acb) "bs %p sector_num %"PRId64" nb_sectors %d is_write %d acb %p"
>   bdrv_co_do_copy_on_readv(void *bs, int64_t sector_num, int nb_sectors, int64_t cluster_sector_num, int cluster_nb_sectors) "bs %p sector_num %"PRId64" nb_sectors %d cluster_sector_num %"PRId64" cluster_nb_sectors %d"
>   

Reviewed-by: Peter Lieven <pl@kamp.de>

[Qemu-devel] [PATCH v3 04/19] block: add bdrv_aio_write_zeroes

[Paolo Bonzini]

This will be used by the SCSI layer.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c               | 11 +++++++++++
 include/block/block.h |  3 +++
 trace-events          |  1 +
 3 files changed, 15 insertions(+)

diff --git a/block.c b/block.c
index cb07e57..f9674d9 100644
--- a/block.c
+++ b/block.c
@@ -3670,6 +3670,17 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
                                  cb, opaque, true);
 }
 
+BlockDriverAIOCB *bdrv_aio_write_zeroes(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, BdrvRequestFlags flags,
+        BlockDriverCompletionFunc *cb, void *opaque)
+{
+    trace_bdrv_aio_write_zeroes(bs, sector_num, nb_sectors, flags, opaque);
+
+    return bdrv_co_aio_rw_vector(bs, sector_num, NULL, nb_sectors,
+                                 BDRV_REQ_ZERO_WRITE | flags,
+                                 cb, opaque, true);
+}
+
 
 typedef struct MultiwriteCB {
     int error;
diff --git a/include/block/block.h b/include/block/block.h
index 703d875..4967ed2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -216,6 +216,9 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
                const uint8_t *buf, int nb_sectors);
 int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
                int nb_sectors, BdrvRequestFlags flags);
+BlockDriverAIOCB *bdrv_aio_write_zeroes(BlockDriverState *bs, int64_t sector_num,
+                                        int nb_sectors, BdrvRequestFlags flags,
+                                        BlockDriverCompletionFunc *cb, void *opaque);
 int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags);
 int bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov);
 int bdrv_pread(BlockDriverState *bs, int64_t offset,
diff --git a/trace-events b/trace-events
index f159ac9..d318d6f 100644
--- a/trace-events
+++ b/trace-events
@@ -60,6 +60,7 @@ bdrv_aio_discard(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs
 bdrv_aio_flush(void *bs, void *opaque) "bs %p opaque %p"
 bdrv_aio_readv(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
 bdrv_aio_writev(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
+bdrv_aio_write_zeroes(void *bs, int64_t sector_num, int nb_sectors, int flags, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x opaque %p"
 bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
 bdrv_co_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
 bdrv_co_copy_on_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 04/19] block: add bdrv_aio_write_zeroes

[Peter Lieven]

On 22.11.2013 13:39, Paolo Bonzini wrote:
> This will be used by the SCSI layer.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block.c               | 11 +++++++++++
>   include/block/block.h |  3 +++
>   trace-events          |  1 +
>   3 files changed, 15 insertions(+)
>
> diff --git a/block.c b/block.c
> index cb07e57..f9674d9 100644
> --- a/block.c
> +++ b/block.c
> @@ -3670,6 +3670,17 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>                                    cb, opaque, true);
>   }
>   
> +BlockDriverAIOCB *bdrv_aio_write_zeroes(BlockDriverState *bs,
> +        int64_t sector_num, int nb_sectors, BdrvRequestFlags flags,
> +        BlockDriverCompletionFunc *cb, void *opaque)
> +{
> +    trace_bdrv_aio_write_zeroes(bs, sector_num, nb_sectors, flags, opaque);
> +
> +    return bdrv_co_aio_rw_vector(bs, sector_num, NULL, nb_sectors,
> +                                 BDRV_REQ_ZERO_WRITE | flags,
> +                                 cb, opaque, true);
> +}
> +
>   
>   typedef struct MultiwriteCB {
>       int error;
> diff --git a/include/block/block.h b/include/block/block.h
> index 703d875..4967ed2 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -216,6 +216,9 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
>                  const uint8_t *buf, int nb_sectors);
>   int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
>                  int nb_sectors, BdrvRequestFlags flags);
> +BlockDriverAIOCB *bdrv_aio_write_zeroes(BlockDriverState *bs, int64_t sector_num,
> +                                        int nb_sectors, BdrvRequestFlags flags,
> +                                        BlockDriverCompletionFunc *cb, void *opaque);
>   int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags);
>   int bdrv_writev(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov);
>   int bdrv_pread(BlockDriverState *bs, int64_t offset,
> diff --git a/trace-events b/trace-events
> index f159ac9..d318d6f 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -60,6 +60,7 @@ bdrv_aio_discard(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs
>   bdrv_aio_flush(void *bs, void *opaque) "bs %p opaque %p"
>   bdrv_aio_readv(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
>   bdrv_aio_writev(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
> +bdrv_aio_write_zeroes(void *bs, int64_t sector_num, int nb_sectors, int flags, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x opaque %p"
>   bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
>   bdrv_co_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
>   bdrv_co_copy_on_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d"
Reviewed-by: Peter Lieven <pl@kamp.de>

[Qemu-devel] [PATCH v3 05/19] block: handle ENOTSUP from discard in generic code

[Paolo Bonzini]

Similar to write_zeroes, let the generic code receive a ENOTSUP for
discard operations.  Since bdrv_discard has advisory semantics,
we can just swallow the error.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c           |  2 +-
 block/raw-posix.c | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index f9674d9..b18ee6b 100644
--- a/block.c
+++ b/block.c
@@ -4364,7 +4364,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
                 ret = co.ret;
             }
         }
-        if (ret) {
+        if (ret && ret != -ENOTSUP) {
             return ret;
         }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index f836c8e..cfa3162 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -323,10 +323,10 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     }
 #endif
 
-    s->has_discard = 1;
+    s->has_discard = true;
 #ifdef CONFIG_XFS
     if (platform_test_xfs_fd(s->fd)) {
-        s->is_xfs = 1;
+        s->is_xfs = true;
     }
 #endif
 
@@ -698,8 +698,8 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
     int ret = -EOPNOTSUPP;
     BDRVRawState *s = aiocb->bs->opaque;
 
-    if (s->has_discard == 0) {
-        return 0;
+    if (!s->has_discard) {
+        return -ENOTSUP;
     }
 
     if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
@@ -734,8 +734,8 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
 
     if (ret == -ENODEV || ret == -ENOSYS || ret == -EOPNOTSUPP ||
         ret == -ENOTTY) {
-        s->has_discard = 0;
-        ret = 0;
+        s->has_discard = false;
+        ret = -ENOTSUP;
     }
     return ret;
 }
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 05/19] block: handle ENOTSUP from discard in generic code

[Peter Lieven]

On 22.11.2013 13:39, Paolo Bonzini wrote:
> Similar to write_zeroes, let the generic code receive a ENOTSUP for
> discard operations.  Since bdrv_discard has advisory semantics,
> we can just swallow the error.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block.c           |  2 +-
>   block/raw-posix.c | 12 ++++++------
>   2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/block.c b/block.c
> index f9674d9..b18ee6b 100644
> --- a/block.c
> +++ b/block.c
> @@ -4364,7 +4364,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>                   ret = co.ret;
>               }
>           }
> -        if (ret) {
> +        if (ret && ret != -ENOTSUP) {
>               return ret;
>           }
>   
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index f836c8e..cfa3162 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -323,10 +323,10 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>       }
>   #endif
>   
> -    s->has_discard = 1;
> +    s->has_discard = true;
>   #ifdef CONFIG_XFS
>       if (platform_test_xfs_fd(s->fd)) {
> -        s->is_xfs = 1;
> +        s->is_xfs = true;
>       }
>   #endif
>   
> @@ -698,8 +698,8 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
>       int ret = -EOPNOTSUPP;
>       BDRVRawState *s = aiocb->bs->opaque;
>   
> -    if (s->has_discard == 0) {
> -        return 0;
> +    if (!s->has_discard) {
> +        return -ENOTSUP;
>       }
>   
>       if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
> @@ -734,8 +734,8 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
>   
>       if (ret == -ENODEV || ret == -ENOSYS || ret == -EOPNOTSUPP ||
>           ret == -ENOTTY) {
> -        s->has_discard = 0;
> -        ret = 0;
> +        s->has_discard = false;
> +        ret = -ENOTSUP;
>       }
>       return ret;
>   }
wouldn't it make sense to return -ENOTSUP in other drivers a well if the
operation is not supported and not return 0?

Otherwise:
Reviewed-by: Peter Lieven <pl@kamp.de>

Re: [Qemu-devel] [PATCH v3 05/19] block: handle ENOTSUP from discard in generic code

[Paolo Bonzini]

Il 25/11/2013 11:06, Peter Lieven ha scritto:
> wouldn't it make sense to return -ENOTSUP in other drivers a well if the
> operation is not supported and not return 0?

Yes; patches are welcome. :)

(I have a patch for block/iscsi.c that handles LBPU=1/LBPWS=0, and one
of the things it does is "return 0" -> "return -ENOTSUP" for consistency
between iscsi_co_discard and iscsi_co_write_zeroes).

Paolo

[Qemu-devel] [PATCH v3 06/19] block: make bdrv_co_do_write_zeroes stricter in producing aligned requests

[Paolo Bonzini]

Right now, bdrv_co_do_write_zeroes will only try to align the
beginning of the request.  However, it is simpler for many
formats to expect the block layer to separate both the head *and*
the tail.  This makes sure that the format's bdrv_co_write_zeroes
function will be called with aligned sector_num and nb_sectors for
the bulk of the request.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index b18ee6b..de66b21 100644
--- a/block.c
+++ b/block.c
@@ -2768,14 +2768,21 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
     while (nb_sectors > 0 && !ret) {
         int num = nb_sectors;
 
-        /* align request */
-        if (bs->bl.write_zeroes_alignment &&
-            num >= bs->bl.write_zeroes_alignment &&
-            sector_num % bs->bl.write_zeroes_alignment) {
-            if (num > bs->bl.write_zeroes_alignment) {
+        /* Align request.  Block drivers can expect the "bulk" of the request
+         * to be aligned.
+         */
+        if (bs->bl.write_zeroes_alignment
+            && num > bs->bl.write_zeroes_alignment) {
+            if (sector_num % bs->bl.write_zeroes_alignment != 0) {
+                /* Make a small request up to the first aligned sector.  */
                 num = bs->bl.write_zeroes_alignment;
+                num -= sector_num % bs->bl.write_zeroes_alignment;
+            } else if ((sector_num + num) % bs->bl.write_zeroes_alignment != 0) {
+                /* Shorten the request to the last aligned sector.  num cannot
+                 * underflow because num > bs->bl.write_zeroes_alignment.
+                 */
+                num -= (sector_num + num) % bs->bl.write_zeroes_alignment;
             }
-            num -= sector_num % bs->bl.write_zeroes_alignment;
         }
 
         /* limit request size */
@@ -2793,16 +2800,20 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
             /* Fall back to bounce buffer if write zeroes is unsupported */
             iov.iov_len = num * BDRV_SECTOR_SIZE;
             if (iov.iov_base == NULL) {
-                /* allocate bounce buffer only once and ensure that it
-                 * is big enough for this and all future requests.
-                 */
-                size_t bufsize = num <= nb_sectors ? num : max_write_zeroes;
-                iov.iov_base = qemu_blockalign(bs, bufsize * BDRV_SECTOR_SIZE);
-                memset(iov.iov_base, 0, bufsize * BDRV_SECTOR_SIZE);
+                iov.iov_base = qemu_blockalign(bs, num * BDRV_SECTOR_SIZE);
+                memset(iov.iov_base, 0, num * BDRV_SECTOR_SIZE);
             }
             qemu_iovec_init_external(&qiov, &iov, 1);
 
             ret = drv->bdrv_co_writev(bs, sector_num, num, &qiov);
+
+            /* Keep bounce buffer around if it is big enough for all
+             * all future requests.
+             */
+            if (num < max_write_zeroes) {
+                qemu_vfree(iov.iov_base);
+                iov.iov_base = NULL;
+            }
         }
 
         sector_num += num;
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 06/19] block: make bdrv_co_do_write_zeroes stricter in producing aligned requests

[Peter Lieven]

On 22.11.2013 13:39, Paolo Bonzini wrote:
> Right now, bdrv_co_do_write_zeroes will only try to align the
> beginning of the request.  However, it is simpler for many
> formats to expect the block layer to separate both the head *and*
> the tail.  This makes sure that the format's bdrv_co_write_zeroes
> function will be called with aligned sector_num and nb_sectors for
> the bulk of the request.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block.c | 35 +++++++++++++++++++++++------------
>   1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/block.c b/block.c
> index b18ee6b..de66b21 100644
> --- a/block.c
> +++ b/block.c
> @@ -2768,14 +2768,21 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>       while (nb_sectors > 0 && !ret) {
>           int num = nb_sectors;
>   
> -        /* align request */
> -        if (bs->bl.write_zeroes_alignment &&
> -            num >= bs->bl.write_zeroes_alignment &&
> -            sector_num % bs->bl.write_zeroes_alignment) {
> -            if (num > bs->bl.write_zeroes_alignment) {
> +        /* Align request.  Block drivers can expect the "bulk" of the request
> +         * to be aligned.
> +         */
> +        if (bs->bl.write_zeroes_alignment
> +            && num > bs->bl.write_zeroes_alignment) {
> +            if (sector_num % bs->bl.write_zeroes_alignment != 0) {
> +                /* Make a small request up to the first aligned sector.  */
>                   num = bs->bl.write_zeroes_alignment;
> +                num -= sector_num % bs->bl.write_zeroes_alignment;
> +            } else if ((sector_num + num) % bs->bl.write_zeroes_alignment != 0) {
> +                /* Shorten the request to the last aligned sector.  num cannot
> +                 * underflow because num > bs->bl.write_zeroes_alignment.
> +                 */
> +                num -= (sector_num + num) % bs->bl.write_zeroes_alignment;
>               }
> -            num -= sector_num % bs->bl.write_zeroes_alignment;
>           }
>   
>           /* limit request size */
> @@ -2793,16 +2800,20 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>               /* Fall back to bounce buffer if write zeroes is unsupported */
>               iov.iov_len = num * BDRV_SECTOR_SIZE;
>               if (iov.iov_base == NULL) {
> -                /* allocate bounce buffer only once and ensure that it
> -                 * is big enough for this and all future requests.
> -                 */
> -                size_t bufsize = num <= nb_sectors ? num : max_write_zeroes;
> -                iov.iov_base = qemu_blockalign(bs, bufsize * BDRV_SECTOR_SIZE);
> -                memset(iov.iov_base, 0, bufsize * BDRV_SECTOR_SIZE);
> +                iov.iov_base = qemu_blockalign(bs, num * BDRV_SECTOR_SIZE);
> +                memset(iov.iov_base, 0, num * BDRV_SECTOR_SIZE);
>               }
>               qemu_iovec_init_external(&qiov, &iov, 1);
>   
>               ret = drv->bdrv_co_writev(bs, sector_num, num, &qiov);
> +
> +            /* Keep bounce buffer around if it is big enough for all
> +             * all future requests.
> +             */
> +            if (num < max_write_zeroes) {
> +                qemu_vfree(iov.iov_base);
> +                iov.iov_base = NULL;
> +            }
>           }
>   
>           sector_num += num;
Reviewed-by: Peter Lieven <pl@kamp.de>

[Qemu-devel] [PATCH v3 07/19] vpc, vhdx: add get_info

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vhdx.c | 10 ++++++++++
 block/vpc.c  | 14 ++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/block/vhdx.c b/block/vhdx.c
index 7d1af96..ed6fa53 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1043,6 +1043,15 @@ static void vhdx_block_translate(BDRVVHDXState *s, int64_t sector_num,
 }
 
 
+static int vhdx_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
+{
+    BDRVVHDXState *s = bs->opaque;
+
+    bdi->cluster_size = s->block_size;
+
+    return 0;
+}
+
 
 static coroutine_fn int vhdx_co_readv(BlockDriverState *bs, int64_t sector_num,
                                       int nb_sectors, QEMUIOVector *qiov)
@@ -1885,6 +1894,7 @@ static BlockDriver bdrv_vhdx = {
     .bdrv_co_readv          = vhdx_co_readv,
     .bdrv_co_writev         = vhdx_co_writev,
     .bdrv_create            = vhdx_create,
+    .bdrv_get_info          = vhdx_get_info,
 
     .create_options         = vhdx_create_options,
 };
diff --git a/block/vpc.c b/block/vpc.c
index 577cc45..551876f 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -455,6 +455,18 @@ fail:
     return -1;
 }
 
+static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
+{
+    BDRVVPCState *s = (BDRVVPCState *)bs->opaque;
+    VHDFooter *footer = (VHDFooter *) s->footer_buf;
+
+    if (cpu_to_be32(footer->type) != VHD_FIXED) {
+        bdi->cluster_size = s->block_size;
+    }
+
+    return 0;
+}
+
 static int vpc_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
@@ -857,6 +869,8 @@ static BlockDriver bdrv_vpc = {
     .bdrv_read              = vpc_co_read,
     .bdrv_write             = vpc_co_write,
 
+    .bdrv_get_info          = vpc_get_info,
+
     .create_options         = vpc_create_options,
     .bdrv_has_zero_init     = vpc_has_zero_init,
 };
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 07/19] vpc, vhdx: add get_info

[Peter Lieven]

On 22.11.2013 13:39, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block/vhdx.c | 10 ++++++++++
>   block/vpc.c  | 14 ++++++++++++++
>   2 files changed, 24 insertions(+)
>
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 7d1af96..ed6fa53 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1043,6 +1043,15 @@ static void vhdx_block_translate(BDRVVHDXState *s, int64_t sector_num,
>   }
>   
>   
> +static int vhdx_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> +{
> +    BDRVVHDXState *s = bs->opaque;
> +
> +    bdi->cluster_size = s->block_size;
> +
> +    return 0;
> +}
> +
>   
>   static coroutine_fn int vhdx_co_readv(BlockDriverState *bs, int64_t sector_num,
>                                         int nb_sectors, QEMUIOVector *qiov)
> @@ -1885,6 +1894,7 @@ static BlockDriver bdrv_vhdx = {
>       .bdrv_co_readv          = vhdx_co_readv,
>       .bdrv_co_writev         = vhdx_co_writev,
>       .bdrv_create            = vhdx_create,
> +    .bdrv_get_info          = vhdx_get_info,
>   
>       .create_options         = vhdx_create_options,
>   };
> diff --git a/block/vpc.c b/block/vpc.c
> index 577cc45..551876f 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -455,6 +455,18 @@ fail:
>       return -1;
>   }
>   
> +static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> +{
> +    BDRVVPCState *s = (BDRVVPCState *)bs->opaque;
> +    VHDFooter *footer = (VHDFooter *) s->footer_buf;
> +
> +    if (cpu_to_be32(footer->type) != VHD_FIXED) {
> +        bdi->cluster_size = s->block_size;
> +    }
> +
> +    return 0;
> +}
> +
>   static int vpc_read(BlockDriverState *bs, int64_t sector_num,
>                       uint8_t *buf, int nb_sectors)
>   {
> @@ -857,6 +869,8 @@ static BlockDriver bdrv_vpc = {
>       .bdrv_read              = vpc_co_read,
>       .bdrv_write             = vpc_co_write,
>   
> +    .bdrv_get_info          = vpc_get_info,
> +
>       .create_options         = vpc_create_options,
>       .bdrv_has_zero_init     = vpc_has_zero_init,
>   };
Reviewed-by: Peter Lieven <pl@kamp.de>

[Qemu-devel] [PATCH v3 08/19] block drivers: add discard/write_zeroes properties to bdrv_get_info implementation

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/qcow2.c | 2 ++
 block/qed.c   | 2 ++
 block/vdi.c   | 1 +
 block/vhdx.c  | 3 +++
 block/vpc.c   | 1 +
 5 files changed, 9 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2fe37ed..1542750 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1892,6 +1892,8 @@ static coroutine_fn int qcow2_co_flush_to_os(BlockDriverState *bs)
 static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BDRVQcowState *s = bs->opaque;
+    bdi->unallocated_blocks_are_zero = true;
+    bdi->can_write_zeroes_with_unmap = (s->qcow_version >= 3);
     bdi->cluster_size = s->cluster_size;
     bdi->vm_state_offset = qcow2_vm_state_offset(s);
     return 0;
diff --git a/block/qed.c b/block/qed.c
index adc2736..59516a5 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1475,6 +1475,8 @@ static int bdrv_qed_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     memset(bdi, 0, sizeof(*bdi));
     bdi->cluster_size = s->header.cluster_size;
     bdi->is_dirty = s->header.features & QED_F_NEED_CHECK;
+    bdi->unallocated_blocks_are_zero = true;
+    bdi->can_write_zeroes_with_unmap = true;
     return 0;
 }
 
diff --git a/block/vdi.c b/block/vdi.c
index b6ec002..2d7490f 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -331,6 +331,7 @@ static int vdi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     logout("\n");
     bdi->cluster_size = s->block_size;
     bdi->vm_state_offset = 0;
+    bdi->unallocated_blocks_are_zero = true;
     return 0;
 }
 
diff --git a/block/vhdx.c b/block/vhdx.c
index ed6fa53..67bbe10 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1049,6 +1049,9 @@ static int vhdx_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 
     bdi->cluster_size = s->block_size;
 
+    bdi->unallocated_blocks_are_zero =
+        (s->params.data_bits & VHDX_PARAMS_HAS_PARENT) == 0;
+
     return 0;
 }
 
diff --git a/block/vpc.c b/block/vpc.c
index 551876f..1d326cb 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -464,6 +464,7 @@ static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
         bdi->cluster_size = s->block_size;
     }
 
+    bdi->unallocated_blocks_are_zero = true;
     return 0;
 }
 
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 08/19] block drivers: add discard/write_zeroes properties to bdrv_get_info implementation

[Peter Lieven]

On 22.11.2013 13:39, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block/qcow2.c | 2 ++
>   block/qed.c   | 2 ++
>   block/vdi.c   | 1 +
>   block/vhdx.c  | 3 +++
>   block/vpc.c   | 1 +
>   5 files changed, 9 insertions(+)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2fe37ed..1542750 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1892,6 +1892,8 @@ static coroutine_fn int qcow2_co_flush_to_os(BlockDriverState *bs)
>   static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>   {
>       BDRVQcowState *s = bs->opaque;
> +    bdi->unallocated_blocks_are_zero = true;
> +    bdi->can_write_zeroes_with_unmap = (s->qcow_version >= 3);
>       bdi->cluster_size = s->cluster_size;
>       bdi->vm_state_offset = qcow2_vm_state_offset(s);
>       return 0;
> diff --git a/block/qed.c b/block/qed.c
> index adc2736..59516a5 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -1475,6 +1475,8 @@ static int bdrv_qed_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>       memset(bdi, 0, sizeof(*bdi));
>       bdi->cluster_size = s->header.cluster_size;
>       bdi->is_dirty = s->header.features & QED_F_NEED_CHECK;
> +    bdi->unallocated_blocks_are_zero = true;
> +    bdi->can_write_zeroes_with_unmap = true;
>       return 0;
>   }
>   
> diff --git a/block/vdi.c b/block/vdi.c
> index b6ec002..2d7490f 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -331,6 +331,7 @@ static int vdi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>       logout("\n");
>       bdi->cluster_size = s->block_size;
>       bdi->vm_state_offset = 0;
> +    bdi->unallocated_blocks_are_zero = true;
>       return 0;
>   }
>   
> diff --git a/block/vhdx.c b/block/vhdx.c
> index ed6fa53..67bbe10 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1049,6 +1049,9 @@ static int vhdx_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>   
>       bdi->cluster_size = s->block_size;
>   
> +    bdi->unallocated_blocks_are_zero =
> +        (s->params.data_bits & VHDX_PARAMS_HAS_PARENT) == 0;
> +
>       return 0;
>   }
>   
> diff --git a/block/vpc.c b/block/vpc.c
> index 551876f..1d326cb 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -464,6 +464,7 @@ static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>           bdi->cluster_size = s->block_size;
>       }
>   
> +    bdi->unallocated_blocks_are_zero = true;
>       return 0;
>   }
>   
Reviewed-by: Peter Lieven <pl@kamp.de>

Re: [Qemu-devel] [PATCH v3 08/19] block drivers: add discard/write_zeroes properties to bdrv_get_info implementation

[Kevin Wolf]

Am 22.11.2013 um 13:39 hat Paolo Bonzini geschrieben:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/qcow2.c | 2 ++
>  block/qed.c   | 2 ++
>  block/vdi.c   | 1 +
>  block/vhdx.c  | 3 +++
>  block/vpc.c   | 1 +
>  5 files changed, 9 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2fe37ed..1542750 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1892,6 +1892,8 @@ static coroutine_fn int qcow2_co_flush_to_os(BlockDriverState *bs)
>  static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>  {
>      BDRVQcowState *s = bs->opaque;
> +    bdi->unallocated_blocks_are_zero = true;
> +    bdi->can_write_zeroes_with_unmap = (s->qcow_version >= 3);
>      bdi->cluster_size = s->cluster_size;
>      bdi->vm_state_offset = qcow2_vm_state_offset(s);
>      return 0;

We must change qcow2_discard_clusters() to set the zero flag instead of
just deallocating the cluster. (We should be doing that anyway, because
the backing file reappearing isn't very nice.)

Not quite sure what to do with version 2 images, though.

For the other formats, I guess this is only correct because they don't
implement discard anyway?

Kevin

Re: [Qemu-devel] [PATCH v3 08/19] block drivers: add discard/write_zeroes properties to bdrv_get_info implementation

[Paolo Bonzini]

Il 03/12/2013 16:09, Kevin Wolf ha scritto:
>> >      BDRVQcowState *s = bs->opaque;
>> > +    bdi->unallocated_blocks_are_zero = true;
>> > +    bdi->can_write_zeroes_with_unmap = (s->qcow_version >= 3);
>> >      bdi->cluster_size = s->cluster_size;
>> >      bdi->vm_state_offset = qcow2_vm_state_offset(s);
>> >      return 0;
> We must change qcow2_discard_clusters() to set the zero flag instead of
> just deallocating the cluster. (We should be doing that anyway, because
> the backing file reappearing isn't very nice.)

No, that's not needed:

* unallocated_blocks_are_zero is only meaningful for bs->backing_hd ==
NULL (not too intuitive, but I didn't introduce that interface :)).  In
fact, v2 was checking backing_hd == NULL but I removed it after Peter
noticed I was being inconsistent.

* can_write_zeroes_with_unmap correctly returns true only if zero
clusters are enabled

> For the other formats, I guess this is only correct because they don't
> implement discard anyway?

No, it is correct because that's what their bdrv_co_readv (or similar)
will return when a block is not allocated and there is no backing file.
 Of course, for many formats there will never be a backing file.

Paolo

Re: [Qemu-devel] [PATCH v3 08/19] block drivers: add discard/write_zeroes properties to bdrv_get_info implementation

[Peter Lieven]

> unallocated_blocks_are_zero  is false if there is a backing hd. Same as has_zero_init.

Peter 

> Am 03.12.2013 um 16:21 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
> Il 03/12/2013 16:09, Kevin Wolf ha scritto:
>>>>     BDRVQcowState *s = bs->opaque;
>>>> +    bdi->unallocated_blocks_are_zero = true;
>>>> +    bdi->can_write_zeroes_with_unmap = (s->qcow_version >= 3);
>>>>     bdi->cluster_size = s->cluster_size;
>>>>     bdi->vm_state_offset = qcow2_vm_state_offset(s);
>>>>     return 0;
>> We must change qcow2_discard_clusters() to set the zero flag instead of
>> just deallocating the cluster. (We should be doing that anyway, because
>> the backing file reappearing isn't very nice.)
> 
> No, that's not needed:
> 
> * unallocated_blocks_are_zero is only meaningful for bs->backing_hd ==
> NULL (not too intuitive, but I didn't introduce that interface :)).  In
> fact, v2 was checking backing_hd == NULL but I removed it after Peter
> noticed I was being inconsistent.
> 
> * can_write_zeroes_with_unmap correctly returns true only if zero
> clusters are enabled
> 
>> For the other formats, I guess this is only correct because they don't
>> implement discard anyway?
> 
> No, it is correct because that's what their bdrv_co_readv (or similar)
> will return when a block is not allocated and there is no backing file.
> Of course, for many formats there will never be a backing file.
> 
> Paolo

[Qemu-devel] [PATCH v3 09/19] block drivers: expose requirement for write same alignment from formats

[Paolo Bonzini]

This will let misaligned but large requests use zero clusters.  This
is important because the cluster size is not guest visible.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/qcow2.c | 1 +
 block/qed.c   | 1 +
 block/vmdk.c  | 4 ++++
 3 files changed, 6 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 1542750..81c4dad 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -718,6 +718,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     qemu_opts_del(opts);
+    bs->bl.write_zeroes_alignment = s->cluster_sectors;
 
     if (s->use_lazy_refcounts && s->qcow_version < 3) {
         error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
diff --git a/block/qed.c b/block/qed.c
index 59516a5..450a1fa 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -495,6 +495,7 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
+    bs->bl.write_zeroes_alignment = s->header.cluster_size >> BDRV_SECTOR_BITS;
     s->need_check_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                             qed_need_check_timer_cb, s);
 
diff --git a/block/vmdk.c b/block/vmdk.c
index 6555663..63ea7ff 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -428,6 +428,10 @@ static int vmdk_add_extent(BlockDriverState *bs,
     extent->l2_size = l2_size;
     extent->cluster_sectors = flat ? sectors : cluster_sectors;
 
+    if (!flat) {
+        bs->bl.write_zeroes_alignment =
+            MAX(bs->bl.write_zeroes_alignment, cluster_sectors);
+    }
     if (s->num_extents > 1) {
         extent->end_sector = (*(extent - 1)).end_sector + extent->sectors;
     } else {
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 09/19] block drivers: expose requirement for write same alignment from formats

[Peter Lieven]

On 22.11.2013 13:39, Paolo Bonzini wrote:
> This will let misaligned but large requests use zero clusters.  This
> is important because the cluster size is not guest visible.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block/qcow2.c | 1 +
>   block/qed.c   | 1 +
>   block/vmdk.c  | 4 ++++
>   3 files changed, 6 insertions(+)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 1542750..81c4dad 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -718,6 +718,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>   
>       qemu_opts_del(opts);
> +    bs->bl.write_zeroes_alignment = s->cluster_sectors;
please add a patch that sets

bs->bl.discard_alignment = s->cluster_sectors;

>   
>       if (s->use_lazy_refcounts && s->qcow_version < 3) {
>           error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
> diff --git a/block/qed.c b/block/qed.c
> index 59516a5..450a1fa 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -495,6 +495,7 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
>           }
>       }
>   
> +    bs->bl.write_zeroes_alignment = s->header.cluster_size >> BDRV_SECTOR_BITS;
>       s->need_check_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                               qed_need_check_timer_cb, s);
>   
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 6555663..63ea7ff 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -428,6 +428,10 @@ static int vmdk_add_extent(BlockDriverState *bs,
>       extent->l2_size = l2_size;
>       extent->cluster_sectors = flat ? sectors : cluster_sectors;
>   
> +    if (!flat) {
> +        bs->bl.write_zeroes_alignment =
> +            MAX(bs->bl.write_zeroes_alignment, cluster_sectors);
> +    }
>       if (s->num_extents > 1) {
>           extent->end_sector = (*(extent - 1)).end_sector + extent->sectors;
>       } else {
>

Reviewed-by: Peter Lieven <pl@kamp.de>

[Qemu-devel] [PATCH v3 10/19] block/iscsi: remove .bdrv_has_zero_init

[Paolo Bonzini]

From: Peter Lieven <pl@kamp.de>

since commit 3ac21627 the default value changed to 0.

Signed-off-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index b7b5238..b6b62aa 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1505,11 +1505,6 @@ static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
     return 0;
 }
 
-static int iscsi_has_zero_init(BlockDriverState *bs)
-{
-    return 0;
-}
-
 static int iscsi_create(const char *filename, QEMUOptionParameter *options,
                         Error **errp)
 {
@@ -1608,8 +1603,6 @@ static BlockDriver bdrv_iscsi = {
     .bdrv_aio_writev = iscsi_aio_writev,
     .bdrv_aio_flush  = iscsi_aio_flush,
 
-    .bdrv_has_zero_init = iscsi_has_zero_init,
-
 #ifdef __linux__
     .bdrv_ioctl       = iscsi_ioctl,
     .bdrv_aio_ioctl   = iscsi_aio_ioctl,
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 11/19] block/iscsi: updated copyright

[Paolo Bonzini]

From: Peter Lieven <pl@kamp.de>

added myself to reflect recent work on the iscsi block driver.

Signed-off-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index b6b62aa..20f4f55 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2,6 +2,7 @@
  * QEMU Block driver for iSCSI images
  *
  * Copyright (c) 2010-2011 Ronnie Sahlberg <ronniesahlberg@gmail.com>
+ * Copyright (c) 2012-2013 Peter Lieven <pl@kamp.de>
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 12/19] block/iscsi: check WRITE SAME support differently depending on MAY_UNMAP

[Paolo Bonzini]

The current check is right for MAY_UNMAP=1.  For MAY_UNMAP=0, just
try and fall back to regular writes as soon as a WRITE SAME command
fails.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 20f4f55..93fee6d 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -55,6 +55,7 @@ typedef struct IscsiLun {
     QEMUTimer *nop_timer;
     uint8_t lbpme;
     uint8_t lbprz;
+    uint8_t has_write_same;
     struct scsi_inquiry_logical_block_provisioning lbp;
     struct scsi_inquiry_block_limits bl;
     unsigned char *zeroblock;
@@ -976,8 +977,13 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
         return -EINVAL;
     }
 
-    if (!iscsilun->lbp.lbpws) {
-        /* WRITE SAME is not supported by the target */
+    if (!(flags & BDRV_REQ_MAY_UNMAP) && !iscsilun->has_write_same) {
+        /* WRITE SAME without UNMAP is not supported by the target */
+        return -ENOTSUP;
+    }
+
+    if ((flags & BDRV_REQ_MAY_UNMAP) && !iscsilun->lbp.lbpws) {
+        /* WRITE SAME with UNMAP is not supported by the target */
         return -ENOTSUP;
     }
 
@@ -1012,6 +1018,14 @@ retry:
     }
 
     if (iTask.status != SCSI_STATUS_GOOD) {
+        if (iTask.status == SCSI_STATUS_CHECK_CONDITION &&
+            iTask.task->sense.key == SCSI_SENSE_ILLEGAL_REQUEST &&
+            iTask.task->sense.ascq == SCSI_SENSE_ASCQ_INVALID_OPERATION_CODE) {
+            /* WRITE SAME is not supported by the target */
+            iscsilun->has_write_same = false;
+            return -ENOTSUP;
+        }
+
         return -EIO;
     }
 
@@ -1375,6 +1389,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     iscsilun->type = inq->periperal_device_type;
+    iscsilun->has_write_same = true;
 
     if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
         goto out;
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 12/19] block/iscsi: check WRITE SAME support differently depending on MAY_UNMAP

[Peter Lieven]

On 22.11.2013 13:39, Paolo Bonzini wrote:
> The current check is right for MAY_UNMAP=1.  For MAY_UNMAP=0, just
> try and fall back to regular writes as soon as a WRITE SAME command
> fails.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block/iscsi.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 20f4f55..93fee6d 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -55,6 +55,7 @@ typedef struct IscsiLun {
>       QEMUTimer *nop_timer;
>       uint8_t lbpme;
>       uint8_t lbprz;
> +    uint8_t has_write_same;
>       struct scsi_inquiry_logical_block_provisioning lbp;
>       struct scsi_inquiry_block_limits bl;
>       unsigned char *zeroblock;
> @@ -976,8 +977,13 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
>           return -EINVAL;
>       }
>   
> -    if (!iscsilun->lbp.lbpws) {
> -        /* WRITE SAME is not supported by the target */
> +    if (!(flags & BDRV_REQ_MAY_UNMAP) && !iscsilun->has_write_same) {
> +        /* WRITE SAME without UNMAP is not supported by the target */
> +        return -ENOTSUP;
> +    }
> +
> +    if ((flags & BDRV_REQ_MAY_UNMAP) && !iscsilun->lbp.lbpws) {
> +        /* WRITE SAME with UNMAP is not supported by the target */
>           return -ENOTSUP;
>       }
>   
> @@ -1012,6 +1018,14 @@ retry:
>       }
>   
>       if (iTask.status != SCSI_STATUS_GOOD) {
> +        if (iTask.status == SCSI_STATUS_CHECK_CONDITION &&
> +            iTask.task->sense.key == SCSI_SENSE_ILLEGAL_REQUEST &&
> +            iTask.task->sense.ascq == SCSI_SENSE_ASCQ_INVALID_OPERATION_CODE) {
> +            /* WRITE SAME is not supported by the target */
> +            iscsilun->has_write_same = false;
> +            return -ENOTSUP;
> +        }
> +
>           return -EIO;
>       }
>   
> @@ -1375,6 +1389,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>   
>       iscsilun->type = inq->periperal_device_type;
> +    iscsilun->has_write_same = true;
>   
>       if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
>           goto out;
Maybe the naming has_write_same is misleading. It might be better to call
it try_write_same or has_write_same_failed with inverse logic.

If you just change the naming:

Reviewed-by: Peter Lieven <pl@kamp.de>

Peter

Re: [Qemu-devel] [PATCH v3 12/19] block/iscsi: check WRITE SAME support differently depending on MAY_UNMAP

[Paolo Bonzini]

Il 25/11/2013 11:34, Peter Lieven ha scritto:
>>   @@ -1012,6 +1018,14 @@ retry:
>>       }
>>         if (iTask.status != SCSI_STATUS_GOOD) {
>> +        if (iTask.status == SCSI_STATUS_CHECK_CONDITION &&
>> +            iTask.task->sense.key == SCSI_SENSE_ILLEGAL_REQUEST &&
>> +            iTask.task->sense.ascq ==
>> SCSI_SENSE_ASCQ_INVALID_OPERATION_CODE) {
>> +            /* WRITE SAME is not supported by the target */
>> +            iscsilun->has_write_same = false;
>> +            return -ENOTSUP;
>> +        }
>> +
>>           return -EIO;
>>       }
>>   @@ -1375,6 +1389,7 @@ static int iscsi_open(BlockDriverState *bs,
>> QDict *options, int flags,
>>       }
>>         iscsilun->type = inq->periperal_device_type;
>> +    iscsilun->has_write_same = true;
>>         if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
>>           goto out;
> 
> Maybe the naming has_write_same is misleading. It might be better to call
> it try_write_same or has_write_same_failed with inverse logic.

I was using the same names as block/raw-posix.c.  I'm not sure I like
the other names, but if the maintainers prefer them I'll gladly change them.

Paolo

[Qemu-devel] [PATCH v3 13/19] raw-posix: implement write_zeroes with MAY_UNMAP for files

[Paolo Bonzini]

Writing zeroes to a file can be done by punching a hole if
MAY_UNMAP is set.

Note that in this case ENOTSUP is not ignored, but makes
the block layer fall back to the generic implementation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/raw-posix.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 trace-events      |  1 +
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index cfa3162..7f3f47d 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -139,9 +139,10 @@ typedef struct BDRVRawState {
     void *aio_ctx;
 #endif
 #ifdef CONFIG_XFS
-    bool is_xfs : 1;
+    bool is_xfs:1;
 #endif
-    bool has_discard : 1;
+    bool has_discard:1;
+    bool discard_zeroes:1;
 } BDRVRawState;
 
 typedef struct BDRVRawReopenState {
@@ -283,6 +284,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     Error *local_err = NULL;
     const char *filename;
     int fd, ret;
+    struct stat st;
 
     opts = qemu_opts_create_nofail(&raw_runtime_opts);
     qemu_opts_absorb_qdict(opts, options, &local_err);
@@ -324,6 +326,15 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
 #endif
 
     s->has_discard = true;
+
+    if (fstat(s->fd, &st) < 0) {
+        error_setg_errno(errp, errno, "Could not stat file");
+        goto fail;
+    }
+    if (S_ISREG(st.st_mode)) {
+        s->discard_zeroes = true;
+    }
+
 #ifdef CONFIG_XFS
     if (platform_test_xfs_fd(s->fd)) {
         s->is_xfs = true;
@@ -787,6 +798,29 @@ static int aio_worker(void *arg)
     return ret;
 }
 
+static int paio_submit_co(BlockDriverState *bs, int fd,
+        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+        int type)
+{
+    RawPosixAIOData *acb = g_slice_new(RawPosixAIOData);
+    ThreadPool *pool;
+
+    acb->bs = bs;
+    acb->aio_type = type;
+    acb->aio_fildes = fd;
+
+    if (qiov) {
+        acb->aio_iov = qiov->iov;
+        acb->aio_niov = qiov->niov;
+    }
+    acb->aio_nbytes = nb_sectors * 512;
+    acb->aio_offset = sector_num * 512;
+
+    trace_paio_submit_co(sector_num, nb_sectors, type);
+    pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
+    return thread_pool_submit_co(pool, aio_worker, acb);
+}
+
 static BlockDriverAIOCB *paio_submit(BlockDriverState *bs, int fd,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque, int type)
@@ -1199,6 +1233,31 @@ static coroutine_fn BlockDriverAIOCB *raw_aio_discard(BlockDriverState *bs,
                        cb, opaque, QEMU_AIO_DISCARD);
 }
 
+static int coroutine_fn raw_co_write_zeroes(
+    BlockDriverState *bs, int64_t sector_num,
+    int nb_sectors, BdrvRequestFlags flags)
+{
+    BDRVRawState *s = bs->opaque;
+
+    if (!(flags & BDRV_REQ_MAY_UNMAP)) {
+        return -ENOTSUP;
+    }
+    if (!s->discard_zeroes) {
+        return -ENOTSUP;
+    }
+    return paio_submit_co(bs, s->fd, sector_num, NULL, nb_sectors,
+                          QEMU_AIO_DISCARD);
+}
+
+static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
+{
+    BDRVRawState *s = bs->opaque;
+
+    bdi->unallocated_blocks_are_zero = s->discard_zeroes;
+    bdi->can_write_zeroes_with_unmap = s->discard_zeroes;
+    return 0;
+}
+
 static QEMUOptionParameter raw_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1222,6 +1281,7 @@ static BlockDriver bdrv_file = {
     .bdrv_create = raw_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_get_block_status = raw_co_get_block_status,
+    .bdrv_co_write_zeroes = raw_co_write_zeroes,
 
     .bdrv_aio_readv = raw_aio_readv,
     .bdrv_aio_writev = raw_aio_writev,
@@ -1230,6 +1290,7 @@ static BlockDriver bdrv_file = {
 
     .bdrv_truncate = raw_truncate,
     .bdrv_getlength = raw_getlength,
+    .bdrv_get_info = raw_get_info,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
 
@@ -1585,6 +1646,7 @@ static BlockDriver bdrv_host_device = {
 
     .bdrv_truncate      = raw_truncate,
     .bdrv_getlength	= raw_getlength,
+    .bdrv_get_info = raw_get_info,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
 
diff --git a/trace-events b/trace-events
index d318d6f..e32d00c 100644
--- a/trace-events
+++ b/trace-events
@@ -128,6 +128,7 @@ thread_pool_cancel(void *req, void *opaque) "req %p opaque %p"
 
 # block/raw-win32.c
 # block/raw-posix.c
+paio_submit_co(int64_t sector_num, int nb_sectors, int type) "sector_num %"PRId64" nb_sectors %d type %d"
 paio_submit(void *acb, void *opaque, int64_t sector_num, int nb_sectors, int type) "acb %p opaque %p sector_num %"PRId64" nb_sectors %d type %d"
 
 # ioport.c
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 14/19] raw-posix: implement write_zeroes with MAY_UNMAP for block devices

[Paolo Bonzini]

See the next commit for the description of the Linux kernel problem
that is worked around in raw_open_common.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/raw-posix.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 7f3f47d..b3feed6 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -334,6 +334,22 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     if (S_ISREG(st.st_mode)) {
         s->discard_zeroes = true;
     }
+    if (S_ISBLK(st.st_mode)) {
+#ifdef BLKDISCARDZEROES
+        unsigned int arg;
+        if (ioctl(s->fd, BLKDISCARDZEROES, &arg) == 0 && arg) {
+            s->discard_zeroes = true;
+        }
+#endif
+#ifdef __linux__
+        /* On Linux 3.10, BLKDISCARD leaves stale data in the page cache.  Do
+         * not rely on the contents of discarded blocks unless using O_DIRECT.
+         */
+        if (!(bs->open_flags & BDRV_O_NOCACHE)) {
+            s->discard_zeroes = false;
+        }
+#endif
+    }
 
 #ifdef CONFIG_XFS
     if (platform_test_xfs_fd(s->fd)) {
@@ -1586,6 +1602,26 @@ static coroutine_fn BlockDriverAIOCB *hdev_aio_discard(BlockDriverState *bs,
                        cb, opaque, QEMU_AIO_DISCARD|QEMU_AIO_BLKDEV);
 }
 
+static coroutine_fn int hdev_co_write_zeroes(BlockDriverState *bs,
+    int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
+{
+    BDRVRawState *s = bs->opaque;
+    int rc;
+
+    rc = fd_open(bs);
+    if (rc < 0) {
+        return rc;
+    }
+    if (!(flags & BDRV_REQ_MAY_UNMAP)) {
+        return -ENOTSUP;
+    }
+    if (!s->discard_zeroes) {
+        return -ENOTSUP;
+    }
+    return paio_submit_co(bs, s->fd, sector_num, NULL, nb_sectors,
+                          QEMU_AIO_DISCARD|QEMU_AIO_BLKDEV);
+}
+
 static int hdev_create(const char *filename, QEMUOptionParameter *options,
                        Error **errp)
 {
@@ -1638,6 +1674,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_reopen_abort   = raw_reopen_abort,
     .bdrv_create        = hdev_create,
     .create_options     = raw_create_options,
+    .bdrv_co_write_zeroes = hdev_co_write_zeroes,
 
     .bdrv_aio_readv	= raw_aio_readv,
     .bdrv_aio_writev	= raw_aio_writev,
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 15/19] raw-posix: add support for write_zeroes on XFS and block devices

[Paolo Bonzini]

The code is similar to the implementation of discard and write_zeroes
with UNMAP.  However, failure must be propagated up to block.c.

The stale page cache problem can be reproduced as follows:

    # modprobe scsi-debug lbpws=1 lbprz=1
    # ./qemu-io /dev/sdXX
    qemu-io> write -P 0xcc 0 2M
    qemu-io> write -z 0 1M
    qemu-io> read -P 0x00 0 512
    Pattern verification failed at offset 0, 512 bytes
    qemu-io> read -v 0 512
    00000000:  cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc  ................
    ...

    # ./qemu-io --cache=none /dev/sdXX
    qemu-io> write -P 0xcc 0 2M
    qemu-io> write -z 0 1M
    qemu-io> read -P 0x00 0 512
    qemu-io> read -v 0 512
    00000000:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    ...

And similarly with discard instead of "write -z".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/raw-aio.h   |  3 +-
 block/raw-posix.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 74 insertions(+), 13 deletions(-)

diff --git a/block/raw-aio.h b/block/raw-aio.h
index c61f159..7ad0a8a 100644
--- a/block/raw-aio.h
+++ b/block/raw-aio.h
@@ -21,9 +21,10 @@
 #define QEMU_AIO_IOCTL        0x0004
 #define QEMU_AIO_FLUSH        0x0008
 #define QEMU_AIO_DISCARD      0x0010
+#define QEMU_AIO_WRITE_ZEROES 0x0020
 #define QEMU_AIO_TYPE_MASK \
         (QEMU_AIO_READ|QEMU_AIO_WRITE|QEMU_AIO_IOCTL|QEMU_AIO_FLUSH| \
-         QEMU_AIO_DISCARD)
+         QEMU_AIO_DISCARD|QEMU_AIO_WRITE_ZEROES)
 
 /* AIO flags */
 #define QEMU_AIO_MISALIGNED   0x1000
diff --git a/block/raw-posix.c b/block/raw-posix.c
index b3feed6..10c6b34 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -142,6 +142,7 @@ typedef struct BDRVRawState {
     bool is_xfs:1;
 #endif
     bool has_discard:1;
+    bool has_write_zeroes:1;
     bool discard_zeroes:1;
 } BDRVRawState;
 
@@ -326,6 +327,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
 #endif
 
     s->has_discard = true;
+    s->has_write_zeroes = true;
 
     if (fstat(s->fd, &st) < 0) {
         error_setg_errno(errp, errno, "Could not stat file");
@@ -344,9 +346,11 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
 #ifdef __linux__
         /* On Linux 3.10, BLKDISCARD leaves stale data in the page cache.  Do
          * not rely on the contents of discarded blocks unless using O_DIRECT.
+         * Same for BLKZEROOUT.
          */
         if (!(bs->open_flags & BDRV_O_NOCACHE)) {
             s->discard_zeroes = false;
+            s->has_write_zeroes = false;
         }
 #endif
     }
@@ -702,6 +706,23 @@ static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
 }
 
 #ifdef CONFIG_XFS
+static int xfs_write_zeroes(BDRVRawState *s, int64_t offset, uint64_t bytes)
+{
+    struct xfs_flock64 fl;
+
+    memset(&fl, 0, sizeof(fl));
+    fl.l_whence = SEEK_SET;
+    fl.l_start = offset;
+    fl.l_len = bytes;
+
+    if (xfsctl(NULL, s->fd, XFS_IOC_ZERO_RANGE, &fl) < 0) {
+        DEBUG_BLOCK_PRINT("cannot write zero range (%s)\n", strerror(errno));
+        return -errno;
+    }
+
+    return 0;
+}
+
 static int xfs_discard(BDRVRawState *s, int64_t offset, uint64_t bytes)
 {
     struct xfs_flock64 fl;
@@ -720,6 +741,42 @@ static int xfs_discard(BDRVRawState *s, int64_t offset, uint64_t bytes)
 }
 #endif
 
+static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
+{
+    int ret = -EOPNOTSUPP;
+    BDRVRawState *s = aiocb->bs->opaque;
+
+    if (s->has_write_zeroes == 0) {
+        return -ENOTSUP;
+    }
+
+    if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
+#ifdef BLKZEROOUT
+        do {
+            uint64_t range[2] = { aiocb->aio_offset, aiocb->aio_nbytes };
+            if (ioctl(aiocb->aio_fildes, BLKZEROOUT, range) == 0) {
+                return 0;
+            }
+        } while (errno == EINTR);
+
+        ret = -errno;
+#endif
+    } else {
+#ifdef CONFIG_XFS
+        if (s->is_xfs) {
+            return xfs_write_zeroes(s, aiocb->aio_offset, aiocb->aio_nbytes);
+        }
+#endif
+    }
+
+    if (ret == -ENODEV || ret == -ENOSYS || ret == -EOPNOTSUPP ||
+        ret == -ENOTTY) {
+        s->has_write_zeroes = false;
+        ret = -ENOTSUP;
+    }
+    return ret;
+}
+
 static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
 {
     int ret = -EOPNOTSUPP;
@@ -804,6 +861,9 @@ static int aio_worker(void *arg)
     case QEMU_AIO_DISCARD:
         ret = handle_aiocb_discard(aiocb);
         break;
+    case QEMU_AIO_WRITE_ZEROES:
+        ret = handle_aiocb_write_zeroes(aiocb);
+        break;
     default:
         fprintf(stderr, "invalid aio request (0x%x)\n", aiocb->aio_type);
         ret = -EINVAL;
@@ -1256,13 +1316,13 @@ static int coroutine_fn raw_co_write_zeroes(
     BDRVRawState *s = bs->opaque;
 
     if (!(flags & BDRV_REQ_MAY_UNMAP)) {
-        return -ENOTSUP;
-    }
-    if (!s->discard_zeroes) {
-        return -ENOTSUP;
+        return paio_submit_co(bs, s->fd, sector_num, NULL, nb_sectors,
+                              QEMU_AIO_WRITE_ZEROES);
+    } else if (s->discard_zeroes) {
+        return paio_submit_co(bs, s->fd, sector_num, NULL, nb_sectors,
+                              QEMU_AIO_DISCARD);
     }
-    return paio_submit_co(bs, s->fd, sector_num, NULL, nb_sectors,
-                          QEMU_AIO_DISCARD);
+    return -ENOTSUP;
 }
 
 static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
@@ -1613,13 +1673,13 @@ static coroutine_fn int hdev_co_write_zeroes(BlockDriverState *bs,
         return rc;
     }
     if (!(flags & BDRV_REQ_MAY_UNMAP)) {
-        return -ENOTSUP;
-    }
-    if (!s->discard_zeroes) {
-        return -ENOTSUP;
+        return paio_submit_co(bs, s->fd, sector_num, NULL, nb_sectors,
+                              QEMU_AIO_WRITE_ZEROES|QEMU_AIO_BLKDEV);
+    } else if (s->discard_zeroes) {
+        return paio_submit_co(bs, s->fd, sector_num, NULL, nb_sectors,
+                              QEMU_AIO_DISCARD|QEMU_AIO_BLKDEV);
     }
-    return paio_submit_co(bs, s->fd, sector_num, NULL, nb_sectors,
-                          QEMU_AIO_DISCARD|QEMU_AIO_BLKDEV);
+    return -ENOTSUP;
 }
 
 static int hdev_create(const char *filename, QEMUOptionParameter *options,
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 16/19] qemu-iotests: 033 is fast

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/qemu-iotests/group | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b63b18c..303e0f3 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -39,7 +39,7 @@
 030 rw auto backing
 031 rw auto quick
 032 rw auto
-033 rw auto
+033 rw auto quick
 034 rw auto backing
 035 rw auto quick
 036 rw auto quick
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 17/19] scsi-disk: catch write protection errors in UNMAP

[Paolo Bonzini]

This is the same that is already done for WRITE SAME.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 74e6a14..4138268 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1543,6 +1543,7 @@ done:
 
 static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
 {
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
     uint8_t *p = inbuf;
     int len = r->req.cmd.xfer;
     UnmapCBData *data;
@@ -1560,6 +1561,11 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
         goto invalid_param_len;
     }
 
+    if (bdrv_is_read_only(s->qdev.conf.bs)) {
+        scsi_check_condition(r, SENSE_CODE(WRITE_PROTECTED));
+        return;
+    }
+
     data = g_new0(UnmapCBData, 1);
     data->r = r;
     data->inbuf = &p[8];
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 18/19] scsi-disk: reject ANCHOR=1 for UNMAP and WRITE SAME commands

[Paolo Bonzini]

Since we report ANC_SUP==0 in VPD page B2h, we need to return
an error (ILLEGAL REQUEST/INVALID FIELD IN CDB) for all WRITE SAME
requests with ANCHOR==1.

Inspired by a similar patch to the LIO in-kernel target.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 4138268..0640bb0 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1548,6 +1548,11 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
     int len = r->req.cmd.xfer;
     UnmapCBData *data;
 
+    /* Reject ANCHOR=1.  */
+    if (r->req.cmd.buf[1] & 0x1) {
+        goto invalid_field;
+    }
+
     if (len < 8) {
         goto invalid_param_len;
     }
@@ -1578,6 +1583,10 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
 
 invalid_param_len:
     scsi_check_condition(r, SENSE_CODE(INVALID_PARAM_LEN));
+    return;
+
+invalid_field:
+    scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
 }
 
 static void scsi_disk_emulate_write_data(SCSIRequest *req)
@@ -1856,8 +1865,9 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
 
         /*
          * We only support WRITE SAME with the unmap bit set for now.
+         * Reject UNMAP=0 or ANCHOR=1.
          */
-        if (!(req->cmd.buf[1] & 0x8)) {
+        if (!(req->cmd.buf[1] & 0x8) || (req->cmd.buf[1] & 0x10)) {
             goto illegal_request;
         }
 
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 19/19] scsi-disk: correctly implement WRITE SAME

[Paolo Bonzini]

Fetch the data to be written from the input buffer.  If it is all zeroes,
we can use the write_zeroes call (possibly with the new MAY_UNMAP flag).
Otherwise, do as many write cycles as needed, writing 512k at a time.

Strictly speaking, this is still incorrect because a zero cluster should
only be written if the MAY_UNMAP flag is set.  But this is a bug in qcow2
and the other formats, not in the SCSI code.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c | 140 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 116 insertions(+), 24 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 0640bb0..efadfc0 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -41,6 +41,7 @@ do { printf("scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
 #include <scsi/sg.h>
 #endif
 
+#define SCSI_WRITE_SAME_MAX         524288
 #define SCSI_DMA_BUF_SIZE           131072
 #define SCSI_MAX_INQUIRY_LEN        256
 #define SCSI_MAX_MODE_LEN           256
@@ -634,6 +635,8 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             buflen = 0x40;
             memset(outbuf + 4, 0, buflen - 4);
 
+            outbuf[4] = 0x1; /* wsnz */
+
             /* optimal transfer length granularity */
             outbuf[6] = (min_io_size >> 8) & 0xff;
             outbuf[7] = min_io_size & 0xff;
@@ -1589,6 +1592,111 @@ invalid_field:
     scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
 }
 
+typedef struct WriteSameCBData {
+    SCSIDiskReq *r;
+    int64_t sector;
+    int nb_sectors;
+    QEMUIOVector qiov;
+    struct iovec iov;
+} WriteSameCBData;
+
+static void scsi_write_same_complete(void *opaque, int ret)
+{
+    WriteSameCBData *data = opaque;
+    SCSIDiskReq *r = data->r;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+    assert(r->req.aiocb != NULL);
+    r->req.aiocb = NULL;
+    bdrv_acct_done(s->qdev.conf.bs, &r->acct);
+    if (r->req.io_canceled) {
+        goto done;
+    }
+
+    if (ret < 0) {
+        if (scsi_handle_rw_error(r, -ret)) {
+            goto done;
+        }
+    }
+
+    data->nb_sectors -= data->iov.iov_len / 512;
+    data->sector += data->iov.iov_len / 512;
+    data->iov.iov_len = MIN(data->nb_sectors * 512, data->iov.iov_len);
+    if (data->iov.iov_len) {
+        bdrv_acct_start(s->qdev.conf.bs, &r->acct, data->iov.iov_len, BDRV_ACCT_WRITE);
+        r->req.aiocb = bdrv_aio_writev(s->qdev.conf.bs, data->sector,
+                                       &data->qiov, data->iov.iov_len / 512,
+                                       scsi_write_same_complete, r);
+        return;
+    }
+
+    scsi_req_complete(&r->req, GOOD);
+
+done:
+    if (!r->req.io_canceled) {
+        scsi_req_unref(&r->req);
+    }
+    qemu_vfree(data->iov.iov_base);
+    g_free(data);
+}
+
+static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf)
+{
+    SCSIRequest *req = &r->req;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
+    uint32_t nb_sectors = scsi_data_cdb_length(r->req.cmd.buf);
+    WriteSameCBData *data;
+    uint8_t *buf;
+    int i;
+
+    /* Fail if PBDATA=1 or LBDATA=1 or ANCHOR=1.  */
+    if (nb_sectors == 0 || (req->cmd.buf[1] & 0x16)) {
+        scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
+        return;
+    }
+
+    if (bdrv_is_read_only(s->qdev.conf.bs)) {
+        scsi_check_condition(r, SENSE_CODE(WRITE_PROTECTED));
+        return;
+    }
+    if (!check_lba_range(s, r->req.cmd.lba, nb_sectors)) {
+        scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
+        return;
+    }
+
+    if (buffer_is_zero(inbuf, s->qdev.blocksize)) {
+        int flags = (req->cmd.buf[1] & 0x8) ? BDRV_REQ_MAY_UNMAP : 0;
+
+        /* The request is used as the AIO opaque value, so add a ref.  */
+        scsi_req_ref(&r->req);
+        bdrv_acct_start(s->qdev.conf.bs, &r->acct, nb_sectors * s->qdev.blocksize,
+                        BDRV_ACCT_WRITE);
+        r->req.aiocb = bdrv_aio_write_zeroes(s->qdev.conf.bs,
+                                             r->req.cmd.lba * (s->qdev.blocksize / 512),
+                                             nb_sectors * (s->qdev.blocksize / 512),
+                                             flags, scsi_aio_complete, r);
+        return;
+    }
+
+    data = g_new0(WriteSameCBData, 1);
+    data->r = r;
+    data->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
+    data->nb_sectors = nb_sectors * (s->qdev.blocksize / 512);
+    data->iov.iov_len = MIN(data->nb_sectors * 512, SCSI_WRITE_SAME_MAX);
+    data->iov.iov_base = buf = qemu_blockalign(s->qdev.conf.bs, data->iov.iov_len);
+    qemu_iovec_init_external(&data->qiov, &data->iov, 1);
+
+    for (i = 0; i < data->iov.iov_len; i += s->qdev.blocksize) {
+        memcpy(&buf[i], inbuf, s->qdev.blocksize);
+    }
+
+    scsi_req_ref(&r->req);
+    bdrv_acct_start(s->qdev.conf.bs, &r->acct, data->iov.iov_len, BDRV_ACCT_WRITE);
+    r->req.aiocb = bdrv_aio_writev(s->qdev.conf.bs, data->sector,
+                                   &data->qiov, data->iov.iov_len / 512,
+                                   scsi_write_same_complete, data);
+}
+
 static void scsi_disk_emulate_write_data(SCSIRequest *req)
 {
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -1612,6 +1720,10 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
         scsi_disk_emulate_unmap(r, r->iov.iov_base);
         break;
 
+    case WRITE_SAME_10:
+    case WRITE_SAME_16:
+        scsi_disk_emulate_write_same(r, r->iov.iov_base);
+        break;
     default:
         abort();
     }
@@ -1854,30 +1966,10 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
         break;
     case WRITE_SAME_10:
     case WRITE_SAME_16:
-        nb_sectors = scsi_data_cdb_length(r->req.cmd.buf);
-        if (bdrv_is_read_only(s->qdev.conf.bs)) {
-            scsi_check_condition(r, SENSE_CODE(WRITE_PROTECTED));
-            return 0;
-        }
-        if (!check_lba_range(s, r->req.cmd.lba, nb_sectors)) {
-            goto illegal_lba;
-        }
-
-        /*
-         * We only support WRITE SAME with the unmap bit set for now.
-         * Reject UNMAP=0 or ANCHOR=1.
-         */
-        if (!(req->cmd.buf[1] & 0x8) || (req->cmd.buf[1] & 0x10)) {
-            goto illegal_request;
-        }
-
-        /* The request is used as the AIO opaque value, so add a ref.  */
-        scsi_req_ref(&r->req);
-        r->req.aiocb = bdrv_aio_discard(s->qdev.conf.bs,
-                                        r->req.cmd.lba * (s->qdev.blocksize / 512),
-                                        nb_sectors * (s->qdev.blocksize / 512),
-                                        scsi_aio_complete, r);
-        return 0;
+        DPRINTF("WRITE SAME %d (len %lu)\n",
+                req->cmd.buf[0] == WRITE_SAME_10 ? 10 : 16,
+                (long)r->req.cmd.xfer);
+        break;
     default:
         DPRINTF("Unknown SCSI command (%2.2x)\n", buf[0]);
         scsi_check_condition(r, SENSE_CODE(INVALID_OPCODE));
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 00/19] block & scsi: write_zeroes support through the whole stack

[Stefan Hajnoczi]

On Fri, Nov 22, 2013 at 01:39:42PM +0100, Paolo Bonzini wrote:
> This series fixes the implementation of WRITE SAME in the SCSI emulation
> layer.  On top of this, it ensures that zero writes from the guest can
> be offloaded to the host device or filesystem if that's supported.
> This is a relatively important part of the thin provisioning feature,
> and builds on the unmap flag to write_zeroes, recently added by Peter
> Lieven.  The feature works with iscsi, block device and filesystem
> targets.
> 
> v2->v3
>         Drop "block/iscsi: use UNMAP to write zeroes if LBPRZ=1" [Peter]
> 
>         Patches 3/4: change tracepoint flags to %#x [Stefan]
> 
>         Patch 6: clean up conditionals for misaligned num [Stefan]
>         Patch 6: fix freeing of bounce buffer [Peter]
> 
>         Patch 7: fix bdi->cluster_size for vhdx [Stefan]
> 
>         Patch 8: do not check bs->backing_hd to set
>         bdi->unallocated_blocks_are_zero [Peter]
> 
>         Patch 19: use qemu_blockalign/qemu_vfree [Stefan]
> 
> v1->v2
>         I cleaned up the series, putting all the block layer changes
>         together and all the SCSI changes at the end.
> 
>         qcow2 and iscsi are now better covered, which caused the
>         introduction of patches 6-11.  I also downloaded the latest
>         which contributed some changes in patches 10-11.  It turns
>         out we were too cautious in treating LBPRZ as advisory when
>         using the UNMAP command.  On the other hand, the API we
>         introduced is useful for qcow2/qed/vmdk WRITE SAME, so no
>         damage done.
> 
> 
> Paolo Bonzini (17):
>   block: generalize BlockLimits handling to cover bdrv_aio_discard too
>   block: add flags to BlockRequest
>   block: add flags argument to bdrv_co_write_zeroes tracepoint
>   block: add bdrv_aio_write_zeroes
>   block: handle ENOTSUP from discard in generic code
>   block: make bdrv_co_do_write_zeroes stricter in producing aligned
>     requests
>   vpc, vhdx: add get_info
>   block drivers: add discard/write_zeroes properties to bdrv_get_info
>     implementation
>   block drivers: expose requirement for write same alignment from
>     formats
>   block/iscsi: check WRITE SAME support differently depending on
>     MAY_UNMAP
>   raw-posix: implement write_zeroes with MAY_UNMAP for files
>   raw-posix: implement write_zeroes with MAY_UNMAP for block devices
>   raw-posix: add support for write_zeroes on XFS and block devices
>   qemu-iotests: 033 is fast
>   scsi-disk: catch write protection errors in UNMAP
>   scsi-disk: reject ANCHOR=1 for UNMAP and WRITE SAME commands
>   scsi-disk: correctly implement WRITE SAME
> 
> Peter Lieven (2):
>   block/iscsi: remove .bdrv_has_zero_init
>   block/iscsi: updated copyright
> 
>  block.c                  | 145 +++++++++++++++++++++++----------------
>  block/iscsi.c            |  27 +++++---
>  block/qcow2.c            |   3 +
>  block/qed.c              |   3 +
>  block/raw-aio.h          |   3 +-
>  block/raw-posix.c        | 175 ++++++++++++++++++++++++++++++++++++++++++++---
>  block/vdi.c              |   1 +
>  block/vhdx.c             |  13 ++++
>  block/vmdk.c             |   4 ++
>  block/vpc.c              |  15 ++++
>  hw/scsi/scsi-disk.c      | 154 ++++++++++++++++++++++++++++++++++-------
>  include/block/block.h    |   4 ++
>  tests/qemu-iotests/group |   2 +-
>  trace-events             |   4 +-
>  14 files changed, 452 insertions(+), 101 deletions(-)

Some of it is SCSI but I've applied the whole series.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan