From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: 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>,
"fam@euphon.net" <fam@euphon.net>,
Denis Lunev <den@virtuozzo.com>,
"jsnow@redhat.com" <jsnow@redhat.com>,
"armbru@redhat.com" <armbru@redhat.com>,
"dgilbert@redhat.com" <dgilbert@redhat.com>,
"stefanha@redhat.com" <stefanha@redhat.com>,
"mreitz@redhat.com" <mreitz@redhat.com>
Subject: Re: [PATCH v2 1/6] qcow2: Allow writing compressed data to multiple clusters
Date: Thu, 3 Oct 2019 14:16:39 +0000 [thread overview]
Message-ID: <e8b7e627-6b0a-9662-5f9a-6afe76a2c2e4@virtuozzo.com> (raw)
In-Reply-To: <1570026166-748566-2-git-send-email-andrey.shinkevich@virtuozzo.com>
02.10.2019 17:22, Andrey Shinkevich wrote:
> QEMU currently supports writing compressed data of size less than or
> equal to one cluster. This patch allows writing QCOW2 compressed data
> that exceed one cluster. The implementation is simple, we split buffered
> data into separate clusters and write them using the existing
> functionality. For unaligned requests, we use a workaround that writes
> the data without compression.
>
> Suggested-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> block/qcow2.c | 113 +++++++++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 85 insertions(+), 28 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7961c05..54ccaf6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4152,10 +4152,8 @@ fail:
> return ret;
> }
>
> -/* XXX: put compressed sectors first, then all the cluster aligned
> - tables to avoid losing bytes in alignment */
> static coroutine_fn int
> -qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
> +qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
> uint64_t offset, uint64_t bytes,
> QEMUIOVector *qiov, size_t qiov_offset)
> {
> @@ -4165,36 +4163,14 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
> uint8_t *buf, *out_buf;
> uint64_t cluster_offset;
>
> - 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, PREALLOC_MODE_OFF, NULL);
> - }
> -
> - if (offset_into_cluster(s, offset)) {
> - return -EINVAL;
> - }
> + assert(bytes <= s->cluster_size);
I think we'd better assert
assert(bytes == s->cluster_size || (bytes < s->cluster_size && (offset + bytes == bdrv_getlength())
to match old logic.jkjkj
>
> buf = qemu_blockalign(bs, s->cluster_size);
> - if (bytes != s->cluster_size) {
> - if (bytes > s->cluster_size ||
> - offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS)
> - {
> - qemu_vfree(buf);
> - return -EINVAL;
> - }
> + if (bytes < s->cluster_size) {
> /* Zero-pad last write if image size is not cluster aligned */
> memset(buf + bytes, 0, s->cluster_size - bytes);
> }
> - qemu_iovec_to_buf(qiov, qiov_offset, buf, bytes);
> + qemu_iovec_to_buf(qiov, qiov_offset, buf, s->cluster_size);
Why did you changed it to s->cluster_size? bytes may be less than cluster
>
> out_buf = g_malloc(s->cluster_size);
>
> @@ -4228,6 +4204,9 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
>
> BLKDBG_EVENT(s->data_file, BLKDBG_WRITE_COMPRESSED);
> ret = bdrv_co_pwrite(s->data_file, cluster_offset, out_len, out_buf, 0);
> + if (ret == -ENOTSUP) {
> + ret = qcow2_co_pwritev_part(bs, offset, bytes, qiov, qiov_offset, 0);
> + }
It should not be here:
1. write will unlikely return ENOTSUP
2. if you mean fallback to normal write if we failed to compress, it's done above already.
> if (ret < 0) {
> goto fail;
> }
> @@ -4239,6 +4218,84 @@ fail:
> return ret;
> }
>
> +static coroutine_fn int qcow2_co_pwritev_compressed_task_entry(AioTask *task)
> +{
> + Qcow2AioTask *t = container_of(task, Qcow2AioTask, task);
> +
> + assert(!t->cluster_type);
and assert(!t->l2meta)
> +
> + return qcow2_co_pwritev_compressed_task(t->bs, t->offset, t->bytes, t->qiov,
> + t->qiov_offset);
> +}
> +
> +/*
> + * XXX: put compressed sectors first, then all the cluster aligned
> + tables to avoid losing bytes in alignment
> + */
> +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;
> + QCowL2Meta *l2meta = NULL;
it's always NULL and actually unused.
> + AioTaskPool *aio = NULL;
> + uint64_t curr_off = 0;
> + int ret;
> +
> + 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 cluster_offset = bdrv_getlength(bs->file->bs);
Hmm, I like old variable name 'len' more.
> + if (cluster_offset < 0) {
> + return cluster_offset;
> + }
> + return bdrv_co_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF,
> + NULL);
> + }
> +
> + if (offset_into_cluster(s, offset)) {
> + return -EINVAL;
> + }
> +
> + while (bytes && aio_task_pool_status(aio) == 0) {
> + uint32_t chunk_size = MIN(bytes, s->cluster_size);
> +
> + assert((curr_off & (BDRV_SECTOR_SIZE - 1)) == 0);
> + assert((chunk_size & (BDRV_SECTOR_SIZE - 1)) == 0);
first: always use QEMU_IS_ALIGNED instead
second: why do you add these asserts, do we need them?
> +
> + 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 + curr_off, chunk_size,
> + qiov, qiov_offset, l2meta);
> + if (ret < 0) {
> + break;
> + }
> + qiov_offset += chunk_size;
> + curr_off += chunk_size;
you may just offset += chunk_size and drop extra variable
> + bytes -= chunk_size;
> + }
> +
> + if (aio) {
> + aio_task_pool_wait_all(aio);
> + if (ret == 0) {
> + ret = aio_task_pool_status(aio);
> + }
> + g_free(aio);
> + }
> +
> + return ret;
> +}
> +
> static int coroutine_fn
> qcow2_co_preadv_compressed(BlockDriverState *bs,
> uint64_t file_cluster_offset,
>
overall - OK
--
Best regards,
Vladimir
next prev parent reply other threads:[~2019-10-03 14:17 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-02 14:22 [PATCH v2 0/6] qcow2: advanced compression options Andrey Shinkevich
2019-10-02 14:22 ` [PATCH v2 1/6] qcow2: Allow writing compressed data to multiple clusters Andrey Shinkevich
2019-10-03 14:16 ` Vladimir Sementsov-Ogievskiy [this message]
2019-10-02 14:22 ` [PATCH v2 2/6] tests/qemu-iotests: add case of " Andrey Shinkevich
2019-10-03 14:21 ` Vladimir Sementsov-Ogievskiy
2019-10-02 14:22 ` [PATCH v2 3/6] qemu-nbd: add compression flag support Andrey Shinkevich
2019-10-03 14:32 ` Vladimir Sementsov-Ogievskiy
2019-10-04 10:19 ` Roman Kagan
2019-10-07 11:52 ` Vladimir Sementsov-Ogievskiy
2019-10-02 14:22 ` [PATCH v2 4/6] block: support compressed write for copy-on-read Andrey Shinkevich
2019-10-03 14:37 ` Vladimir Sementsov-Ogievskiy
2019-10-02 14:22 ` [PATCH v2 5/6] block-stream: add compress option Andrey Shinkevich
2019-10-03 14:48 ` Vladimir Sementsov-Ogievskiy
2019-10-02 14:22 ` [PATCH v2 6/6] tests/qemu-iotests: add case for block-stream compress Andrey Shinkevich
2019-10-03 14:58 ` Vladimir Sementsov-Ogievskiy
2019-10-15 17:57 ` Andrey Shinkevich
2019-10-02 15:07 ` [PATCH v2 0/6] qcow2: advanced compression options no-reply
2019-10-02 15:35 ` Vladimir Sementsov-Ogievskiy
2019-10-02 15:58 ` Max Reitz
2019-10-02 19:04 ` Markus Armbruster
2019-10-02 15:07 ` no-reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e8b7e627-6b0a-9662-5f9a-6afe76a2c2e4@virtuozzo.com \
--to=vsementsov@virtuozzo.com \
--cc=andrey.shinkevich@virtuozzo.com \
--cc=armbru@redhat.com \
--cc=den@virtuozzo.com \
--cc=dgilbert@redhat.com \
--cc=fam@euphon.net \
--cc=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).