Re: [PATCH v3 6/6] block/qcow2: use seqcache for compressed writes

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

12.03.2021 21:15, Max Reitz wrote:
> On 05.03.21 18:35, Vladimir Sementsov-Ogievskiy wrote:
>> Compressed writes are unaligned to 512, which works very slow in
>> O_DIRECT mode. Let's use the cache.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/coroutines.h     |   3 +
>>   block/qcow2.h          |   4 ++
>>   block/qcow2-refcount.c |  10 +++
>>   block/qcow2.c          | 158 ++++++++++++++++++++++++++++++++++++++---
>>   4 files changed, 164 insertions(+), 11 deletions(-)
>>

[..]

>> +static int coroutine_fn
>> +qcow2_co_compressed_flush_one(BlockDriverState *bs, bool unfinished)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +    int ret;
>> +    int64_t align = 1;
>> +    int64_t offset, bytes;
>> +    uint8_t *buf;
>> +
>> +    if (!s->compressed_cache) {
>> +        return 0;
>> +    }
>> +
>> +    if (!seqcache_get_next_flush(s->compressed_cache, &offset, &bytes, &buf,
>> +                                 &unfinished))
>> +    {
>> +        return 0;
>> +    }
>> +
>> +    qcow2_inflight_writes_inc(bs, offset, bytes);
>> +
>> +    /*
>> +     * Don't try to align-up unfinished cached cluster, parallel write to it is
>> +     * possible! For finished host clusters data in the tail of the cluster will
>> +     * be never used. So, take some good alignment for speed.
>> +     *
>> +     * Note also, that seqcache guarantees that allocated size of @buf is enough
>> +     * to fill the cluster up to the end, so we are safe to align up with
>> +     * align <= cluster_size.
>> +     */
>> +    if (!unfinished) {
>> +        align = MIN(s->cluster_size,
>> +                    MAX(s->data_file->bs->bl.request_alignment,
>> +                        bdrv_opt_mem_align(bs)));
>> +    }
>> +
> 
> I’d move the pre-write overlap check here, because its purpose is to prevent writes to metadata structures as they are happening, not as they are queued into a cache.  (I’d say.)

Hmm. pre-write overlap check usually done under mutex.. Should I add an additional critical section to do overlap check? I'm not sure.

> 
>> +    BLKDBG_EVENT(s->data_file, BLKDBG_WRITE_COMPRESSED);
>> +    ret = bdrv_co_pwrite(s->data_file, offset,
>> +                         QEMU_ALIGN_UP(offset + bytes, align) - offset,
> 
> I remember you said you wanted to describe more of the properties of the buffer returned by seqcache_get_next_flush().  That would be nice here, because intuitively one would assume that the buffer is @bytes bytes long, which aligning here will exceed.  (It’s fine, but it would be nicer if there was a comment that assured that it’s fine.)
> 

It's here, read several lines above: "Note also, that..."

>> +                         buf, 0);
>> +    if (ret < 0 && s->compressed_flush_ret == 0) {
>> +        /*
>> +         * The data that was "written" earlier is lost. We don't want to
>> +         * care with storing it somehow to retry flushing later.



-- 
Best regards,
Vladimir