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

[Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>]

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

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?

Andrey

________________________________
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

[-- Attachment #2: Type: text/html, Size: 7955 bytes --]