Re: [PATCH v12 2/3] qcow2: Allow writing compressed data of multiple clusters

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

10.04.2020 3:12, Andrey Shinkevich wrote:
> We could assign indices to the clusters/chunks and improve the algorithm to write them down on the disk in the same order adjacently. If you find it feasible for QEMU, I'd like to create a task for doing that, shall I?
> 

Compressed cluster occupy different size chunks in the image. How are you going to preallocate? Anyway, I don't see any benefit in ordering compressed clusters, I think it's not worth doing.

> 
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> *From:* Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> *Sent:* Thursday, April 9, 2020 9:39 PM
> *To:* Alberto Garcia <berto@igalia.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>; qemu-devel@nongnu.org <qemu-devel@nongnu.org>; qemu-block@nongnu.org <qemu-block@nongnu.org>
> *Cc:* kwolf@redhat.com <kwolf@redhat.com>; armbru@redhat.com <armbru@redhat.com>; mreitz@redhat.com <mreitz@redhat.com>; Denis Lunev <den@virtuozzo.com>
> *Subject:* Re: [PATCH v12 2/3] qcow2: Allow writing compressed data of multiple clusters
> 09.04.2020 19:50, Alberto Garcia wrote:
>> On Mon 02 Dec 2019 01:15:05 PM CET, Andrey Shinkevich wrote:
>>> +static coroutine_fn int
>>> +qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
>>> +                                 uint64_t offset, uint64_t bytes,
>>> +                                 QEMUIOVector *qiov, size_t qiov_offset)
>>> +{
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +    AioTaskPool *aio = NULL;
>>> +    int ret = 0;
>>> +
>>> +    if (has_data_file(bs)) {
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>> +    if (bytes == 0) {
>>> +        /*
>>> +         * align end of file to a sector boundary to ease reading with
>>> +         * sector based I/Os
>>> +         */
>>> +        int64_t len = bdrv_getlength(bs->file->bs);
>>> +        if (len < 0) {
>>> +            return len;
>>> +        }
>>> +        return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, NULL);
>>> +    }
>>> +
>>> +    if (offset_into_cluster(s, offset)) {
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    while (bytes && aio_task_pool_status(aio) == 0) {
>>> +        uint64_t chunk_size = MIN(bytes, s->cluster_size);
>>> +
>>> +        if (!aio && chunk_size != bytes) {
>>> +            aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
>>> +        }
>>> +
>>> +        ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_compressed_task_entry,
>>> +                             0, 0, offset, chunk_size, qiov, qiov_offset, NULL);
>>> +        if (ret < 0) {
>>> +            break;
>>> +        }
>>> +        qiov_offset += chunk_size;
>>> +        offset += chunk_size;
>>> +        bytes -= chunk_size;
>>> +    }
>> 
>> This patch allows the user to write more than one cluster of compressed
>> data at a time, and it does so by splitting the request into many
>> cluster-sized requests and using qcow2_add_task() for each one of them.
>> 
>> What happens however is that there's no guarantee that the requests are
>> processed in the same order that they were added.
>> 
>> One consequence is that running on an empty qcow2 file a command as
>> simple as this one:
>> 
>>     qemu-io -c 'write -c 0 256k' image.qcow2
>> 
>> does not always produce the same results.
>> 
>> This does not have any user-visible consequences for the guest. In all
>> cases the data is correctly written, it's just that the ordering of the
>> compressed clusters (and therefore the contents of the L2 entries) will
>> be different each time.
>> 
>> Because of this a test cannot expect that running the same commands on
>> an empty image produces always the same results.
>> 
>> Is this something that we should be concerned about?
>> 
> 
> Parallel writing compressed clusters is significant improvement, as it allow compressing in really parallel threads.
> 
> Generally, async parallel issuing of several requests gives more performance than handling peaces one-by-one, mirror works on this basis and it is fast. I've already moved qcow2 to this idea (aio tasks in qcow2 code), and in progress of moving backup job. So, I think that asynchrony and ambiguity would be native for block-layer anyway.
> 
> Hmm. Still, what about cluster sequence? For normal clusters there may be simple thing to do: preallocation (at least of metadata). So, we can pre-create cluster sequence.. But what to do with compressed clusters if we want specific order for them, I don't know. On the other hand, ordering of normal cluster may make sence: it should increase performnace of following IO. But for compressed clusters it's not the case.
> 
> So, I don't think we should make specific workaround for testing... What exactly is the case?
> 
> -- 
> Best regards,
> Vladimir


-- 
Best regards,
Vladimir