Re: [Qemu-devel] [RFC V6 04/33] qcow2: Add qcow2_dedup_read_missing_and_concatenate

[Stefan Hajnoczi]

On Wed, Feb 06, 2013 at 01:31:37PM +0100, Benoît Canet wrote:
> +/*
> + * Prepare a buffer containing all the required data required to compute 
> cluster

Easier to read this way:
s/all the required data required/everything required/

> + * sized deduplication hashes.
> + * If sector_num or nb_sectors are not cluster-aligned, missing data
> + * before/after the qiov will be read.
> + *
> + * @qiov:               the qiov for which missing data must be read
> + * @sector_num:         the first sectors that must be read into the qiov
> + * @nb_sectors:         the number of sectors to read into the qiov
> + * @data:               the place where the data will be concatenated and 
> stored
> + * @nb_data_sectors:    the resulting size of the contatenated data (in 
> sectors)
> + * @ret:                negative on error
> + */
> +int qcow2_dedup_read_missing_and_concatenate(BlockDriverState *bs,
> +                                             QEMUIOVector *qiov,
> +                                             uint64_t sector_num,
> +                                             int nb_sectors,
> +                                             uint8_t **data,
> +                                             int *nb_data_sectors)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int ret = 0;
> +    uint64_t cluster_beginning_sector;
> +    uint64_t first_sector_after_qiov;
> +    int cluster_beginning_nr;
> +    int cluster_ending_nr;
> +    int unaligned_ending_nr;
> +    uint64_t max_cluster_ending_nr;
> +
> +    /* compute how much and where to read at the beginning */
> +    cluster_beginning_nr = sector_num & (s->cluster_sectors - 1);
> +    cluster_beginning_sector = sector_num - cluster_beginning_nr;
> +
> +    /* for the ending */
> +    first_sector_after_qiov = sector_num + nb_sectors;
> +    unaligned_ending_nr = first_sector_after_qiov & (s->cluster_sectors - 1);
> +    cluster_ending_nr = unaligned_ending_nr ?
> +                        s->cluster_sectors - unaligned_ending_nr : 0;
> +
> +    /* compute total size in sectors and allocate memory */
> +    *nb_data_sectors = cluster_beginning_nr + nb_sectors + cluster_ending_nr;
> +    *data = qemu_blockalign(bs, *nb_data_sectors * BDRV_SECTOR_SIZE);
> +
> +    /* read beginning */
> +    if (cluster_beginning_nr) {
> +        ret = qcow2_read_cluster_data(bs,
> +                                      *data,
> +                                      cluster_beginning_sector,
> +                                      cluster_beginning_nr);
> +    }
> +
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    /* append qiov content */
> +    qemu_iovec_to_buf(qiov, 0, *data + cluster_beginning_nr * 
> BDRV_SECTOR_SIZE,
> +                      qiov->size);
> +
> +    /* Fix cluster_ending_nr if we are at risk of reading outside the image
> +     * (Cluster unaligned image size)
> +     */
> +    max_cluster_ending_nr = bs->total_sectors - first_sector_after_qiov;
> +    cluster_ending_nr = max_cluster_ending_nr < (uint64_t) cluster_ending_nr 
> ?
> +                        (int) max_cluster_ending_nr : cluster_ending_nr;

Is there a test case for the cluster unaligned image size scenario?

> +
> +    /* read and add ending */
> +    if (cluster_ending_nr) {
> +        ret = qcow2_read_cluster_data(bs,
> +                                      *data +
> +                                      (cluster_beginning_nr +
> +                                      nb_sectors) *
> +                                      BDRV_SECTOR_SIZE,
> +                                      first_sector_after_qiov,
> +                                      cluster_ending_nr);
> +    }
> +
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    return 0;
> +
> +fail:
> +    qemu_vfree(*data);
> +    *data = NULL;
> +    return ret;
> +}
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7610e56..ecbe352 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1110,6 +1110,41 @@ fail:
>      return ret;
>  }
>  
> +/**
> + * Read some data from the QCOW2 file
> + *
> + * Important: s->lock is dropped. Things can change before the function 
> returns
> + *            to the caller.
> + *
> + * @data:       the buffer where the data must be stored
> + * @sector_num: the sector number to read in the QCOW2 file
> + * @nb_sectors: the number of sectors to read
> + * @ret:        negative on error
> + */
> +int qcow2_read_cluster_data(BlockDriverState *bs,
> +                            uint8_t *data,
> +                            uint64_t sector_num,
> +                            int nb_sectors)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    QEMUIOVector qiov;
> +    struct iovec iov;
> +    int ret;
> +
> +    iov.iov_len = nb_sectors * BDRV_SECTOR_SIZE;
> +    iov.iov_base = data;
> +    qemu_iovec_init_external(&qiov, &iov, 1);
> +    qemu_co_mutex_unlock(&s->lock);
> +    ret = bdrv_co_readv(bs, sector_num, nb_sectors, &qiov);

This function should be marked coroutine_fn - it may only be called from
inside a coroutine.  It's good to mark all coroutine functions so the
reader knows immediately this will run in coroutine context.

bdrv_co_readv() is does I/O throttling.  This is wrong here since we
don't want to charge for internal I/O.