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 <vsement...@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
