Re: [Qemu-devel] [PATCH v7 04/16] qcow2: add qcow2_co_pwritev_compressed

From: Kevin Wolf <kwolf@redhat.com>
To: "Denis V. Lunev" <den@openvz.org>
Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org,
	Pavel Butsykin <pbutsykin@virtuozzo.com>,
	Jeff Cody <jcody@redhat.com>,
	Markus Armbruster <armbru@redhat.com>,
	Eric Blake <eblake@redhat.com>, John Snow <jsnow@redhat.com>,
	Stefan Hajnoczi <stefanha@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v7 04/16] qcow2: add qcow2_co_pwritev_compressed
Date: Mon, 8 Aug 2016 15:44:00 +0200	[thread overview]
Message-ID: <20160808134400.GC25992@noname.str.redhat.com> (raw)
In-Reply-To: <1469175475-15420-5-git-send-email-den@openvz.org>

Am 22.07.2016 um 10:17 hat Denis V. Lunev geschrieben:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> Added implementation of the qcow2_co_pwritev_compressed function that
> will allow us to safely use compressed writes for the qcow2 from running
> VMs.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>

>  /* XXX: put compressed sectors first, then all the cluster aligned
>     tables to avoid losing bytes in alignment */
> -static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
> -                                  const uint8_t *buf, int nb_sectors)
> +static coroutine_fn int
> +qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
> +                            uint64_t bytes, QEMUIOVector *qiov)
>  {
>      BDRVQcow2State *s = bs->opaque;
> +    QEMUIOVector hd_qiov;
> +    struct iovec iov;
>      z_stream strm;
>      int ret, out_len;
> -    uint8_t *out_buf;
> +    uint8_t *buf, *out_buf;
>      uint64_t cluster_offset;
>  
> -    if (nb_sectors == 0) {
> +    if (bytes == 0) {
>          /* align end of file to a sector boundary to ease reading with
>             sector based I/Os */
>          cluster_offset = bdrv_getlength(bs->file->bs);
>          return bdrv_truncate(bs->file->bs, cluster_offset);
>      }
>  
> -    if (nb_sectors != s->cluster_sectors) {
> +    if (bytes != s->cluster_size) {
>          ret = -EINVAL;
>  
>          /* Zero-pad last write if image size is not cluster aligned */
> -        if (sector_num + nb_sectors == bs->total_sectors &&
> -            nb_sectors < s->cluster_sectors) {
> +        if (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS &&
> +            bytes < s->cluster_size)
> +        {
>              uint8_t *pad_buf = qemu_blockalign(bs, s->cluster_size);
>              memset(pad_buf, 0, s->cluster_size);
> -            memcpy(pad_buf, buf, nb_sectors * BDRV_SECTOR_SIZE);
> -            ret = qcow2_write_compressed(bs, sector_num,
> -                                         pad_buf, s->cluster_sectors);
> +            qemu_iovec_to_buf(qiov, 0, pad_buf, s->cluster_size);

I think, bytes would be more exact than s->cluster_size here. Anyway,
shouldn't hurt because we stop at the end of the qiov anyway and its
size should match bytes.

> +            iov = (struct iovec) {
> +                .iov_base   = pad_buf,
> +                .iov_len    = s->cluster_size,
> +            };
> +            qemu_iovec_init_external(&hd_qiov, &iov, 1);
> +            ret = qcow2_co_pwritev_compressed(bs, offset, bytes, &hd_qiov);
>              qemu_vfree(pad_buf);
>          }
>          return ret;
>      }

Kevin