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

[Vladimir Sementsov-Ogievskiy]

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