From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"qemu-block@nongnu.org" <qemu-block@nongnu.org>
Cc: "kwolf@redhat.com" <kwolf@redhat.com>,
"fam@euphon.net" <fam@euphon.net>,
Denis Lunev <den@virtuozzo.com>,
"jsnow@redhat.com" <jsnow@redhat.com>,
"armbru@redhat.com" <armbru@redhat.com>,
"dgilbert@redhat.com" <dgilbert@redhat.com>,
"stefanha@redhat.com" <stefanha@redhat.com>,
"mreitz@redhat.com" <mreitz@redhat.com>
Subject: Re: [PATCH v2 3/6] qemu-nbd: add compression flag support
Date: Thu, 3 Oct 2019 14:32:24 +0000 [thread overview]
Message-ID: <f236d4c4-d96e-3980-ac9d-618c2f66c92c@virtuozzo.com> (raw)
In-Reply-To: <1570026166-748566-4-git-send-email-andrey.shinkevich@virtuozzo.com>
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
next prev parent reply other threads:[~2019-10-03 14:34 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-02 14:22 [PATCH v2 0/6] qcow2: advanced compression options Andrey Shinkevich
2019-10-02 14:22 ` [PATCH v2 1/6] qcow2: Allow writing compressed data to multiple clusters Andrey Shinkevich
2019-10-03 14:16 ` Vladimir Sementsov-Ogievskiy
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 [this message]
2019-10-04 10:19 ` Roman Kagan
2019-10-07 11:52 ` Vladimir Sementsov-Ogievskiy
2019-10-02 14:22 ` [PATCH v2 4/6] block: support compressed write for copy-on-read Andrey Shinkevich
2019-10-03 14:37 ` Vladimir Sementsov-Ogievskiy
2019-10-02 14:22 ` [PATCH v2 5/6] block-stream: add compress option Andrey Shinkevich
2019-10-03 14:48 ` Vladimir Sementsov-Ogievskiy
2019-10-02 14:22 ` [PATCH v2 6/6] tests/qemu-iotests: add case for block-stream compress Andrey Shinkevich
2019-10-03 14:58 ` Vladimir Sementsov-Ogievskiy
2019-10-15 17:57 ` Andrey Shinkevich
2019-10-02 15:07 ` [PATCH v2 0/6] qcow2: advanced compression options no-reply
2019-10-02 15:35 ` Vladimir Sementsov-Ogievskiy
2019-10-02 15:58 ` Max Reitz
2019-10-02 19:04 ` Markus Armbruster
2019-10-02 15:07 ` no-reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f236d4c4-d96e-3980-ac9d-618c2f66c92c@virtuozzo.com \
--to=vsementsov@virtuozzo.com \
--cc=andrey.shinkevich@virtuozzo.com \
--cc=armbru@redhat.com \
--cc=den@virtuozzo.com \
--cc=dgilbert@redhat.com \
--cc=fam@euphon.net \
--cc=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).