[v5,3/8] mirror: Do zero write on target if sectors not allocated

If guest discards a source cluster, mirroring with bdrv_aio_readv is overkill.
Some protocols do zero upon discard, where it's best to use
bdrv_aio_write_zeroes, otherwise, bdrv_aio_discard will be enough.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Am 26.05.2015 um 05:36 schrieb Fam Zheng:
> If guest discards a source cluster, mirroring with bdrv_aio_readv is overkill.
> Some protocols do zero upon discard, where it's best to use
> bdrv_aio_write_zeroes, otherwise, bdrv_aio_discard will be enough.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/mirror.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 85995b2..ba33254 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -164,6 +164,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>       int64_t end, sector_num, next_chunk, next_sector, hbitmap_next_sector;
>       uint64_t delay_ns = 0;
>       MirrorOp *op;
> +    int pnum;
> +    int64_t ret;
>   
>       s->sector_num = hbitmap_iter_next(&s->hbi);
>       if (s->sector_num < 0) {
> @@ -290,8 +292,22 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>       s->in_flight++;
>       s->sectors_in_flight += nb_sectors;
>       trace_mirror_one_iteration(s, sector_num, nb_sectors);
> -    bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> -                   mirror_read_complete, op);
> +
> +    ret = bdrv_get_block_status_above(source, NULL, sector_num,
> +                                      nb_sectors, &pnum);
> +    if (ret < 0 || pnum < nb_sectors ||
> +            (ret & BDRV_BLOCK_DATA && !(ret & BDRV_BLOCK_ZERO))) {
> +        bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> +                       mirror_read_complete, op);
> +    } else if (ret & BDRV_BLOCK_ZERO) {
> +        bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
> +                              s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
> +                              mirror_write_complete, op);
> +    } else {
> +        assert(!(ret & BDRV_BLOCK_DATA));
> +        bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
> +                         mirror_write_complete, op);
> +    }

I wonder what happens if on the destination the discard is a NOP which is legal (at least in SCSI).
In this case we might end up having different contents and source and destination. Or is this
not a problem?

Peter

On Tue, 05/26 07:53, Peter Lieven wrote:
> Am 26.05.2015 um 05:36 schrieb Fam Zheng:
> >If guest discards a source cluster, mirroring with bdrv_aio_readv is overkill.
> >Some protocols do zero upon discard, where it's best to use
> >bdrv_aio_write_zeroes, otherwise, bdrv_aio_discard will be enough.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  block/mirror.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> >
> >diff --git a/block/mirror.c b/block/mirror.c
> >index 85995b2..ba33254 100644
> >--- a/block/mirror.c
> >+++ b/block/mirror.c
> >@@ -164,6 +164,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> >      int64_t end, sector_num, next_chunk, next_sector, hbitmap_next_sector;
> >      uint64_t delay_ns = 0;
> >      MirrorOp *op;
> >+    int pnum;
> >+    int64_t ret;
> >      s->sector_num = hbitmap_iter_next(&s->hbi);
> >      if (s->sector_num < 0) {
> >@@ -290,8 +292,22 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> >      s->in_flight++;
> >      s->sectors_in_flight += nb_sectors;
> >      trace_mirror_one_iteration(s, sector_num, nb_sectors);
> >-    bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> >-                   mirror_read_complete, op);
> >+
> >+    ret = bdrv_get_block_status_above(source, NULL, sector_num,
> >+                                      nb_sectors, &pnum);
> >+    if (ret < 0 || pnum < nb_sectors ||
> >+            (ret & BDRV_BLOCK_DATA && !(ret & BDRV_BLOCK_ZERO))) {
> >+        bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> >+                       mirror_read_complete, op);
> >+    } else if (ret & BDRV_BLOCK_ZERO) {
> >+        bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
> >+                              s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
> >+                              mirror_write_complete, op);
> >+    } else {
> >+        assert(!(ret & BDRV_BLOCK_DATA));
> >+        bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
> >+                         mirror_write_complete, op);
> >+    }
> 
> I wonder what happens if on the destination the discard is a NOP which is legal (at least in SCSI).
> In this case we might end up having different contents and source and destination. Or is this
> not a problem?

This is not a problem, because the guest already doesn't care about the
content.

Fam