Re: [Qemu-devel] [RFC V3 03/24] qcow2: Add qcow2_dedup_read_missing_and_concatenate

[Stefan Hajnoczi]

On Mon, Nov 26, 2012 at 02:05:02PM +0100, Benoît Canet wrote:
> +/**
> + * Read some data from the QCOW2 file
> + *
> + * @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

Dropping s->lock is important information to document - it means things
can change by the time this function returns and the caller needs to be
prepared.

Perhaps this function can be moved to qcow2.c and given a generic name.
It does nothing dedup-specific.

> + */
> +static int qcow2_dedup_read_missing_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);
> +    qemu_co_mutex_lock(&s->lock);
> +    if (ret < 0) {
> +        error_report("failed to read %d sectors at offset %" PRIu64 "\n",
> +                     nb_sectors, sector_num);
> +    }
> +
> +    return ret;
> +}
> +
> +/*
> + * Prepare a buffer containing all the required data required to compute 
> cluster
> + * sized deduplication hashes.
> + * If sector_num and nb_sectors are unaligned cluster wize it read the 
> missing
> + * data before and after the qiov.

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;
> +    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);
> +    memset(*data, 0, *nb_data_sectors * BDRV_SECTOR_SIZE);

Is memset necessary since we either read all data or return an error?

> +    /* read beginning */
> +    if (cluster_beginning_nr) {
> +        ret = qcow2_dedup_read_missing_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;
> +
> +    /* read and add ending */
> +    if (cluster_ending_nr) {
> +        ret = qcow2_dedup_read_missing_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:

Is it useful to leave the caller with a failed buffer still allocated?

qemu_vfree(*data);
*data = NULL;