* [PATCH v2 0/6] qcow2: advanced compression options
@ 2019-10-02 14:22 Andrey Shinkevich
2019-10-02 14:22 ` [PATCH v2 1/6] qcow2: Allow writing compressed data to multiple clusters Andrey Shinkevich
` (7 more replies)
0 siblings, 8 replies; 21+ messages in thread
From: Andrey Shinkevich @ 2019-10-02 14:22 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: kwolf, fam, vsementsov, jsnow, armbru, dgilbert, stefanha,
andrey.shinkevich, den, mreitz
New enhancements for writing compressed data to QCOW2 image.
The preceding patches have been queued in the Max's block branch:
Based-on: https://github.com/XanClic/qemu.git block
v2:
The number of parameters in nbd_export_new() has been reduced by
the introduced flags for all the boolean ones (suggested by Eric).
Andrey Shinkevich (6):
qcow2: Allow writing compressed data to multiple clusters
tests/qemu-iotests: add case of writing compressed data to multiple
clusters
qemu-nbd: add compression flag support
block: support compressed write for copy-on-read
block-stream: add compress option
tests/qemu-iotests: add case for block-stream compress
block/io.c | 21 +++++++--
block/qcow2.c | 113 ++++++++++++++++++++++++++++++++++-----------
block/stream.c | 19 +++++---
block/trace-events | 2 +-
blockdev-nbd.c | 8 +++-
blockdev.c | 14 +++++-
hmp-commands.hx | 4 +-
include/block/block_int.h | 3 +-
include/block/nbd.h | 11 ++++-
monitor/hmp-cmds.c | 5 +-
nbd/server.c | 14 ++++--
qapi/block-core.json | 5 +-
qemu-nbd.c | 30 ++++++++++--
qemu-nbd.texi | 2 +
tests/qemu-iotests/030 | 49 +++++++++++++++++++-
tests/qemu-iotests/030.out | 4 +-
tests/qemu-iotests/214 | 9 ++++
tests/qemu-iotests/214.out | 6 +++
18 files changed, 257 insertions(+), 62 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 1/6] qcow2: Allow writing compressed data to multiple clusters
2019-10-02 14:22 [PATCH v2 0/6] qcow2: advanced compression options Andrey Shinkevich
@ 2019-10-02 14:22 ` Andrey Shinkevich
2019-10-03 14:16 ` Vladimir Sementsov-Ogievskiy
2019-10-02 14:22 ` [PATCH v2 2/6] tests/qemu-iotests: add case of " Andrey Shinkevich
` (6 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Andrey Shinkevich @ 2019-10-02 14:22 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: kwolf, fam, vsementsov, jsnow, armbru, dgilbert, stefanha,
andrey.shinkevich, den, mreitz
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);
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);
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);
+ }
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);
+
+ 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;
+ 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);
+ 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);
+
+ 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;
+ 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,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 2/6] tests/qemu-iotests: add case of writing compressed data to multiple clusters
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-02 14:22 ` 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
` (5 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Andrey Shinkevich @ 2019-10-02 14:22 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: kwolf, fam, vsementsov, jsnow, armbru, dgilbert, stefanha,
andrey.shinkevich, den, mreitz
Add the test case to the iotest #214 that checks possibility of writing
compressed data to more than one cluster.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
tests/qemu-iotests/214 | 9 +++++++++
tests/qemu-iotests/214.out | 6 ++++++
2 files changed, 15 insertions(+)
diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
index 21ec8a2..5f437e4 100755
--- a/tests/qemu-iotests/214
+++ b/tests/qemu-iotests/214
@@ -89,6 +89,15 @@ _check_test_img -r all
$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
$QEMU_IO -c "read -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
+echo
+echo "=== Write compressed data to multiple clusters ==="
+echo
+# Create an empty image and fill three and half clusters with compressed data.
+_make_test_img 2M -o cluster_size=0x10000
+data_size=3*0x10000+0x8000
+$QEMU_IO -c "write -c -P 0x11 0 256k" "$TEST_IMG" \
+ 2>&1 | _filter_qemu_io | _filter_testdir
+
# success, all done
echo '*** done'
rm -f $seq.full
diff --git a/tests/qemu-iotests/214.out b/tests/qemu-iotests/214.out
index 0fcd8dc..2b908f6 100644
--- a/tests/qemu-iotests/214.out
+++ b/tests/qemu-iotests/214.out
@@ -32,4 +32,10 @@ read 4194304/4194304 bytes at offset 0
4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
read 4194304/4194304 bytes at offset 4194304
4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Write compressed data to multiple clusters ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
+wrote 262144/262144 bytes at offset 0
+256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
*** done
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 3/6] qemu-nbd: add compression flag support
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-02 14:22 ` [PATCH v2 2/6] tests/qemu-iotests: add case of " Andrey Shinkevich
@ 2019-10-02 14:22 ` Andrey Shinkevich
2019-10-03 14:32 ` Vladimir Sementsov-Ogievskiy
2019-10-04 10:19 ` Roman Kagan
2019-10-02 14:22 ` [PATCH v2 4/6] block: support compressed write for copy-on-read Andrey Shinkevich
` (4 subsequent siblings)
7 siblings, 2 replies; 21+ messages in thread
From: Andrey Shinkevich @ 2019-10-02 14:22 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: kwolf, fam, vsementsov, jsnow, armbru, dgilbert, stefanha,
andrey.shinkevich, den, mreitz
Added possibility to write compressed data by using the
blk_write_compressed. This action has the limitations of the format
driver. For example we can't write compressed data over other.
$ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
$ sudo ./qemu-nbd -c /dev/nbd0 ./image.qcow2
$ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
10+0 records in
10+0 records out
104857600 bytes (105 MB) copied, 0,0890581 s, 1,2 GB/s
$ sudo ./qemu-nbd -d /dev/nbd0
$ du -sh ./image.qcow2
101M ./image.qcow2
$ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
$ sudo ./qemu-nbd -C -c /dev/nbd0 ./image.qcow2
$ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
10+0 records in
10+0 records out
104857600 bytes (105 MB) copied, 0,076046 s, 1,4 GB/s
$ sudo ./qemu-nbd -d /dev/nbd0
$ du -sh ./image.qcow2
5,3M ./image.qcow2
Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
blockdev-nbd.c | 8 ++++++--
include/block/nbd.h | 11 +++++++++--
nbd/server.c | 14 ++++++++++----
qemu-nbd.c | 30 ++++++++++++++++++++++++++----
qemu-nbd.texi | 2 ++
5 files changed, 53 insertions(+), 12 deletions(-)
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 6a8b206..e83036b 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -151,6 +151,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
BlockBackend *on_eject_blk;
NBDExport *exp;
int64_t len;
+ uint32_t iflags = 0;
AioContext *aio_context;
if (!nbd_server) {
@@ -189,9 +190,12 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
if (bdrv_is_read_only(bs)) {
writable = false;
}
+ if (!writable) {
+ iflags = NBD_INTERNAL_FLAG_READONLY | NBD_INTERNAL_FLAG_SHARED;
+ }
- exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable,
- NULL, false, on_eject_blk, errp);
+ exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, iflags, NULL,
+ on_eject_blk, errp);
if (!exp) {
goto out;
}
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 316fd70..80be9d5 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -25,6 +25,13 @@
#include "crypto/tlscreds.h"
#include "qapi/error.h"
+enum {
+ NBD_INTERNAL_FLAG_READONLY = 1 << 0, /* Device is read-only */
+ NBD_INTERNAL_FLAG_SHARED = 1 << 1, /* Device can be shared */
+ NBD_INTERNAL_FLAG_WRITETHROUGH = 1 << 2, /* Enable write cache */
+ NBD_INTERNAL_FLAG_COMPRESS = 1 << 3, /* Use compressed write */
+};
+
/* Handshake phase structs - this struct is passed on the wire */
struct NBDOption {
@@ -330,8 +337,8 @@ typedef struct NBDClient NBDClient;
NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
uint64_t size, const char *name, const char *desc,
- const char *bitmap, bool readonly, bool shared,
- void (*close)(NBDExport *), bool writethrough,
+ const char *bitmap, uint32_t iflags,
+ void (*close)(NBDExport *),
BlockBackend *on_eject_blk, Error **errp);
void nbd_export_close(NBDExport *exp);
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
diff --git a/nbd/server.c b/nbd/server.c
index d8d1e62..cf3b75d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -91,6 +91,7 @@ struct NBDExport {
uint16_t nbdflags;
QTAILQ_HEAD(, NBDClient) clients;
QTAILQ_ENTRY(NBDExport) next;
+ uint32_t iflags;
AioContext *ctx;
@@ -1471,13 +1472,14 @@ static void nbd_eject_notifier(Notifier *n, void *data)
NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
uint64_t size, const char *name, const char *desc,
- const char *bitmap, bool readonly, bool shared,
- void (*close)(NBDExport *), bool writethrough,
+ const char *bitmap, uint32_t iflags,
+ void (*close)(NBDExport *),
BlockBackend *on_eject_blk, Error **errp)
{
AioContext *ctx;
BlockBackend *blk;
NBDExport *exp = g_new0(NBDExport, 1);
+ bool readonly = iflags & NBD_INTERNAL_FLAG_READONLY;
uint64_t perm;
int ret;
@@ -1504,7 +1506,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
if (ret < 0) {
goto fail;
}
- blk_set_enable_write_cache(blk, !writethrough);
+ blk_set_enable_write_cache(blk, !(iflags & NBD_INTERNAL_FLAG_WRITETHROUGH));
blk_set_allow_aio_context_change(blk, true);
exp->refcount = 1;
@@ -1518,13 +1520,14 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE);
if (readonly) {
exp->nbdflags |= NBD_FLAG_READ_ONLY;
- if (shared) {
+ if (iflags & NBD_INTERNAL_FLAG_SHARED) {
exp->nbdflags |= NBD_FLAG_CAN_MULTI_CONN;
}
} else {
exp->nbdflags |= (NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES |
NBD_FLAG_SEND_FAST_ZERO);
}
+ exp->iflags = iflags;
assert(size <= INT64_MAX - dev_offset);
exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
@@ -2312,6 +2315,9 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
if (request->flags & NBD_CMD_FLAG_FUA) {
flags |= BDRV_REQ_FUA;
}
+ if (exp->iflags & NBD_INTERNAL_FLAG_COMPRESS) {
+ flags |= BDRV_REQ_WRITE_COMPRESSED;
+ }
ret = blk_pwrite(exp->blk, request->from + exp->dev_offset,
data, request->len, flags);
return nbd_send_generic_reply(client, request->handle, ret,
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 9032b6d..4163135 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -139,6 +139,7 @@ static void usage(const char *name)
" --discard=MODE set discard mode (ignore, unmap)\n"
" --detect-zeroes=MODE set detect-zeroes mode (off, on, unmap)\n"
" --image-opts treat FILE as a full set of image options\n"
+" -C, --compress use data compression (if the target format supports it)\n"
"\n"
QEMU_HELP_BOTTOM "\n"
, name, name, NBD_DEFAULT_PORT, "DEVICE");
@@ -602,6 +603,7 @@ int main(int argc, char **argv)
BlockDriverState *bs;
uint64_t dev_offset = 0;
bool readonly = false;
+ uint32_t iflags = 0;
bool disconnect = false;
const char *bindto = NULL;
const char *port = NULL;
@@ -610,7 +612,7 @@ int main(int argc, char **argv)
int64_t fd_size;
QemuOpts *sn_opts = NULL;
const char *sn_id_or_name = NULL;
- const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:L";
+ const char *sopt = "hVb:o:p:rsnCP:c:dvk:e:f:tl:x:T:D:B:L";
struct option lopt[] = {
{ "help", no_argument, NULL, 'h' },
{ "version", no_argument, NULL, 'V' },
@@ -619,6 +621,7 @@ int main(int argc, char **argv)
{ "socket", required_argument, NULL, 'k' },
{ "offset", required_argument, NULL, 'o' },
{ "read-only", no_argument, NULL, 'r' },
+ { "compress", no_argument, NULL, 'C'},
{ "partition", required_argument, NULL, 'P' },
{ "bitmap", required_argument, NULL, 'B' },
{ "connect", required_argument, NULL, 'c' },
@@ -786,6 +789,9 @@ int main(int argc, char **argv)
readonly = true;
flags &= ~BDRV_O_RDWR;
break;
+ case 'C':
+ iflags |= NBD_INTERNAL_FLAG_COMPRESS;
+ break;
case 'P':
warn_report("The '-P' option is deprecated; use --image-opts with "
"a raw device wrapper for subset exports instead");
@@ -1117,6 +1123,14 @@ int main(int argc, char **argv)
blk_set_enable_write_cache(blk, !writethrough);
+ if ((iflags & NBD_INTERNAL_FLAG_COMPRESS) &&
+ !(bs->drv && bs->drv->bdrv_co_pwritev_compressed_part))
+ {
+ error_report("Compression not supported for this file format %s",
+ argv[optind]);
+ exit(EXIT_FAILURE);
+ }
+
if (sn_opts) {
ret = bdrv_snapshot_load_tmp(bs,
qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
@@ -1173,10 +1187,18 @@ int main(int argc, char **argv)
fd_size = limit;
}
+ if (writethrough) {
+ iflags |= NBD_INTERNAL_FLAG_WRITETHROUGH;
+ }
+ if (readonly) {
+ iflags |= NBD_INTERNAL_FLAG_READONLY;
+ }
+ if (shared > 1) {
+ iflags |= NBD_INTERNAL_FLAG_SHARED;
+ }
export = nbd_export_new(bs, dev_offset, fd_size, export_name,
- export_description, bitmap, readonly, shared > 1,
- nbd_export_closed, writethrough, NULL,
- &error_fatal);
+ export_description, bitmap, iflags,
+ nbd_export_closed, NULL, &error_fatal);
if (device) {
#if HAVE_NBD_DEVICE
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 7f55657..26ea1ec 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -55,6 +55,8 @@ Force the use of the block driver for format @var{fmt} instead of
auto-detecting.
@item -r, --read-only
Export the disk as read-only.
+@item -C, --compress
+Use data compression (if the target format supports it)
@item -P, --partition=@var{num}
Deprecated: Only expose MBR partition @var{num}. Understands physical
partitions 1-4 and logical partition 5. New code should instead use
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 4/6] block: support compressed write for copy-on-read
2019-10-02 14:22 [PATCH v2 0/6] qcow2: advanced compression options Andrey Shinkevich
` (2 preceding siblings ...)
2019-10-02 14:22 ` [PATCH v2 3/6] qemu-nbd: add compression flag support Andrey Shinkevich
@ 2019-10-02 14:22 ` 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
` (3 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Andrey Shinkevich @ 2019-10-02 14:22 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: kwolf, fam, vsementsov, jsnow, armbru, dgilbert, stefanha,
andrey.shinkevich, den, mreitz
Support the data compression during block-stream job over a backup
backing chain implemented in the following patch 'block-stream:
add compress option'.
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
block/io.c | 21 ++++++++++++++++-----
block/trace-events | 2 +-
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/block/io.c b/block/io.c
index f8c3596..a7cd24f 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1264,12 +1264,13 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
* allocating cluster in the image file. Note that this value may exceed
* BDRV_REQUEST_MAX_BYTES (even when the original read did not), which
* is one reason we loop rather than doing it all at once.
+ * Also, this is crucial for compressed copy-on-read.
*/
bdrv_round_to_clusters(bs, offset, bytes, &cluster_offset, &cluster_bytes);
skip_bytes = offset - cluster_offset;
trace_bdrv_co_do_copy_on_readv(bs, offset, bytes,
- cluster_offset, cluster_bytes);
+ cluster_offset, cluster_bytes, flags);
while (cluster_bytes) {
int64_t pnum;
@@ -1328,9 +1329,15 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
/* This does not change the data on the disk, it is not
* necessary to flush even in cache=writethrough mode.
*/
- ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
- &local_qiov, 0,
- BDRV_REQ_WRITE_UNCHANGED);
+ if (flags & BDRV_REQ_WRITE_COMPRESSED) {
+ ret = bdrv_driver_pwritev_compressed(bs, cluster_offset,
+ pnum, &local_qiov,
+ qiov_offset);
+ } else {
+ ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
+ &local_qiov, 0,
+ BDRV_REQ_WRITE_UNCHANGED);
+ }
}
if (ret < 0) {
@@ -1396,7 +1403,11 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
* to pass through to drivers. For now, there aren't any
* passthrough flags. */
assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
- BDRV_REQ_PREFETCH)));
+ BDRV_REQ_PREFETCH | BDRV_REQ_WRITE_COMPRESSED)));
+
+ /* write compressed only makes sense with copy on read */
+ assert(!(flags & BDRV_REQ_WRITE_COMPRESSED) ||
+ (flags & BDRV_REQ_COPY_ON_READ));
/* Handle Copy on Read and associated serialisation */
if (flags & BDRV_REQ_COPY_ON_READ) {
diff --git a/block/trace-events b/block/trace-events
index 3aa27e6..f444548 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -14,7 +14,7 @@ blk_root_detach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
bdrv_co_pwritev(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int count, int flags) "bs %p offset %"PRId64" count %d flags 0x%x"
-bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t cluster_offset, int64_t cluster_bytes) "bs %p offset %"PRId64" bytes %u cluster_offset %"PRId64" cluster_bytes %"PRId64
+bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t cluster_offset, int64_t cluster_bytes, int flags) "bs %p offset %"PRId64" bytes %u cluster_offset %"PRId64" cluster_bytes %"PRId64" flags 0x%x"
bdrv_co_copy_range_from(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int read_flags, int write_flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" rw flags 0x%x 0x%x"
bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int read_flags, int write_flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" rw flags 0x%x 0x%x"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 5/6] block-stream: add compress option
2019-10-02 14:22 [PATCH v2 0/6] qcow2: advanced compression options Andrey Shinkevich
` (3 preceding siblings ...)
2019-10-02 14:22 ` [PATCH v2 4/6] block: support compressed write for copy-on-read Andrey Shinkevich
@ 2019-10-02 14:22 ` 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
` (2 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Andrey Shinkevich @ 2019-10-02 14:22 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: kwolf, fam, vsementsov, jsnow, armbru, dgilbert, stefanha,
andrey.shinkevich, den, mreitz
Allow data compression during block-stream job for backup backing chain.
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
block/stream.c | 19 +++++++++++++------
blockdev.c | 14 +++++++++++++-
hmp-commands.hx | 4 ++--
include/block/block_int.h | 3 ++-
monitor/hmp-cmds.c | 5 +++--
qapi/block-core.json | 5 ++++-
6 files changed, 37 insertions(+), 13 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 5562ccb..51cc49e 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -36,15 +36,21 @@ typedef struct StreamBlockJob {
char *backing_file_str;
bool bs_read_only;
bool chain_frozen;
+ bool compress;
} StreamBlockJob;
-static int coroutine_fn stream_populate(BlockBackend *blk,
- int64_t offset, uint64_t bytes)
+static int coroutine_fn stream_populate(BlockBackend *blk, int64_t offset,
+ uint64_t bytes, bool compress)
{
+ int flags = BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH;
+
+ if (compress) {
+ flags |= BDRV_REQ_WRITE_COMPRESSED;
+ }
+
assert(bytes < SIZE_MAX);
- return blk_co_preadv(blk, offset, bytes, NULL,
- BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH);
+ return blk_co_preadv(blk, offset, bytes, NULL, flags);
}
static void stream_abort(Job *job)
@@ -167,7 +173,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
}
trace_stream_one_iteration(s, offset, n, ret);
if (copy) {
- ret = stream_populate(blk, offset, n);
+ ret = stream_populate(blk, offset, n, s->compress);
}
if (ret < 0) {
BlockErrorAction action =
@@ -217,7 +223,7 @@ static const BlockJobDriver stream_job_driver = {
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
- int creation_flags, int64_t speed,
+ int creation_flags, int64_t speed, bool compress,
BlockdevOnError on_error, Error **errp)
{
StreamBlockJob *s;
@@ -267,6 +273,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
s->backing_file_str = g_strdup(backing_file_str);
s->bs_read_only = bs_read_only;
s->chain_frozen = true;
+ s->compress = compress;
s->on_error = on_error;
trace_stream_start(bs, base, s);
diff --git a/blockdev.c b/blockdev.c
index fbef684..290ee4b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3238,6 +3238,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
bool has_base_node, const char *base_node,
bool has_backing_file, const char *backing_file,
bool has_speed, int64_t speed,
+ bool has_compress, bool compress,
bool has_on_error, BlockdevOnError on_error,
bool has_auto_finalize, bool auto_finalize,
bool has_auto_dismiss, bool auto_dismiss,
@@ -3254,6 +3255,10 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
on_error = BLOCKDEV_ON_ERROR_REPORT;
}
+ if (!has_compress) {
+ compress = false;
+ }
+
bs = bdrv_lookup_bs(device, device, errp);
if (!bs) {
return;
@@ -3308,6 +3313,12 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
goto out;
}
+ if (compress && bs->drv->bdrv_co_pwritev_compressed_part == NULL) {
+ error_setg(errp, "Compression is not supported for this drive %s",
+ bdrv_get_device_name(bs));
+ goto out;
+ }
+
/* backing_file string overrides base bs filename */
base_name = has_backing_file ? backing_file : base_name;
@@ -3319,7 +3330,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
}
stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
- job_flags, has_speed ? speed : 0, on_error, &local_err);
+ job_flags, has_speed ? speed : 0, compress, on_error,
+ &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto out;
diff --git a/hmp-commands.hx b/hmp-commands.hx
index cfcc044..3a347fd 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -95,8 +95,8 @@ ETEXI
{
.name = "block_stream",
- .args_type = "device:B,speed:o?,base:s?",
- .params = "device [speed [base]]",
+ .args_type = "device:B,speed:o?,base:s?,compress:o?",
+ .params = "device [speed [base]] [compress]",
.help = "copy data from a backing file into a block device",
.cmd = hmp_block_stream,
},
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 0422acd..5e7fce8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1065,6 +1065,7 @@ int is_windows_drive(const char *filename);
* @creation_flags: Flags that control the behavior of the Job lifetime.
* See @BlockJobCreateFlags
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @compress: True to compress data.
* @on_error: The action to take upon error.
* @errp: Error object.
*
@@ -1077,7 +1078,7 @@ int is_windows_drive(const char *filename);
*/
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
- int creation_flags, int64_t speed,
+ int creation_flags, int64_t speed, bool compress,
BlockdevOnError on_error, Error **errp);
/**
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b2551c1..91201fe 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2025,11 +2025,12 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
const char *device = qdict_get_str(qdict, "device");
const char *base = qdict_get_try_str(qdict, "base");
int64_t speed = qdict_get_try_int(qdict, "speed", 0);
+ bool compress = qdict_get_try_bool(qdict, "compress", false);
qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
false, NULL, qdict_haskey(qdict, "speed"), speed, true,
- BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
- &error);
+ compress, true, BLOCKDEV_ON_ERROR_REPORT,
+ false, false, false, false, &error);
hmp_handle_error(mon, &error);
}
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e6edd64..9c2093e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2544,6 +2544,9 @@
#
# @speed: the maximum speed, in bytes per second
#
+# @compress: true to compress data, if the target format supports it
+# (default: false). Since 4.2.
+#
# @on-error: the action to take on an error (default report).
# 'stop' and 'enospc' can only be used if the block device
# supports io-status (see BlockInfo). Since 1.3.
@@ -2576,7 +2579,7 @@
{ 'command': 'block-stream',
'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
'*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
- '*on-error': 'BlockdevOnError',
+ '*compress': 'bool', '*on-error': 'BlockdevOnError',
'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
##
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 6/6] tests/qemu-iotests: add case for block-stream compress
2019-10-02 14:22 [PATCH v2 0/6] qcow2: advanced compression options Andrey Shinkevich
` (4 preceding siblings ...)
2019-10-02 14:22 ` [PATCH v2 5/6] block-stream: add compress option Andrey Shinkevich
@ 2019-10-02 14:22 ` Andrey Shinkevich
2019-10-03 14:58 ` Vladimir Sementsov-Ogievskiy
2019-10-02 15:07 ` [PATCH v2 0/6] qcow2: advanced compression options no-reply
2019-10-02 15:07 ` no-reply
7 siblings, 1 reply; 21+ messages in thread
From: Andrey Shinkevich @ 2019-10-02 14:22 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: kwolf, fam, vsementsov, jsnow, armbru, dgilbert, stefanha,
andrey.shinkevich, den, mreitz
Add a test case to the iotest #030 that checks 'compress' option for a
block-stream job.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
tests/qemu-iotests/030 | 49 +++++++++++++++++++++++++++++++++++++++++++++-
tests/qemu-iotests/030.out | 4 ++--
2 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index f3766f2..13fe5a2 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -21,7 +21,8 @@
import time
import os
import iotests
-from iotests import qemu_img, qemu_io
+from iotests import qemu_img, qemu_io, qemu_img_pipe
+import json
backing_img = os.path.join(iotests.test_dir, 'backing.img')
mid_img = os.path.join(iotests.test_dir, 'mid.img')
@@ -956,6 +957,52 @@ class TestSetSpeed(iotests.QMPTestCase):
self.cancel_and_wait(resume=True)
+class TestCompressed(iotests.QMPTestCase):
+
+ def setUp(self):
+ qemu_img('create', '-f', iotests.imgfmt, backing_img, '1M')
+ qemu_img('create', '-f', iotests.imgfmt, '-o',
+ 'backing_file=%s' % backing_img, mid_img)
+ qemu_img('create', '-f', iotests.imgfmt, '-o',
+ 'backing_file=%s' % mid_img, test_img)
+ qemu_io('-c', 'write -P 0x1 0 512k', backing_img)
+ self.vm = iotests.VM().add_drive(test_img, "backing.node-name=mid," +
+ "backing.backing.node-name=base")
+ self.vm.launch()
+
+ def tearDown(self):
+ self.vm.shutdown()
+ os.remove(test_img)
+ os.remove(mid_img)
+ os.remove(backing_img)
+
+ def test_stream_compress(self):
+ self.assert_no_active_block_jobs()
+
+ result = self.vm.qmp('block-stream', device='mid', job_id='stream-mid')
+ self.assert_qmp(result, 'return', {})
+
+ self.wait_until_completed(drive='stream-mid')
+ for event in self.vm.get_qmp_events(wait=True):
+ if event['event'] == 'BLOCK_JOB_COMPLETED':
+ self.dictpath(event, 'data/device')
+ self.assert_qmp_absent(event, 'data/error')
+
+ result = self.vm.qmp('block-stream', device='drive0', base=mid_img,
+ job_id='stream-top', compress=True)
+ self.assert_qmp(result, 'return', {})
+
+ self.wait_until_completed(drive='stream-top')
+ self.assert_no_active_block_jobs()
+ self.vm.shutdown()
+
+ top = json.loads(qemu_img_pipe('info', '--output=json', test_img))
+ mid = json.loads(qemu_img_pipe('info', '--output=json', mid_img))
+ base = json.loads(qemu_img_pipe('info', '--output=json', backing_img))
+
+ self.assertEqual(mid['actual-size'], base['actual-size'])
+ self.assertLess(top['actual-size'], mid['actual-size'])
+
if __name__ == '__main__':
iotests.main(supported_fmts=['qcow2', 'qed'],
supported_protocols=['file'])
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 6d9bee1..af8dac1 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-...........................
+............................
----------------------------------------------------------------------
-Ran 27 tests
+Ran 28 tests
OK
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/6] qcow2: advanced compression options
2019-10-02 14:22 [PATCH v2 0/6] qcow2: advanced compression options Andrey Shinkevich
` (5 preceding siblings ...)
2019-10-02 14:22 ` [PATCH v2 6/6] tests/qemu-iotests: add case for block-stream compress Andrey Shinkevich
@ 2019-10-02 15:07 ` no-reply
2019-10-02 15:35 ` Vladimir Sementsov-Ogievskiy
2019-10-02 15:07 ` no-reply
7 siblings, 1 reply; 21+ messages in thread
From: no-reply @ 2019-10-02 15:07 UTC (permalink / raw)
To: andrey.shinkevich
Cc: kwolf, fam, vsementsov, qemu-block, qemu-devel, armbru, stefanha,
den, andrey.shinkevich, mreitz, jsnow, dgilbert
Patchew URL: https://patchew.org/QEMU/1570026166-748566-1-git-send-email-andrey.shinkevich@virtuozzo.com/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
CC block/parallels.o
CC block/blklogwrites.o
CC block/block-backend.o
/tmp/qemu-test/src/block/qcow2.c:4077:64: error: unknown type name 'AioTask'
static coroutine_fn int qcow2_co_pwritev_compressed_task_entry(AioTask *task)
^
/tmp/qemu-test/src/block/qcow2.c: In function 'qcow2_co_pwritev_compressed_part':
/tmp/qemu-test/src/block/qcow2.c:4098:5: error: unknown type name 'AioTaskPool'
AioTaskPool *aio = NULL;
^
/tmp/qemu-test/src/block/qcow2.c:4123:5: error: implicit declaration of function 'aio_task_pool_status' [-Werror=implicit-function-declaration]
while (bytes && aio_task_pool_status(aio) == 0) {
^
/tmp/qemu-test/src/block/qcow2.c:4123:5: error: nested extern declaration of 'aio_task_pool_status' [-Werror=nested-externs]
/tmp/qemu-test/src/block/qcow2.c:4130:13: error: implicit declaration of function 'aio_task_pool_new' [-Werror=implicit-function-declaration]
aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
^
/tmp/qemu-test/src/block/qcow2.c:4130:13: error: nested extern declaration of 'aio_task_pool_new' [-Werror=nested-externs]
/tmp/qemu-test/src/block/qcow2.c:4130:37: error: 'QCOW2_MAX_WORKERS' undeclared (first use in this function)
aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
^
/tmp/qemu-test/src/block/qcow2.c:4130:37: note: each undeclared identifier is reported only once for each function it appears in
/tmp/qemu-test/src/block/qcow2.c:4133:9: error: implicit declaration of function 'qcow2_add_task' [-Werror=implicit-function-declaration]
ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_compressed_task_entry,
^
/tmp/qemu-test/src/block/qcow2.c:4133:9: error: nested extern declaration of 'qcow2_add_task' [-Werror=nested-externs]
/tmp/qemu-test/src/block/qcow2.c:4133:39: error: 'qcow2_co_pwritev_compressed_task_entry' undeclared (first use in this function)
ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_compressed_task_entry,
^
/tmp/qemu-test/src/block/qcow2.c:4145:9: error: implicit declaration of function 'aio_task_pool_wait_all' [-Werror=implicit-function-declaration]
aio_task_pool_wait_all(aio);
^
/tmp/qemu-test/src/block/qcow2.c:4145:9: error: nested extern declaration of 'aio_task_pool_wait_all' [-Werror=nested-externs]
/tmp/qemu-test/src/block/qcow2.c: At top level:
/tmp/qemu-test/src/block/qcow2.c:4012:1: error: 'qcow2_co_pwritev_compressed_task' defined but not used [-Werror=unused-function]
qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
^
cc1: all warnings being treated as errors
make: *** [block/qcow2.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 662, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=4a0fa981cdb74b48946f62d3df956bf1', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-zk1t0pwl/src/docker-src.2019-10-02-11.05.34.28094:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=4a0fa981cdb74b48946f62d3df956bf1
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-zk1t0pwl/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 1m39.268s
user 0m7.984s
The full log is available at
http://patchew.org/logs/1570026166-748566-1-git-send-email-andrey.shinkevich@virtuozzo.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/6] qcow2: advanced compression options
2019-10-02 14:22 [PATCH v2 0/6] qcow2: advanced compression options Andrey Shinkevich
` (6 preceding siblings ...)
2019-10-02 15:07 ` [PATCH v2 0/6] qcow2: advanced compression options no-reply
@ 2019-10-02 15:07 ` no-reply
7 siblings, 0 replies; 21+ messages in thread
From: no-reply @ 2019-10-02 15:07 UTC (permalink / raw)
To: andrey.shinkevich
Cc: kwolf, fam, vsementsov, qemu-block, qemu-devel, armbru, stefanha,
den, andrey.shinkevich, mreitz, jsnow, dgilbert
Patchew URL: https://patchew.org/QEMU/1570026166-748566-1-git-send-email-andrey.shinkevich@virtuozzo.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
CC block/quorum.o
CC block/blkdebug.o
CC block/blkverify.o
/tmp/qemu-test/src/block/qcow2.c:4077:64: error: unknown type name 'AioTask'; did you mean 'AioWait'?
static coroutine_fn int qcow2_co_pwritev_compressed_task_entry(AioTask *task)
^~~~~~~
AioWait
/tmp/qemu-test/src/block/qcow2.c: In function 'qcow2_co_pwritev_compressed_part':
/tmp/qemu-test/src/block/qcow2.c:4098:5: error: unknown type name 'AioTaskPool'
AioTaskPool *aio = NULL;
^~~~~~~~~~~
/tmp/qemu-test/src/block/qcow2.c:4123:21: error: implicit declaration of function 'aio_task_pool_status' [-Werror=implicit-function-declaration]
while (bytes && aio_task_pool_status(aio) == 0) {
^~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/block/qcow2.c:4123:21: error: nested extern declaration of 'aio_task_pool_status' [-Werror=nested-externs]
/tmp/qemu-test/src/block/qcow2.c:4130:19: error: implicit declaration of function 'aio_task_pool_new'; did you mean 'aio_timer_new'? [-Werror=implicit-function-declaration]
aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
^~~~~~~~~~~~~~~~~
aio_timer_new
/tmp/qemu-test/src/block/qcow2.c:4130:19: error: nested extern declaration of 'aio_task_pool_new' [-Werror=nested-externs]
/tmp/qemu-test/src/block/qcow2.c:4130:37: error: 'QCOW2_MAX_WORKERS' undeclared (first use in this function); did you mean 'QCOW2_MAX_THREADS'?
aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
^~~~~~~~~~~~~~~~~
QCOW2_MAX_THREADS
/tmp/qemu-test/src/block/qcow2.c:4130:37: note: each undeclared identifier is reported only once for each function it appears in
/tmp/qemu-test/src/block/qcow2.c:4133:15: error: implicit declaration of function 'qcow2_add_task'; did you mean 'qcow2_do_open'? [-Werror=implicit-function-declaration]
ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_compressed_task_entry,
^~~~~~~~~~~~~~
qcow2_do_open
/tmp/qemu-test/src/block/qcow2.c:4133:15: error: nested extern declaration of 'qcow2_add_task' [-Werror=nested-externs]
/tmp/qemu-test/src/block/qcow2.c:4133:39: error: 'qcow2_co_pwritev_compressed_task_entry' undeclared (first use in this function); did you mean 'qcow2_co_pwritev_compressed_task'?
ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_compressed_task_entry,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
qcow2_co_pwritev_compressed_task
/tmp/qemu-test/src/block/qcow2.c:4145:9: error: implicit declaration of function 'aio_task_pool_wait_all' [-Werror=implicit-function-declaration]
aio_task_pool_wait_all(aio);
^~~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/block/qcow2.c:4145:9: error: nested extern declaration of 'aio_task_pool_wait_all' [-Werror=nested-externs]
At top level:
/tmp/qemu-test/src/block/qcow2.c:4012:1: error: 'qcow2_co_pwritev_compressed_task' defined but not used [-Werror=unused-function]
qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: block/qcow2.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 662, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=b4ead0a324e949c19b89413b42658f34', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-w_sflkq1/src/docker-src.2019-10-02-11.05.59.29878:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=b4ead0a324e949c19b89413b42658f34
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-w_sflkq1/src'
make: *** [docker-run-test-mingw@fedora] Error 2
real 1m50.644s
user 0m7.709s
The full log is available at
http://patchew.org/logs/1570026166-748566-1-git-send-email-andrey.shinkevich@virtuozzo.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/6] qcow2: advanced compression options
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
0 siblings, 1 reply; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-10-02 15:35 UTC (permalink / raw)
To: qemu-devel, Andrey Shinkevich
Cc: kwolf, fam, Denis Lunev, qemu-block, patchew-devel, armbru,
dgilbert, stefanha, mreitz, jsnow
Hi,
02.10.2019 18:07, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/1570026166-748566-1-git-send-email-andrey.shinkevich@virtuozzo.com/
>
>
>
> Hi,
>
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
>
> CC block/parallels.o
> CC block/blklogwrites.o
> CC block/block-backend.o
> /tmp/qemu-test/src/block/qcow2.c:4077:64: error: unknown type name 'AioTask'
> static coroutine_fn int qcow2_co_pwritev_compressed_task_entry(AioTask *task)
> ^
> /tmp/qemu-test/src/block/qcow2.c: In function 'qcow2_co_pwritev_compressed_part':
> /tmp/qemu-test/src/block/qcow2.c:4098:5: error: unknown type name 'AioTaskPool'
> AioTaskPool *aio = NULL;
> ^
Who knows, what is wrong with it? Seems patchew ignores Based-on: tag in cover-letter,
which is written as "Based-on: https://github.com/XanClic/qemu.git block"...
These new types and functions are defined in Max's block branch.
> /tmp/qemu-test/src/block/qcow2.c:4123:5: error: implicit declaration of function 'aio_task_pool_status' [-Werror=implicit-function-declaration]
> while (bytes && aio_task_pool_status(aio) == 0) {
> ^
> /tmp/qemu-test/src/block/qcow2.c:4123:5: error: nested extern declaration of 'aio_task_pool_status' [-Werror=nested-externs]
> /tmp/qemu-test/src/block/qcow2.c:4130:13: error: implicit declaration of function 'aio_task_pool_new' [-Werror=implicit-function-declaration]
> aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
> ^
> /tmp/qemu-test/src/block/qcow2.c:4130:13: error: nested extern declaration of 'aio_task_pool_new' [-Werror=nested-externs]
> /tmp/qemu-test/src/block/qcow2.c:4130:37: error: 'QCOW2_MAX_WORKERS' undeclared (first use in this function)
> aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
> ^
> /tmp/qemu-test/src/block/qcow2.c:4130:37: note: each undeclared identifier is reported only once for each function it appears in
> /tmp/qemu-test/src/block/qcow2.c:4133:9: error: implicit declaration of function 'qcow2_add_task' [-Werror=implicit-function-declaration]
> ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_compressed_task_entry,
> ^
> /tmp/qemu-test/src/block/qcow2.c:4133:9: error: nested extern declaration of 'qcow2_add_task' [-Werror=nested-externs]
> /tmp/qemu-test/src/block/qcow2.c:4133:39: error: 'qcow2_co_pwritev_compressed_task_entry' undeclared (first use in this function)
> ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_compressed_task_entry,
> ^
> /tmp/qemu-test/src/block/qcow2.c:4145:9: error: implicit declaration of function 'aio_task_pool_wait_all' [-Werror=implicit-function-declaration]
> aio_task_pool_wait_all(aio);
> ^
> /tmp/qemu-test/src/block/qcow2.c:4145:9: error: nested extern declaration of 'aio_task_pool_wait_all' [-Werror=nested-externs]
> /tmp/qemu-test/src/block/qcow2.c: At top level:
> /tmp/qemu-test/src/block/qcow2.c:4012:1: error: 'qcow2_co_pwritev_compressed_task' defined but not used [-Werror=unused-function]
> qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
> ^
> cc1: all warnings being treated as errors
> make: *** [block/qcow2.o] Error 1
> make: *** Waiting for unfinished jobs....
> Traceback (most recent call last):
> File "./tests/docker/docker.py", line 662, in <module>
> ---
> raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=4a0fa981cdb74b48946f62d3df956bf1', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-zk1t0pwl/src/docker-src.2019-10-02-11.05.34.28094:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
> filter=--filter=label=com.qemu.instance.uuid=4a0fa981cdb74b48946f62d3df956bf1
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-zk1t0pwl/src'
> make: *** [docker-run-test-quick@centos7] Error 2
>
> real 1m39.268s
> user 0m7.984s
>
>
> The full log is available at
> http://patchew.org/logs/1570026166-748566-1-git-send-email-andrey.shinkevich@virtuozzo.com/testing.docker-quick@centos7/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/6] qcow2: advanced compression options
2019-10-02 15:35 ` Vladimir Sementsov-Ogievskiy
@ 2019-10-02 15:58 ` Max Reitz
2019-10-02 19:04 ` Markus Armbruster
0 siblings, 1 reply; 21+ messages in thread
From: Max Reitz @ 2019-10-02 15:58 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, Andrey Shinkevich
Cc: kwolf, fam, Denis Lunev, qemu-block, patchew-devel, armbru,
dgilbert, stefanha, jsnow
[-- Attachment #1.1: Type: text/plain, Size: 1656 bytes --]
On 02.10.19 17:35, Vladimir Sementsov-Ogievskiy wrote:
> Hi,
>
> 02.10.2019 18:07, no-reply@patchew.org wrote:
>> Patchew URL: https://patchew.org/QEMU/1570026166-748566-1-git-send-email-andrey.shinkevich@virtuozzo.com/
>>
>>
>>
>> Hi,
>>
>> This series failed the docker-quick@centos7 build test. Please find the testing commands and
>> their output below. If you have Docker installed, you can probably reproduce it
>> locally.
>>
>> === TEST SCRIPT BEGIN ===
>> #!/bin/bash
>> make docker-image-centos7 V=1 NETWORK=1
>> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
>> === TEST SCRIPT END ===
>>
>> CC block/parallels.o
>> CC block/blklogwrites.o
>> CC block/block-backend.o
>> /tmp/qemu-test/src/block/qcow2.c:4077:64: error: unknown type name 'AioTask'
>> static coroutine_fn int qcow2_co_pwritev_compressed_task_entry(AioTask *task)
>> ^
>> /tmp/qemu-test/src/block/qcow2.c: In function 'qcow2_co_pwritev_compressed_part':
>> /tmp/qemu-test/src/block/qcow2.c:4098:5: error: unknown type name 'AioTaskPool'
>> AioTaskPool *aio = NULL;
>> ^
>
>
> Who knows, what is wrong with it? Seems patchew ignores Based-on: tag in cover-letter,
> which is written as "Based-on: https://github.com/XanClic/qemu.git block"...
> These new types and functions are defined in Max's block branch.
It would be news to me if Patchew supported such URLs. I just put it
into my cover letter for human reviewers...
(Actually, it would be news to me if Patchew supported Based-on at all
reliably...)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/6] qcow2: advanced compression options
2019-10-02 15:58 ` Max Reitz
@ 2019-10-02 19:04 ` Markus Armbruster
0 siblings, 0 replies; 21+ messages in thread
From: Markus Armbruster @ 2019-10-02 19:04 UTC (permalink / raw)
To: Max Reitz
Cc: kwolf, fam, Vladimir Sementsov-Ogievskiy, Denis Lunev,
qemu-block, patchew-devel, qemu-devel, armbru, stefanha,
Andrey Shinkevich, jsnow, dgilbert
Max Reitz <mreitz@redhat.com> writes:
> On 02.10.19 17:35, Vladimir Sementsov-Ogievskiy wrote:
>> Hi,
>>
>> 02.10.2019 18:07, no-reply@patchew.org wrote:
>>> Patchew URL: https://patchew.org/QEMU/1570026166-748566-1-git-send-email-andrey.shinkevich@virtuozzo.com/
>>>
>>>
>>>
>>> Hi,
>>>
>>> This series failed the docker-quick@centos7 build test. Please find the testing commands and
>>> their output below. If you have Docker installed, you can probably reproduce it
>>> locally.
>>>
>>> === TEST SCRIPT BEGIN ===
>>> #!/bin/bash
>>> make docker-image-centos7 V=1 NETWORK=1
>>> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
>>> === TEST SCRIPT END ===
>>>
>>> CC block/parallels.o
>>> CC block/blklogwrites.o
>>> CC block/block-backend.o
>>> /tmp/qemu-test/src/block/qcow2.c:4077:64: error: unknown type name 'AioTask'
>>> static coroutine_fn int qcow2_co_pwritev_compressed_task_entry(AioTask *task)
>>> ^
>>> /tmp/qemu-test/src/block/qcow2.c: In function 'qcow2_co_pwritev_compressed_part':
>>> /tmp/qemu-test/src/block/qcow2.c:4098:5: error: unknown type name 'AioTaskPool'
>>> AioTaskPool *aio = NULL;
>>> ^
>>
>>
>> Who knows, what is wrong with it? Seems patchew ignores Based-on: tag in cover-letter,
>> which is written as "Based-on: https://github.com/XanClic/qemu.git block"...
>> These new types and functions are defined in Max's block branch.
>
> It would be news to me if Patchew supported such URLs. I just put it
> into my cover letter for human reviewers...
>
> (Actually, it would be news to me if Patchew supported Based-on at all
> reliably...)
https://github.com/patchew-project/patchew/commit/5e461e7c49c1913cb34349f45cc7566627b37288
I use Based-on: <Message-Id> all the time, and it works reliably for me.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] qcow2: Allow writing compressed data to multiple clusters
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
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-10-03 14:16 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-devel, qemu-block
Cc: kwolf, fam, Denis Lunev, jsnow, armbru, dgilbert, stefanha, mreitz
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/6] tests/qemu-iotests: add case of writing compressed data to multiple clusters
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
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-10-03 14:21 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-devel, qemu-block
Cc: kwolf, fam, Denis Lunev, jsnow, armbru, dgilbert, stefanha, mreitz
02.10.2019 17:22, Andrey Shinkevich wrote:
> Add the test case to the iotest #214 that checks possibility of writing
> compressed data to more than one cluster.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> tests/qemu-iotests/214 | 9 +++++++++
> tests/qemu-iotests/214.out | 6 ++++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
> index 21ec8a2..5f437e4 100755
> --- a/tests/qemu-iotests/214
> +++ b/tests/qemu-iotests/214
> @@ -89,6 +89,15 @@ _check_test_img -r all
> $QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
> $QEMU_IO -c "read -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
>
> +echo
> +echo "=== Write compressed data to multiple clusters ==="
> +echo
> +# Create an empty image and fill three and half clusters with compressed data.
Hmm, but we support only aligned to cluster writes, isn't it? With the only exception for
last cluster of the image?
> +_make_test_img 2M -o cluster_size=0x10000
> +data_size=3*0x10000+0x8000
data_size is unused.
> +$QEMU_IO -c "write -c -P 0x11 0 256k" "$TEST_IMG" \
> + 2>&1 | _filter_qemu_io | _filter_testdir
> +
will be good to check img and print qemu img map (or something like this) to see these
compressed clusters.
> # success, all done
> echo '*** done'
> rm -f $seq.full
> diff --git a/tests/qemu-iotests/214.out b/tests/qemu-iotests/214.out
> index 0fcd8dc..2b908f6 100644
> --- a/tests/qemu-iotests/214.out
> +++ b/tests/qemu-iotests/214.out
> @@ -32,4 +32,10 @@ read 4194304/4194304 bytes at offset 0
> 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> read 4194304/4194304 bytes at offset 4194304
> 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +=== Write compressed data to multiple clusters ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
> +wrote 262144/262144 bytes at offset 0
> +256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> *** done
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/6] qemu-nbd: add compression flag support
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
1 sibling, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-10-03 14:32 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-devel, qemu-block
Cc: kwolf, fam, Denis Lunev, jsnow, armbru, dgilbert, stefanha, mreitz
02.10.2019 17:22, Andrey Shinkevich wrote:
> Added possibility to write compressed data by using the
> blk_write_compressed. This action has the limitations of the format
> driver. For example we can't write compressed data over other.
>
> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
> $ sudo ./qemu-nbd -c /dev/nbd0 ./image.qcow2
> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
> 10+0 records in
> 10+0 records out
> 104857600 bytes (105 MB) copied, 0,0890581 s, 1,2 GB/s
> $ sudo ./qemu-nbd -d /dev/nbd0
> $ du -sh ./image.qcow2
> 101M ./image.qcow2
>
> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
> $ sudo ./qemu-nbd -C -c /dev/nbd0 ./image.qcow2
> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
> 10+0 records in
> 10+0 records out
> 104857600 bytes (105 MB) copied, 0,076046 s, 1,4 GB/s
> $ sudo ./qemu-nbd -d /dev/nbd0
> $ du -sh ./image.qcow2
> 5,3M ./image.qcow2
>
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> blockdev-nbd.c | 8 ++++++--
> include/block/nbd.h | 11 +++++++++--
> nbd/server.c | 14 ++++++++++----
> qemu-nbd.c | 30 ++++++++++++++++++++++++++----
> qemu-nbd.texi | 2 ++
> 5 files changed, 53 insertions(+), 12 deletions(-)
>
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 6a8b206..e83036b 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -151,6 +151,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
> BlockBackend *on_eject_blk;
> NBDExport *exp;
> int64_t len;
> + uint32_t iflags = 0;
> AioContext *aio_context;
>
> if (!nbd_server) {
> @@ -189,9 +190,12 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
> if (bdrv_is_read_only(bs)) {
> writable = false;
> }
> + if (!writable) {
> + iflags = NBD_INTERNAL_FLAG_READONLY | NBD_INTERNAL_FLAG_SHARED;
> + }
>
> - exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable,
> - NULL, false, on_eject_blk, errp);
> + exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, iflags, NULL,
> + on_eject_blk, errp);
> if (!exp) {
> goto out;
> }
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 316fd70..80be9d5 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -25,6 +25,13 @@
> #include "crypto/tlscreds.h"
> #include "qapi/error.h"
>
> +enum {
> + NBD_INTERNAL_FLAG_READONLY = 1 << 0, /* Device is read-only */
> + NBD_INTERNAL_FLAG_SHARED = 1 << 1, /* Device can be shared */
> + NBD_INTERNAL_FLAG_WRITETHROUGH = 1 << 2, /* Enable write cache */
> + NBD_INTERNAL_FLAG_COMPRESS = 1 << 3, /* Use compressed write */
> +};
Please, do this refactoring in a separate commit.
> +
> /* Handshake phase structs - this struct is passed on the wire */
>
> struct NBDOption {
> @@ -330,8 +337,8 @@ typedef struct NBDClient NBDClient;
>
> NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> uint64_t size, const char *name, const char *desc,
> - const char *bitmap, bool readonly, bool shared,
> - void (*close)(NBDExport *), bool writethrough,
> + const char *bitmap, uint32_t iflags,
> + void (*close)(NBDExport *),
> BlockBackend *on_eject_blk, Error **errp);
> void nbd_export_close(NBDExport *exp);
> void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
> diff --git a/nbd/server.c b/nbd/server.c
> index d8d1e62..cf3b75d 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -91,6 +91,7 @@ struct NBDExport {
> uint16_t nbdflags;
> QTAILQ_HEAD(, NBDClient) clients;
> QTAILQ_ENTRY(NBDExport) next;
> + uint32_t iflags;
>
> AioContext *ctx;
>
> @@ -1471,13 +1472,14 @@ static void nbd_eject_notifier(Notifier *n, void *data)
>
> NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> uint64_t size, const char *name, const char *desc,
> - const char *bitmap, bool readonly, bool shared,
> - void (*close)(NBDExport *), bool writethrough,
> + const char *bitmap, uint32_t iflags,
> + void (*close)(NBDExport *),
> BlockBackend *on_eject_blk, Error **errp)
> {
> AioContext *ctx;
> BlockBackend *blk;
> NBDExport *exp = g_new0(NBDExport, 1);
> + bool readonly = iflags & NBD_INTERNAL_FLAG_READONLY;
> uint64_t perm;
> int ret;
Hmm, we should check in this function that compression is supported, if it required by
iflags and return error otherwise.
>
> @@ -1504,7 +1506,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> if (ret < 0) {
> goto fail;
> }
> - blk_set_enable_write_cache(blk, !writethrough);
> + blk_set_enable_write_cache(blk, !(iflags & NBD_INTERNAL_FLAG_WRITETHROUGH));
> blk_set_allow_aio_context_change(blk, true);
>
> exp->refcount = 1;
> @@ -1518,13 +1520,14 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE);
> if (readonly) {
> exp->nbdflags |= NBD_FLAG_READ_ONLY;
> - if (shared) {
> + if (iflags & NBD_INTERNAL_FLAG_SHARED) {
> exp->nbdflags |= NBD_FLAG_CAN_MULTI_CONN;
> }
> } else {
> exp->nbdflags |= (NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES |
> NBD_FLAG_SEND_FAST_ZERO);
> }
> + exp->iflags = iflags;
> assert(size <= INT64_MAX - dev_offset);
> exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
>
> @@ -2312,6 +2315,9 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
> if (request->flags & NBD_CMD_FLAG_FUA) {
> flags |= BDRV_REQ_FUA;
> }
> + if (exp->iflags & NBD_INTERNAL_FLAG_COMPRESS) {
> + flags |= BDRV_REQ_WRITE_COMPRESSED;
> + }
> ret = blk_pwrite(exp->blk, request->from + exp->dev_offset,
> data, request->len, flags);
> return nbd_send_generic_reply(client, request->handle, ret,
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 9032b6d..4163135 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -139,6 +139,7 @@ static void usage(const char *name)
> " --discard=MODE set discard mode (ignore, unmap)\n"
> " --detect-zeroes=MODE set detect-zeroes mode (off, on, unmap)\n"
> " --image-opts treat FILE as a full set of image options\n"
> +" -C, --compress use data compression (if the target format supports it)\n"
IMHO s/target/image/
> "\n"
> QEMU_HELP_BOTTOM "\n"
> , name, name, NBD_DEFAULT_PORT, "DEVICE");
> @@ -602,6 +603,7 @@ int main(int argc, char **argv)
> BlockDriverState *bs;
> uint64_t dev_offset = 0;
> bool readonly = false;
> + uint32_t iflags = 0;
> bool disconnect = false;
> const char *bindto = NULL;
> const char *port = NULL;
> @@ -610,7 +612,7 @@ int main(int argc, char **argv)
> int64_t fd_size;
> QemuOpts *sn_opts = NULL;
> const char *sn_id_or_name = NULL;
> - const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:L";
> + const char *sopt = "hVb:o:p:rsnCP:c:dvk:e:f:tl:x:T:D:B:L";
> struct option lopt[] = {
> { "help", no_argument, NULL, 'h' },
> { "version", no_argument, NULL, 'V' },
> @@ -619,6 +621,7 @@ int main(int argc, char **argv)
> { "socket", required_argument, NULL, 'k' },
> { "offset", required_argument, NULL, 'o' },
> { "read-only", no_argument, NULL, 'r' },
> + { "compress", no_argument, NULL, 'C'},
> { "partition", required_argument, NULL, 'P' },
> { "bitmap", required_argument, NULL, 'B' },
> { "connect", required_argument, NULL, 'c' },
> @@ -786,6 +789,9 @@ int main(int argc, char **argv)
> readonly = true;
> flags &= ~BDRV_O_RDWR;
> break;
> + case 'C':
> + iflags |= NBD_INTERNAL_FLAG_COMPRESS;
> + break;
> case 'P':
> warn_report("The '-P' option is deprecated; use --image-opts with "
> "a raw device wrapper for subset exports instead");
> @@ -1117,6 +1123,14 @@ int main(int argc, char **argv)
>
> blk_set_enable_write_cache(blk, !writethrough);
>
> + if ((iflags & NBD_INTERNAL_FLAG_COMPRESS) &&
> + !(bs->drv && bs->drv->bdrv_co_pwritev_compressed_part))
> + {
> + error_report("Compression not supported for this file format %s",
> + argv[optind]);
> + exit(EXIT_FAILURE);
> + }
> +
> if (sn_opts) {
> ret = bdrv_snapshot_load_tmp(bs,
> qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
> @@ -1173,10 +1187,18 @@ int main(int argc, char **argv)
> fd_size = limit;
> }
>
> + if (writethrough) {
> + iflags |= NBD_INTERNAL_FLAG_WRITETHROUGH;
> + }
> + if (readonly) {
> + iflags |= NBD_INTERNAL_FLAG_READONLY;
> + }
> + if (shared > 1) {
> + iflags |= NBD_INTERNAL_FLAG_SHARED;
> + }
I think, you instead should drop these boolean variables and merely fill iflags instead,
like you do for compression.
> export = nbd_export_new(bs, dev_offset, fd_size, export_name,
> - export_description, bitmap, readonly, shared > 1,
> - nbd_export_closed, writethrough, NULL,
> - &error_fatal);
> + export_description, bitmap, iflags,
> + nbd_export_closed, NULL, &error_fatal);
>
> if (device) {
> #if HAVE_NBD_DEVICE
> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index 7f55657..26ea1ec 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -55,6 +55,8 @@ Force the use of the block driver for format @var{fmt} instead of
> auto-detecting.
> @item -r, --read-only
> Export the disk as read-only.
> +@item -C, --compress
> +Use data compression (if the target format supports it)
Not sure about word "target", I think just "image" would be better.
> @item -P, --partition=@var{num}
> Deprecated: Only expose MBR partition @var{num}. Understands physical
> partitions 1-4 and logical partition 5. New code should instead use
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/6] block: support compressed write for copy-on-read
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
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-10-03 14:37 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-devel, qemu-block
Cc: kwolf, fam, Denis Lunev, jsnow, armbru, dgilbert, stefanha, mreitz
02.10.2019 17:22, Andrey Shinkevich wrote:
> Support the data compression during block-stream job over a backup
> backing chain implemented in the following patch 'block-stream:
> add compress option'.
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> block/io.c | 21 ++++++++++++++++-----
> block/trace-events | 2 +-
> 2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index f8c3596..a7cd24f 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1264,12 +1264,13 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
> * allocating cluster in the image file. Note that this value may exceed
> * BDRV_REQUEST_MAX_BYTES (even when the original read did not), which
> * is one reason we loop rather than doing it all at once.
> + * Also, this is crucial for compressed copy-on-read.
> */
> bdrv_round_to_clusters(bs, offset, bytes, &cluster_offset, &cluster_bytes);
> skip_bytes = offset - cluster_offset;
>
> trace_bdrv_co_do_copy_on_readv(bs, offset, bytes,
> - cluster_offset, cluster_bytes);
> + cluster_offset, cluster_bytes, flags);
>
> while (cluster_bytes) {
> int64_t pnum;
> @@ -1328,9 +1329,15 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
> /* This does not change the data on the disk, it is not
> * necessary to flush even in cache=writethrough mode.
> */
> - ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
> - &local_qiov, 0,
> - BDRV_REQ_WRITE_UNCHANGED);
> + if (flags & BDRV_REQ_WRITE_COMPRESSED) {
> + ret = bdrv_driver_pwritev_compressed(bs, cluster_offset,
> + pnum, &local_qiov,
> + qiov_offset);
qiov_offset is wrong: you should use 0 together with local_qiov, as local_qiov is buffer with
data to be written.
> + } else {
> + ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
> + &local_qiov, 0,
> + BDRV_REQ_WRITE_UNCHANGED);
> + }
> }
>
> if (ret < 0) {
> @@ -1396,7 +1403,11 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
> * to pass through to drivers. For now, there aren't any
> * passthrough flags. */
> assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
> - BDRV_REQ_PREFETCH)));
> + BDRV_REQ_PREFETCH | BDRV_REQ_WRITE_COMPRESSED)));
> +
> + /* write compressed only makes sense with copy on read */
> + assert(!(flags & BDRV_REQ_WRITE_COMPRESSED) ||
> + (flags & BDRV_REQ_COPY_ON_READ));
>
> /* Handle Copy on Read and associated serialisation */
> if (flags & BDRV_REQ_COPY_ON_READ) {
> diff --git a/block/trace-events b/block/trace-events
> index 3aa27e6..f444548 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -14,7 +14,7 @@ blk_root_detach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
> bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
> bdrv_co_pwritev(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
> bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int count, int flags) "bs %p offset %"PRId64" count %d flags 0x%x"
> -bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t cluster_offset, int64_t cluster_bytes) "bs %p offset %"PRId64" bytes %u cluster_offset %"PRId64" cluster_bytes %"PRId64
> +bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t cluster_offset, int64_t cluster_bytes, int flags) "bs %p offset %"PRId64" bytes %u cluster_offset %"PRId64" cluster_bytes %"PRId64" flags 0x%x"
> bdrv_co_copy_range_from(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int read_flags, int write_flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" rw flags 0x%x 0x%x"
> bdrv_co_copy_range_to(void *src, uint64_t src_offset, void *dst, uint64_t dst_offset, uint64_t bytes, int read_flags, int write_flags) "src %p offset %"PRIu64" dst %p offset %"PRIu64" bytes %"PRIu64" rw flags 0x%x 0x%x"
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 5/6] block-stream: add compress option
2019-10-02 14:22 ` [PATCH v2 5/6] block-stream: add compress option Andrey Shinkevich
@ 2019-10-03 14:48 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-10-03 14:48 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-devel, qemu-block
Cc: kwolf, fam, Denis Lunev, jsnow, armbru, dgilbert, stefanha, mreitz
02.10.2019 17:22, Andrey Shinkevich wrote:
> Allow data compression during block-stream job for backup backing chain.
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> block/stream.c | 19 +++++++++++++------
> blockdev.c | 14 +++++++++++++-
> hmp-commands.hx | 4 ++--
> include/block/block_int.h | 3 ++-
> monitor/hmp-cmds.c | 5 +++--
> qapi/block-core.json | 5 ++++-
> 6 files changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index 5562ccb..51cc49e 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -36,15 +36,21 @@ typedef struct StreamBlockJob {
> char *backing_file_str;
> bool bs_read_only;
> bool chain_frozen;
> + bool compress;
> } StreamBlockJob;
>
> -static int coroutine_fn stream_populate(BlockBackend *blk,
> - int64_t offset, uint64_t bytes)
> +static int coroutine_fn stream_populate(BlockBackend *blk, int64_t offset,
> + uint64_t bytes, bool compress)
> {
> + int flags = BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH;
better: BdrvRequestFlags flags = ...
> +
> + if (compress) {
> + flags |= BDRV_REQ_WRITE_COMPRESSED;
> + }
> +
> assert(bytes < SIZE_MAX);
>
> - return blk_co_preadv(blk, offset, bytes, NULL,
> - BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH);
> + return blk_co_preadv(blk, offset, bytes, NULL, flags);
> }
>
> static void stream_abort(Job *job)
> @@ -167,7 +173,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
> }
> trace_stream_one_iteration(s, offset, n, ret);
> if (copy) {
> - ret = stream_populate(blk, offset, n);
> + ret = stream_populate(blk, offset, n, s->compress);
> }
> if (ret < 0) {
> BlockErrorAction action =
> @@ -217,7 +223,7 @@ static const BlockJobDriver stream_job_driver = {
>
> void stream_start(const char *job_id, BlockDriverState *bs,
> BlockDriverState *base, const char *backing_file_str,
> - int creation_flags, int64_t speed,
> + int creation_flags, int64_t speed, bool compress,
> BlockdevOnError on_error, Error **errp)
> {
> StreamBlockJob *s;
> @@ -267,6 +273,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
> s->backing_file_str = g_strdup(backing_file_str);
> s->bs_read_only = bs_read_only;
> s->chain_frozen = true;
> + s->compress = compress;
I'd prefer check for compress support to be in this function, not in the caller.
>
> s->on_error = on_error;
> trace_stream_start(bs, base, s);
> diff --git a/blockdev.c b/blockdev.c
> index fbef684..290ee4b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3238,6 +3238,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
> bool has_base_node, const char *base_node,
> bool has_backing_file, const char *backing_file,
> bool has_speed, int64_t speed,
> + bool has_compress, bool compress,
> bool has_on_error, BlockdevOnError on_error,
> bool has_auto_finalize, bool auto_finalize,
> bool has_auto_dismiss, bool auto_dismiss,
> @@ -3254,6 +3255,10 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
> on_error = BLOCKDEV_ON_ERROR_REPORT;
> }
>
> + if (!has_compress) {
> + compress = false;
> + }
This is extra thing: it's guaranteed for it to be false, if it is absent.
> +
> bs = bdrv_lookup_bs(device, device, errp);
> if (!bs) {
> return;
> @@ -3308,6 +3313,12 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
> goto out;
> }
>
> + if (compress && bs->drv->bdrv_co_pwritev_compressed_part == NULL) {
> + error_setg(errp, "Compression is not supported for this drive %s",
> + bdrv_get_device_name(bs));
> + goto out;
> + }
> +
> /* backing_file string overrides base bs filename */
> base_name = has_backing_file ? backing_file : base_name;
>
> @@ -3319,7 +3330,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
> }
>
> stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
> - job_flags, has_speed ? speed : 0, on_error, &local_err);
> + job_flags, has_speed ? speed : 0, compress, on_error,
> + &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> goto out;
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index cfcc044..3a347fd 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -95,8 +95,8 @@ ETEXI
>
> {
> .name = "block_stream",
> - .args_type = "device:B,speed:o?,base:s?",
> - .params = "device [speed [base]]",
> + .args_type = "device:B,speed:o?,base:s?,compress:o?",
> + .params = "device [speed [base]] [compress]",
> .help = "copy data from a backing file into a block device",
> .cmd = hmp_block_stream,
> },
I'm not a fan of contributing into hmp, and I don't remember the syntax of this file,
but I think it's wrong to define boolean field like interger speed...
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 0422acd..5e7fce8 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1065,6 +1065,7 @@ int is_windows_drive(const char *filename);
> * @creation_flags: Flags that control the behavior of the Job lifetime.
> * See @BlockJobCreateFlags
> * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> + * @compress: True to compress data.
> * @on_error: The action to take upon error.
> * @errp: Error object.
> *
> @@ -1077,7 +1078,7 @@ int is_windows_drive(const char *filename);
> */
> void stream_start(const char *job_id, BlockDriverState *bs,
> BlockDriverState *base, const char *backing_file_str,
> - int creation_flags, int64_t speed,
> + int creation_flags, int64_t speed, bool compress,
> BlockdevOnError on_error, Error **errp);
>
> /**
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index b2551c1..91201fe 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -2025,11 +2025,12 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
> const char *device = qdict_get_str(qdict, "device");
> const char *base = qdict_get_try_str(qdict, "base");
> int64_t speed = qdict_get_try_int(qdict, "speed", 0);
> + bool compress = qdict_get_try_bool(qdict, "compress", false);
>
> qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
> false, NULL, qdict_haskey(qdict, "speed"), speed, true,
> - BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
> - &error);
> + compress, true, BLOCKDEV_ON_ERROR_REPORT,
> + false, false, false, false, &error);
>
> hmp_handle_error(mon, &error);
> }
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e6edd64..9c2093e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2544,6 +2544,9 @@
> #
> # @speed: the maximum speed, in bytes per second
> #
> +# @compress: true to compress data, if the target format supports it
> +# (default: false). Since 4.2.
> +#
> # @on-error: the action to take on an error (default report).
> # 'stop' and 'enospc' can only be used if the block device
> # supports io-status (see BlockInfo). Since 1.3.
> @@ -2576,7 +2579,7 @@
> { 'command': 'block-stream',
> 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
> '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
> - '*on-error': 'BlockdevOnError',
> + '*compress': 'bool', '*on-error': 'BlockdevOnError',
> '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>
> ##
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 6/6] tests/qemu-iotests: add case for block-stream compress
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
0 siblings, 1 reply; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-10-03 14:58 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-devel, qemu-block
Cc: kwolf, fam, Denis Lunev, jsnow, armbru, dgilbert, stefanha, mreitz
02.10.2019 17:22, Andrey Shinkevich wrote:
> Add a test case to the iotest #030 that checks 'compress' option for a
> block-stream job.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> tests/qemu-iotests/030 | 49 +++++++++++++++++++++++++++++++++++++++++++++-
> tests/qemu-iotests/030.out | 4 ++--
> 2 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index f3766f2..13fe5a2 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -21,7 +21,8 @@
> import time
> import os
> import iotests
> -from iotests import qemu_img, qemu_io
> +from iotests import qemu_img, qemu_io, qemu_img_pipe
> +import json
>
> backing_img = os.path.join(iotests.test_dir, 'backing.img')
> mid_img = os.path.join(iotests.test_dir, 'mid.img')
> @@ -956,6 +957,52 @@ class TestSetSpeed(iotests.QMPTestCase):
>
> self.cancel_and_wait(resume=True)
>
> +class TestCompressed(iotests.QMPTestCase):
> +
> + def setUp(self):
> + qemu_img('create', '-f', iotests.imgfmt, backing_img, '1M')
> + qemu_img('create', '-f', iotests.imgfmt, '-o',
> + 'backing_file=%s' % backing_img, mid_img)
> + qemu_img('create', '-f', iotests.imgfmt, '-o',
> + 'backing_file=%s' % mid_img, test_img)
> + qemu_io('-c', 'write -P 0x1 0 512k', backing_img)
> + self.vm = iotests.VM().add_drive(test_img, "backing.node-name=mid," +
> + "backing.backing.node-name=base")
> + self.vm.launch()
Why you can't just add a test-case to TestSingleDrive class?
> +
> + def tearDown(self):
> + self.vm.shutdown()
> + os.remove(test_img)
> + os.remove(mid_img)
> + os.remove(backing_img)
> +
> + def test_stream_compress(self):
> + self.assert_no_active_block_jobs()
> +
> + result = self.vm.qmp('block-stream', device='mid', job_id='stream-mid')
> + self.assert_qmp(result, 'return', {})
> +
> + self.wait_until_completed(drive='stream-mid')
> + for event in self.vm.get_qmp_events(wait=True):
> + if event['event'] == 'BLOCK_JOB_COMPLETED':
> + self.dictpath(event, 'data/device')
> + self.assert_qmp_absent(event, 'data/error')
COMPLETED event is for sure already waited by wait_until_completed
> +
> + result = self.vm.qmp('block-stream', device='drive0', base=mid_img,
> + job_id='stream-top', compress=True)
> + self.assert_qmp(result, 'return', {})
> +
> + self.wait_until_completed(drive='stream-top')
> + self.assert_no_active_block_jobs()
this assertion is done in wait_until_completed
> + self.vm.shutdown()
> +
> + top = json.loads(qemu_img_pipe('info', '--output=json', test_img))
> + mid = json.loads(qemu_img_pipe('info', '--output=json', mid_img))
> + base = json.loads(qemu_img_pipe('info', '--output=json', backing_img))
> +
> + self.assertEqual(mid['actual-size'], base['actual-size'])
> + self.assertLess(top['actual-size'], mid['actual-size'])
> +
> if __name__ == '__main__':
> iotests.main(supported_fmts=['qcow2', 'qed'],
> supported_protocols=['file'])
> diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
> index 6d9bee1..af8dac1 100644
> --- a/tests/qemu-iotests/030.out
> +++ b/tests/qemu-iotests/030.out
> @@ -1,5 +1,5 @@
> -...........................
> +............................
> ----------------------------------------------------------------------
> -Ran 27 tests
> +Ran 28 tests
>
> OK
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/6] qemu-nbd: add compression flag support
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
1 sibling, 1 reply; 21+ messages in thread
From: Roman Kagan @ 2019-10-04 10:19 UTC (permalink / raw)
To: Andrey Shinkevich
Cc: kwolf, fam, Vladimir Sementsov-Ogievskiy, Denis Lunev,
qemu-block, qemu-devel, armbru, stefanha, mreitz, jsnow,
dgilbert
On Wed, Oct 02, 2019 at 05:22:43PM +0300, Andrey Shinkevich wrote:
> Added possibility to write compressed data by using the
> blk_write_compressed. This action has the limitations of the format
> driver. For example we can't write compressed data over other.
>
> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
> $ sudo ./qemu-nbd -c /dev/nbd0 ./image.qcow2
> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
> 10+0 records in
> 10+0 records out
> 104857600 bytes (105 MB) copied, 0,0890581 s, 1,2 GB/s
> $ sudo ./qemu-nbd -d /dev/nbd0
> $ du -sh ./image.qcow2
> 101M ./image.qcow2
>
> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
> $ sudo ./qemu-nbd -C -c /dev/nbd0 ./image.qcow2
> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
> 10+0 records in
> 10+0 records out
> 104857600 bytes (105 MB) copied, 0,076046 s, 1,4 GB/s
> $ sudo ./qemu-nbd -d /dev/nbd0
> $ du -sh ./image.qcow2
> 5,3M ./image.qcow2
>
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> blockdev-nbd.c | 8 ++++++--
> include/block/nbd.h | 11 +++++++++--
> nbd/server.c | 14 ++++++++++----
> qemu-nbd.c | 30 ++++++++++++++++++++++++++----
> qemu-nbd.texi | 2 ++
> 5 files changed, 53 insertions(+), 12 deletions(-)
>
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 6a8b206..e83036b 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -151,6 +151,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
> BlockBackend *on_eject_blk;
> NBDExport *exp;
> int64_t len;
> + uint32_t iflags = 0;
> AioContext *aio_context;
>
> if (!nbd_server) {
> @@ -189,9 +190,12 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
> if (bdrv_is_read_only(bs)) {
> writable = false;
> }
> + if (!writable) {
> + iflags = NBD_INTERNAL_FLAG_READONLY | NBD_INTERNAL_FLAG_SHARED;
> + }
>
> - exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable,
> - NULL, false, on_eject_blk, errp);
> + exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, iflags, NULL,
> + on_eject_blk, errp);
> if (!exp) {
> goto out;
> }
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 316fd70..80be9d5 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -25,6 +25,13 @@
> #include "crypto/tlscreds.h"
> #include "qapi/error.h"
>
> +enum {
> + NBD_INTERNAL_FLAG_READONLY = 1 << 0, /* Device is read-only */
> + NBD_INTERNAL_FLAG_SHARED = 1 << 1, /* Device can be shared */
> + NBD_INTERNAL_FLAG_WRITETHROUGH = 1 << 2, /* Enable write cache */
> + NBD_INTERNAL_FLAG_COMPRESS = 1 << 3, /* Use compressed write */
> +};
> +
> /* Handshake phase structs - this struct is passed on the wire */
>
> struct NBDOption {
> @@ -330,8 +337,8 @@ typedef struct NBDClient NBDClient;
>
> NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> uint64_t size, const char *name, const char *desc,
> - const char *bitmap, bool readonly, bool shared,
> - void (*close)(NBDExport *), bool writethrough,
> + const char *bitmap, uint32_t iflags,
> + void (*close)(NBDExport *),
> BlockBackend *on_eject_blk, Error **errp);
> void nbd_export_close(NBDExport *exp);
> void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
> diff --git a/nbd/server.c b/nbd/server.c
> index d8d1e62..cf3b75d 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -91,6 +91,7 @@ struct NBDExport {
> uint16_t nbdflags;
> QTAILQ_HEAD(, NBDClient) clients;
> QTAILQ_ENTRY(NBDExport) next;
> + uint32_t iflags;
>
> AioContext *ctx;
>
> @@ -1471,13 +1472,14 @@ static void nbd_eject_notifier(Notifier *n, void *data)
>
> NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> uint64_t size, const char *name, const char *desc,
> - const char *bitmap, bool readonly, bool shared,
> - void (*close)(NBDExport *), bool writethrough,
> + const char *bitmap, uint32_t iflags,
> + void (*close)(NBDExport *),
> BlockBackend *on_eject_blk, Error **errp)
> {
> AioContext *ctx;
> BlockBackend *blk;
> NBDExport *exp = g_new0(NBDExport, 1);
> + bool readonly = iflags & NBD_INTERNAL_FLAG_READONLY;
> uint64_t perm;
> int ret;
>
> @@ -1504,7 +1506,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> if (ret < 0) {
> goto fail;
> }
> - blk_set_enable_write_cache(blk, !writethrough);
> + blk_set_enable_write_cache(blk, !(iflags & NBD_INTERNAL_FLAG_WRITETHROUGH));
> blk_set_allow_aio_context_change(blk, true);
>
> exp->refcount = 1;
> @@ -1518,13 +1520,14 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE);
> if (readonly) {
> exp->nbdflags |= NBD_FLAG_READ_ONLY;
> - if (shared) {
> + if (iflags & NBD_INTERNAL_FLAG_SHARED) {
> exp->nbdflags |= NBD_FLAG_CAN_MULTI_CONN;
> }
> } else {
> exp->nbdflags |= (NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES |
> NBD_FLAG_SEND_FAST_ZERO);
> }
> + exp->iflags = iflags;
> assert(size <= INT64_MAX - dev_offset);
> exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
>
> @@ -2312,6 +2315,9 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
> if (request->flags & NBD_CMD_FLAG_FUA) {
> flags |= BDRV_REQ_FUA;
> }
> + if (exp->iflags & NBD_INTERNAL_FLAG_COMPRESS) {
> + flags |= BDRV_REQ_WRITE_COMPRESSED;
> + }
> ret = blk_pwrite(exp->blk, request->from + exp->dev_offset,
> data, request->len, flags);
> return nbd_send_generic_reply(client, request->handle, ret,
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 9032b6d..4163135 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -139,6 +139,7 @@ static void usage(const char *name)
> " --discard=MODE set discard mode (ignore, unmap)\n"
> " --detect-zeroes=MODE set detect-zeroes mode (off, on, unmap)\n"
> " --image-opts treat FILE as a full set of image options\n"
> +" -C, --compress use data compression (if the target format supports it)\n"
> "\n"
> QEMU_HELP_BOTTOM "\n"
> , name, name, NBD_DEFAULT_PORT, "DEVICE");
> @@ -602,6 +603,7 @@ int main(int argc, char **argv)
> BlockDriverState *bs;
> uint64_t dev_offset = 0;
> bool readonly = false;
> + uint32_t iflags = 0;
> bool disconnect = false;
> const char *bindto = NULL;
> const char *port = NULL;
> @@ -610,7 +612,7 @@ int main(int argc, char **argv)
> int64_t fd_size;
> QemuOpts *sn_opts = NULL;
> const char *sn_id_or_name = NULL;
> - const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:L";
> + const char *sopt = "hVb:o:p:rsnCP:c:dvk:e:f:tl:x:T:D:B:L";
> struct option lopt[] = {
> { "help", no_argument, NULL, 'h' },
> { "version", no_argument, NULL, 'V' },
> @@ -619,6 +621,7 @@ int main(int argc, char **argv)
> { "socket", required_argument, NULL, 'k' },
> { "offset", required_argument, NULL, 'o' },
> { "read-only", no_argument, NULL, 'r' },
> + { "compress", no_argument, NULL, 'C'},
> { "partition", required_argument, NULL, 'P' },
> { "bitmap", required_argument, NULL, 'B' },
> { "connect", required_argument, NULL, 'c' },
> @@ -786,6 +789,9 @@ int main(int argc, char **argv)
> readonly = true;
> flags &= ~BDRV_O_RDWR;
> break;
> + case 'C':
> + iflags |= NBD_INTERNAL_FLAG_COMPRESS;
> + break;
> case 'P':
> warn_report("The '-P' option is deprecated; use --image-opts with "
> "a raw device wrapper for subset exports instead");
> @@ -1117,6 +1123,14 @@ int main(int argc, char **argv)
>
> blk_set_enable_write_cache(blk, !writethrough);
>
> + if ((iflags & NBD_INTERNAL_FLAG_COMPRESS) &&
> + !(bs->drv && bs->drv->bdrv_co_pwritev_compressed_part))
> + {
> + error_report("Compression not supported for this file format %s",
> + argv[optind]);
> + exit(EXIT_FAILURE);
> + }
> +
> if (sn_opts) {
> ret = bdrv_snapshot_load_tmp(bs,
> qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
> @@ -1173,10 +1187,18 @@ int main(int argc, char **argv)
> fd_size = limit;
> }
>
> + if (writethrough) {
> + iflags |= NBD_INTERNAL_FLAG_WRITETHROUGH;
> + }
> + if (readonly) {
> + iflags |= NBD_INTERNAL_FLAG_READONLY;
> + }
> + if (shared > 1) {
> + iflags |= NBD_INTERNAL_FLAG_SHARED;
> + }
> export = nbd_export_new(bs, dev_offset, fd_size, export_name,
> - export_description, bitmap, readonly, shared > 1,
> - nbd_export_closed, writethrough, NULL,
> - &error_fatal);
> + export_description, bitmap, iflags,
> + nbd_export_closed, NULL, &error_fatal);
>
> if (device) {
> #if HAVE_NBD_DEVICE
> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index 7f55657..26ea1ec 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -55,6 +55,8 @@ Force the use of the block driver for format @var{fmt} instead of
> auto-detecting.
> @item -r, --read-only
> Export the disk as read-only.
> +@item -C, --compress
> +Use data compression (if the target format supports it)
> @item -P, --partition=@var{num}
> Deprecated: Only expose MBR partition @var{num}. Understands physical
> partitions 1-4 and logical partition 5. New code should instead use
I wonder if we're better off adding a generic blockdev option instead,
so that all tools can benefit from it?
Roman.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/6] qemu-nbd: add compression flag support
2019-10-04 10:19 ` Roman Kagan
@ 2019-10-07 11:52 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-10-07 11:52 UTC (permalink / raw)
To: Roman Kagan, Andrey Shinkevich
Cc: kwolf, fam, Denis Lunev, qemu-block, qemu-devel, armbru,
stefanha, mreitz, jsnow, dgilbert
04.10.2019 13:19, Roman Kagan wrote:
> On Wed, Oct 02, 2019 at 05:22:43PM +0300, Andrey Shinkevich wrote:
>> Added possibility to write compressed data by using the
>> blk_write_compressed. This action has the limitations of the format
>> driver. For example we can't write compressed data over other.
>>
>> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
>> $ sudo ./qemu-nbd -c /dev/nbd0 ./image.qcow2
>> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
>> 10+0 records in
>> 10+0 records out
>> 104857600 bytes (105 MB) copied, 0,0890581 s, 1,2 GB/s
>> $ sudo ./qemu-nbd -d /dev/nbd0
>> $ du -sh ./image.qcow2
>> 101M ./image.qcow2
>>
>> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
>> $ sudo ./qemu-nbd -C -c /dev/nbd0 ./image.qcow2
>> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
>> 10+0 records in
>> 10+0 records out
>> 104857600 bytes (105 MB) copied, 0,076046 s, 1,4 GB/s
>> $ sudo ./qemu-nbd -d /dev/nbd0
>> $ du -sh ./image.qcow2
>> 5,3M ./image.qcow2
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> blockdev-nbd.c | 8 ++++++--
>> include/block/nbd.h | 11 +++++++++--
>> nbd/server.c | 14 ++++++++++----
>> qemu-nbd.c | 30 ++++++++++++++++++++++++++----
>> qemu-nbd.texi | 2 ++
>> 5 files changed, 53 insertions(+), 12 deletions(-)
>>
>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>> index 6a8b206..e83036b 100644
>> --- a/blockdev-nbd.c
>> +++ b/blockdev-nbd.c
>> @@ -151,6 +151,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>> BlockBackend *on_eject_blk;
>> NBDExport *exp;
>> int64_t len;
>> + uint32_t iflags = 0;
>> AioContext *aio_context;
>>
>> if (!nbd_server) {
>> @@ -189,9 +190,12 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>> if (bdrv_is_read_only(bs)) {
>> writable = false;
>> }
>> + if (!writable) {
>> + iflags = NBD_INTERNAL_FLAG_READONLY | NBD_INTERNAL_FLAG_SHARED;
>> + }
>>
>> - exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable,
>> - NULL, false, on_eject_blk, errp);
>> + exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, iflags, NULL,
>> + on_eject_blk, errp);
>> if (!exp) {
>> goto out;
>> }
>> diff --git a/include/block/nbd.h b/include/block/nbd.h
>> index 316fd70..80be9d5 100644
>> --- a/include/block/nbd.h
>> +++ b/include/block/nbd.h
>> @@ -25,6 +25,13 @@
>> #include "crypto/tlscreds.h"
>> #include "qapi/error.h"
>>
>> +enum {
>> + NBD_INTERNAL_FLAG_READONLY = 1 << 0, /* Device is read-only */
>> + NBD_INTERNAL_FLAG_SHARED = 1 << 1, /* Device can be shared */
>> + NBD_INTERNAL_FLAG_WRITETHROUGH = 1 << 2, /* Enable write cache */
>> + NBD_INTERNAL_FLAG_COMPRESS = 1 << 3, /* Use compressed write */
>> +};
>> +
>> /* Handshake phase structs - this struct is passed on the wire */
>>
>> struct NBDOption {
>> @@ -330,8 +337,8 @@ typedef struct NBDClient NBDClient;
>>
>> NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>> uint64_t size, const char *name, const char *desc,
>> - const char *bitmap, bool readonly, bool shared,
>> - void (*close)(NBDExport *), bool writethrough,
>> + const char *bitmap, uint32_t iflags,
>> + void (*close)(NBDExport *),
>> BlockBackend *on_eject_blk, Error **errp);
>> void nbd_export_close(NBDExport *exp);
>> void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
>> diff --git a/nbd/server.c b/nbd/server.c
>> index d8d1e62..cf3b75d 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -91,6 +91,7 @@ struct NBDExport {
>> uint16_t nbdflags;
>> QTAILQ_HEAD(, NBDClient) clients;
>> QTAILQ_ENTRY(NBDExport) next;
>> + uint32_t iflags;
>>
>> AioContext *ctx;
>>
>> @@ -1471,13 +1472,14 @@ static void nbd_eject_notifier(Notifier *n, void *data)
>>
>> NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>> uint64_t size, const char *name, const char *desc,
>> - const char *bitmap, bool readonly, bool shared,
>> - void (*close)(NBDExport *), bool writethrough,
>> + const char *bitmap, uint32_t iflags,
>> + void (*close)(NBDExport *),
>> BlockBackend *on_eject_blk, Error **errp)
>> {
>> AioContext *ctx;
>> BlockBackend *blk;
>> NBDExport *exp = g_new0(NBDExport, 1);
>> + bool readonly = iflags & NBD_INTERNAL_FLAG_READONLY;
>> uint64_t perm;
>> int ret;
>>
>> @@ -1504,7 +1506,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>> if (ret < 0) {
>> goto fail;
>> }
>> - blk_set_enable_write_cache(blk, !writethrough);
>> + blk_set_enable_write_cache(blk, !(iflags & NBD_INTERNAL_FLAG_WRITETHROUGH));
>> blk_set_allow_aio_context_change(blk, true);
>>
>> exp->refcount = 1;
>> @@ -1518,13 +1520,14 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>> NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE);
>> if (readonly) {
>> exp->nbdflags |= NBD_FLAG_READ_ONLY;
>> - if (shared) {
>> + if (iflags & NBD_INTERNAL_FLAG_SHARED) {
>> exp->nbdflags |= NBD_FLAG_CAN_MULTI_CONN;
>> }
>> } else {
>> exp->nbdflags |= (NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES |
>> NBD_FLAG_SEND_FAST_ZERO);
>> }
>> + exp->iflags = iflags;
>> assert(size <= INT64_MAX - dev_offset);
>> exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
>>
>> @@ -2312,6 +2315,9 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
>> if (request->flags & NBD_CMD_FLAG_FUA) {
>> flags |= BDRV_REQ_FUA;
>> }
>> + if (exp->iflags & NBD_INTERNAL_FLAG_COMPRESS) {
>> + flags |= BDRV_REQ_WRITE_COMPRESSED;
>> + }
>> ret = blk_pwrite(exp->blk, request->from + exp->dev_offset,
>> data, request->len, flags);
>> return nbd_send_generic_reply(client, request->handle, ret,
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 9032b6d..4163135 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -139,6 +139,7 @@ static void usage(const char *name)
>> " --discard=MODE set discard mode (ignore, unmap)\n"
>> " --detect-zeroes=MODE set detect-zeroes mode (off, on, unmap)\n"
>> " --image-opts treat FILE as a full set of image options\n"
>> +" -C, --compress use data compression (if the target format supports it)\n"
>> "\n"
>> QEMU_HELP_BOTTOM "\n"
>> , name, name, NBD_DEFAULT_PORT, "DEVICE");
>> @@ -602,6 +603,7 @@ int main(int argc, char **argv)
>> BlockDriverState *bs;
>> uint64_t dev_offset = 0;
>> bool readonly = false;
>> + uint32_t iflags = 0;
>> bool disconnect = false;
>> const char *bindto = NULL;
>> const char *port = NULL;
>> @@ -610,7 +612,7 @@ int main(int argc, char **argv)
>> int64_t fd_size;
>> QemuOpts *sn_opts = NULL;
>> const char *sn_id_or_name = NULL;
>> - const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:L";
>> + const char *sopt = "hVb:o:p:rsnCP:c:dvk:e:f:tl:x:T:D:B:L";
>> struct option lopt[] = {
>> { "help", no_argument, NULL, 'h' },
>> { "version", no_argument, NULL, 'V' },
>> @@ -619,6 +621,7 @@ int main(int argc, char **argv)
>> { "socket", required_argument, NULL, 'k' },
>> { "offset", required_argument, NULL, 'o' },
>> { "read-only", no_argument, NULL, 'r' },
>> + { "compress", no_argument, NULL, 'C'},
>> { "partition", required_argument, NULL, 'P' },
>> { "bitmap", required_argument, NULL, 'B' },
>> { "connect", required_argument, NULL, 'c' },
>> @@ -786,6 +789,9 @@ int main(int argc, char **argv)
>> readonly = true;
>> flags &= ~BDRV_O_RDWR;
>> break;
>> + case 'C':
>> + iflags |= NBD_INTERNAL_FLAG_COMPRESS;
>> + break;
>> case 'P':
>> warn_report("The '-P' option is deprecated; use --image-opts with "
>> "a raw device wrapper for subset exports instead");
>> @@ -1117,6 +1123,14 @@ int main(int argc, char **argv)
>>
>> blk_set_enable_write_cache(blk, !writethrough);
>>
>> + if ((iflags & NBD_INTERNAL_FLAG_COMPRESS) &&
>> + !(bs->drv && bs->drv->bdrv_co_pwritev_compressed_part))
>> + {
>> + error_report("Compression not supported for this file format %s",
>> + argv[optind]);
>> + exit(EXIT_FAILURE);
>> + }
>> +
>> if (sn_opts) {
>> ret = bdrv_snapshot_load_tmp(bs,
>> qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
>> @@ -1173,10 +1187,18 @@ int main(int argc, char **argv)
>> fd_size = limit;
>> }
>>
>> + if (writethrough) {
>> + iflags |= NBD_INTERNAL_FLAG_WRITETHROUGH;
>> + }
>> + if (readonly) {
>> + iflags |= NBD_INTERNAL_FLAG_READONLY;
>> + }
>> + if (shared > 1) {
>> + iflags |= NBD_INTERNAL_FLAG_SHARED;
>> + }
>> export = nbd_export_new(bs, dev_offset, fd_size, export_name,
>> - export_description, bitmap, readonly, shared > 1,
>> - nbd_export_closed, writethrough, NULL,
>> - &error_fatal);
>> + export_description, bitmap, iflags,
>> + nbd_export_closed, NULL, &error_fatal);
>>
>> if (device) {
>> #if HAVE_NBD_DEVICE
>> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
>> index 7f55657..26ea1ec 100644
>> --- a/qemu-nbd.texi
>> +++ b/qemu-nbd.texi
>> @@ -55,6 +55,8 @@ Force the use of the block driver for format @var{fmt} instead of
>> auto-detecting.
>> @item -r, --read-only
>> Export the disk as read-only.
>> +@item -C, --compress
>> +Use data compression (if the target format supports it)
>> @item -P, --partition=@var{num}
>> Deprecated: Only expose MBR partition @var{num}. Understands physical
>> partitions 1-4 and logical partition 5. New code should instead use
>
> I wonder if we're better off adding a generic blockdev option instead,
> so that all tools can benefit from it?
>
I like the idea. And then we'll deprecate compression option in backup job.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 6/6] tests/qemu-iotests: add case for block-stream compress
2019-10-03 14:58 ` Vladimir Sementsov-Ogievskiy
@ 2019-10-15 17:57 ` Andrey Shinkevich
0 siblings, 0 replies; 21+ messages in thread
From: Andrey Shinkevich @ 2019-10-15 17:57 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Cc: kwolf, fam, Denis Lunev, jsnow, armbru, dgilbert, stefanha, mreitz
On 03/10/2019 17:58, Vladimir Sementsov-Ogievskiy wrote:
> 02.10.2019 17:22, Andrey Shinkevich wrote:
>> Add a test case to the iotest #030 that checks 'compress' option for a
>> block-stream job.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> tests/qemu-iotests/030 | 49 +++++++++++++++++++++++++++++++++++++++++++++-
>> tests/qemu-iotests/030.out | 4 ++--
>> 2 files changed, 50 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>> index f3766f2..13fe5a2 100755
>> --- a/tests/qemu-iotests/030
>> +++ b/tests/qemu-iotests/030
>> @@ -21,7 +21,8 @@
>> import time
>> import os
>> import iotests
>> -from iotests import qemu_img, qemu_io
>> +from iotests import qemu_img, qemu_io, qemu_img_pipe
>> +import json
>>
>> backing_img = os.path.join(iotests.test_dir, 'backing.img')
>> mid_img = os.path.join(iotests.test_dir, 'mid.img')
>> @@ -956,6 +957,52 @@ class TestSetSpeed(iotests.QMPTestCase):
>>
>> self.cancel_and_wait(resume=True)
>>
>> +class TestCompressed(iotests.QMPTestCase):
>> +
>> + def setUp(self):
>> + qemu_img('create', '-f', iotests.imgfmt, backing_img, '1M')
>> + qemu_img('create', '-f', iotests.imgfmt, '-o',
>> + 'backing_file=%s' % backing_img, mid_img)
>> + qemu_img('create', '-f', iotests.imgfmt, '-o',
>> + 'backing_file=%s' % mid_img, test_img)
>> + qemu_io('-c', 'write -P 0x1 0 512k', backing_img)
>> + self.vm = iotests.VM().add_drive(test_img, "backing.node-name=mid," +
>> + "backing.backing.node-name=base")
>> + self.vm.launch()
>
> Why you can't just add a test-case to TestSingleDrive class?
Their setUp() functions differ.
>
>> +
>> + def tearDown(self):
>> + self.vm.shutdown()
>> + os.remove(test_img)
>> + os.remove(mid_img)
>> + os.remove(backing_img)
>> +
>> + def test_stream_compress(self):
>> + self.assert_no_active_block_jobs()
>> +
>> + result = self.vm.qmp('block-stream', device='mid', job_id='stream-mid')
>> + self.assert_qmp(result, 'return', {})
>> +
>> + self.wait_until_completed(drive='stream-mid')
>> + for event in self.vm.get_qmp_events(wait=True):
>> + if event['event'] == 'BLOCK_JOB_COMPLETED':
>> + self.dictpath(event, 'data/device')
>> + self.assert_qmp_absent(event, 'data/error')
>
> COMPLETED event is for sure already waited by wait_until_completed
>
>> +
>> + result = self.vm.qmp('block-stream', device='drive0', base=mid_img,
>> + job_id='stream-top', compress=True)
>> + self.assert_qmp(result, 'return', {})
>> +
>> + self.wait_until_completed(drive='stream-top')
>> + self.assert_no_active_block_jobs()
>
> this assertion is done in wait_until_completed
>
>> + self.vm.shutdown()
>> +
>> + top = json.loads(qemu_img_pipe('info', '--output=json', test_img))
>> + mid = json.loads(qemu_img_pipe('info', '--output=json', mid_img))
>> + base = json.loads(qemu_img_pipe('info', '--output=json', backing_img))
>> +
>> + self.assertEqual(mid['actual-size'], base['actual-size'])
>> + self.assertLess(top['actual-size'], mid['actual-size'])
>> +
>> if __name__ == '__main__':
>> iotests.main(supported_fmts=['qcow2', 'qed'],
>> supported_protocols=['file'])
>> diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
>> index 6d9bee1..af8dac1 100644
>> --- a/tests/qemu-iotests/030.out
>> +++ b/tests/qemu-iotests/030.out
>> @@ -1,5 +1,5 @@
>> -...........................
>> +............................
>> ----------------------------------------------------------------------
>> -Ran 27 tests
>> +Ran 28 tests
>>
>> OK
>>
>
>
--
With the best regards,
Andrey Shinkevich
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-10-15 17:59 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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).