Re: [Qemu-devel] [PATCH 5/5] qcow2: Implement .bdrv_co_pwritev

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5222 bytes --]

On 06/03/2016 11:21 AM, Kevin Wolf wrote:
> This changes qcow2 to implement the byte-based .bdrv_co_pwritev
> interface rather than the sector-based old one.
> 
> As preallocation uses the same allocation function as normal writes, and
> the interface of that function needs to be changed, it is converted in
> the same patch.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2-cluster.c | 13 ++++----
>  block/qcow2.c         | 82 ++++++++++++++++++++++++---------------------------
>  block/qcow2.h         |  3 +-
>  trace-events          |  6 ++--
>  4 files changed, 49 insertions(+), 55 deletions(-)
> 

> +++ b/block/qcow2-cluster.c
> @@ -1261,7 +1261,8 @@ fail:
>   * Return 0 on success and -errno in error cases
>   */
>  int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> -    int *num, uint64_t *host_offset, QCowL2Meta **m)
> +                               unsigned int *bytes, uint64_t *host_offset,
> +                               QCowL2Meta **m)
>  {

>  
> -    *num -= remaining >> BDRV_SECTOR_BITS;
> -    assert(*num > 0);
> +    *bytes -= remaining;
> +    assert(*bytes > 0);

The assertion is now weaker.  Beforehand, num could go negative.  But
bytes cannot, all it can do is go to 0.  If we wrap around, the
assertion won't catch it.  Do you want to strengthen it by also
asserting that bytes < INT_MAX?

> +++ b/block/qcow2.c
> @@ -1544,23 +1544,20 @@ fail:
>      return ret;
>  }
>  
> -static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
> -                           int64_t sector_num,
> -                           int remaining_sectors,
> -                           QEMUIOVector *qiov)
> +static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs,
> +        uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)

Worth consistent indentation, while touching it?


> +    while (bytes != 0) {
>  
>          l2meta = NULL;
>  
>          trace_qcow2_writev_start_part(qemu_coroutine_self());
> -        index_in_cluster = sector_num & (s->cluster_sectors - 1);
> -        cur_nr_sectors = remaining_sectors;
> -        if (bs->encrypted &&
> -            cur_nr_sectors >
> -            QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster) {
> -            cur_nr_sectors =
> -                QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster;
> +        offset_in_cluster = offset_into_cluster(s, offset);
> +        cur_bytes = MIN(bytes, INT_MAX);
> +        if (bs->encrypted) {
> +            cur_bytes = MIN(cur_bytes,
> +                            QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size
> +                            - offset_in_cluster);

Again, if the block layer learns to auto-fragment, then we should just
inform the block layer about QCOW_MAX_CRYPT_CLUSTERS as our max_transfer
limit.


> @@ -1634,10 +1628,10 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
>          qemu_co_mutex_unlock(&s->lock);
>          BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
>          trace_qcow2_writev_data(qemu_coroutine_self(),
> -                                (cluster_offset >> 9) + index_in_cluster);
> -        ret = bdrv_co_writev(bs->file->bs,
> -                             (cluster_offset >> 9) + index_in_cluster,
> -                             cur_nr_sectors, &hd_qiov);
> +                                cluster_offset + offset_in_cluster);
> +        ret = bdrv_co_pwritev(bs->file->bs,
> +                              cluster_offset + offset_in_cluster,
> +                              cur_bytes, &hd_qiov, 0);

s/0/flags/, even if .supported_write_flags stays 0 for now.

> @@ -2008,19 +2002,19 @@ static int qcow2_change_backing_file(BlockDriverState *bs,
>  
>  static int preallocate(BlockDriverState *bs)
>  {
> -    uint64_t nb_sectors;
> +    uint64_t bytes;
>      uint64_t offset;
>      uint64_t host_offset = 0;
> -    int num;
> +    unsigned int cur_bytes;
>      int ret;
>      QCowL2Meta *meta;
>  
> -    nb_sectors = bdrv_nb_sectors(bs);
> +    bytes = bdrv_getlength(bs);
>      offset = 0;
>  
> -    while (nb_sectors) {
> -        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);
> -        ret = qcow2_alloc_cluster_offset(bs, offset, &num,
> +    while (bytes) {
> +        cur_bytes = MIN(bytes, INT_MAX);

Okay, while bdrv_co_pwritev() should (eventually) let the block layer
auto-fragment, here, you are right that you have to fragment yourself
since this is not being called from the block layer.


> @@ -2059,7 +2053,7 @@ static int preallocate(BlockDriverState *bs)
>          uint8_t buf[BDRV_SECTOR_SIZE];
>          memset(buf, 0, BDRV_SECTOR_SIZE);
>          ret = bdrv_write(bs->file->bs,
> -                         (host_offset >> BDRV_SECTOR_BITS) + num - 1,
> +                         ((host_offset + cur_bytes) >> BDRV_SECTOR_BITS) - 1,
>                           buf, 1);

Do we still have to call bdrv_write(), or can we convert this to a
byte-based interface call?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]