Re: [PATCH v3 2/2] quorum: Implement bdrv_co_pwrite_zeroes()

[Max Reitz]

On 11.11.20 17:53, Alberto Garcia wrote:
> This simply calls bdrv_co_pwrite_zeroes() in all children
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/quorum.c             | 18 ++++++++++++++++--
>   tests/qemu-iotests/312     |  7 +++++++
>   tests/qemu-iotests/312.out |  4 ++++
>   3 files changed, 27 insertions(+), 2 deletions(-)

Should we set supported_zero_flags to something?  I think we can at 
least set BDRV_REQ_NO_FALLBACK.  We could also try BDRV_REQ_FUA.

> diff --git a/block/quorum.c b/block/quorum.c
> index 9691a9bee9..c81572f513 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -692,8 +692,13 @@ static void write_quorum_entry(void *opaque)
>       QuorumChildRequest *sacb = &acb->qcrs[i];
>   
>       sacb->bs = s->children[i]->bs;
> -    sacb->ret = bdrv_co_pwritev(s->children[i], acb->offset, acb->bytes,
> -                                acb->qiov, acb->flags);
> +    if (acb->flags & BDRV_REQ_ZERO_WRITE) {
> +        sacb->ret = bdrv_co_pwrite_zeroes(s->children[i], acb->offset,
> +                                          acb->bytes, acb->flags);
> +    } else {
> +        sacb->ret = bdrv_co_pwritev(s->children[i], acb->offset, acb->bytes,
> +                                    acb->qiov, acb->flags);
> +    }

Seems unnecessary (bdrv_co_pwritev() can handle BDRV_REQ_ZERO_WRITE), 
but perhaps it’s good to be explicit.

>       if (sacb->ret == 0) {
>           acb->success_count++;
>       } else {

[...]

> diff --git a/tests/qemu-iotests/312 b/tests/qemu-iotests/312
> index 1b08f1552f..93046393e7 100755
> --- a/tests/qemu-iotests/312
> +++ b/tests/qemu-iotests/312
> @@ -114,6 +114,13 @@ $QEMU_IO -c "write -P 0 $((0x200000)) $((0x10000))" 
> "$TEST_IMG.0" | _filter_qemu
>   $QEMU_IO -c "write -z   $((0x200000)) $((0x30000))" "$TEST_IMG.1" | 
> _filter_qemu_io
>   $QEMU_IO -c "write -P 0 $((0x200000)) $((0x20000))" "$TEST_IMG.2" | 
> _filter_qemu_io
>   
> +# Test 5: write data to a region and then zeroize it, doing it
> +# directly on the quorum device instead of the individual images.
> +# This has no effect on the end result but proves that the quorum driver
> +# supports 'write -z'.
> +$QEMU_IO -c "open -o $quorum" -c "write $((0x250000)) $((0x10000))" | 
> _filter_qemu_io
> +$QEMU_IO -c "open -o $quorum" -c "write -z $((0x250000)) $((0x10000))" | 
> _filter_qemu_io
> +

My gut would have preferred a test where the data region is larger than 
the zeroed region (so we can see that the first write has done 
something), but who cares about my gut.

I don’t mind not setting supported_zero_flags enough to warrant 
withholding a

Reviewed-by: Max Reitz <mreitz@redhat.com>

But I’ll give you some time to reply before I’d take this patch to 
block-next.  (That is, unless Kevin takes it during my two-week PTO...)

>   echo
>   echo '### Launch the drive-mirror job'
>   echo