From: Eric Blake <eblake@redhat.com>
To: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>,
qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: kwolf@redhat.com, fam@euphon.net, vsementsov@virtuozzo.com,
armbru@redhat.com, dgilbert@redhat.com, stefanha@redhat.com,
den@openvz.org, mreitz@redhat.com, jsnow@redhat.com
Subject: Re: [PATCH 4/6] qemu-nbd: add compression flag support
Date: Tue, 1 Oct 2019 15:47:03 -0500 [thread overview]
Message-ID: <4afd7d6c-7e38-c325-b009-c798186715bd@redhat.com> (raw)
In-Reply-To: <1569958040-697220-5-git-send-email-andrey.shinkevich@virtuozzo.com>
On 10/1/19 2:27 PM, 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.
>
> +++ b/blockdev-nbd.c
> @@ -191,7 +191,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
> }
>
> exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable,
> - NULL, false, on_eject_blk, errp);
> + 0, NULL, false, on_eject_blk, errp);
This is a lot of parameters. Should we be combining some of them into a
struct, or even at least the booleans into a flags parameter?
> +++ b/include/block/nbd.h
> @@ -25,6 +25,10 @@
> #include "crypto/tlscreds.h"
> #include "qapi/error.h"
>
> +enum {
> + NBD_INTERNAL_FLAG_COMPRESS = 1 << 1, /* Use write compressed */
> +};
What happened to flag 1 << 0? What other flags do you anticipate adding?
> +++ 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,7 +1472,8 @@ 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,
> + const char *bitmap, bool readonly,
> + bool shared, uint32_t iflags,
> void (*close)(NBDExport *), bool writethrough,
> BlockBackend *on_eject_blk, Error **errp)
Again, this feels like a lot of parameters, combining more through
iflags may make sense.
> {
> @@ -1525,6 +1527,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> 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;
> + }
This unconditionally tries to make all writes compressed if the option
was selected when starting qemu-nbd. Should we at least sanity check
that it will work during nbd_export_new, rather than waiting to find out
on the first (failed) write, whether it actually works?
/me looks ahead [1]
> 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..3765c4b 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"
I'm not necessarily opposed to burning a short option. But it's a shame
that we can't use -c to be similar to 'qemu-img convert -c'. Requiring
the use of a long option is also okay (short options have to be for the
more likely uses, although it does seem like this use case might qualify).
> @@ -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:CrsnP:c:dvk:e:f:tl:x:T:D:B:L";
Pre-existing, but we don't sort this very well.
> 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' },
Above you put 'C' between 'p' and 'r', but here between 'r' and 'P'. We
really don't sort very well :)
> { "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':
At least this matches your lopt[] ordering.
> 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);
> + }
> +
[1] ah, you DO make sure that compression is supported before passing
the option through.
The idea seems reasonable.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
next prev parent reply other threads:[~2019-10-01 21:31 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-01 19:27 [PATCH 0/6] qcow2: advanced compression options Andrey Shinkevich
2019-10-01 19:27 ` [PATCH 1/6] qcow2: multiple clusters write compressed Andrey Shinkevich
2019-10-01 19:27 ` [PATCH 2/6] tests/qemu-iotests: add case of writing compressed data to multiple clusters Andrey Shinkevich
2019-10-01 19:27 ` [PATCH 3/6] block: support compressed write for copy-on-read Andrey Shinkevich
2019-10-01 19:27 ` [PATCH 4/6] qemu-nbd: add compression flag support Andrey Shinkevich
2019-10-01 20:47 ` Eric Blake [this message]
2019-10-02 10:29 ` Vladimir Sementsov-Ogievskiy
2019-10-01 19:27 ` [PATCH 5/6] block-stream: add compress option Andrey Shinkevich
2019-10-01 20:50 ` Eric Blake
2019-10-01 19:27 ` [PATCH 6/6] tests/qemu-iotests: add case for block-stream compress Andrey Shinkevich
2019-10-02 2:23 ` [PATCH 0/6] qcow2: advanced compression options no-reply
2019-10-02 2:25 ` 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=4afd7d6c-7e38-c325-b009-c798186715bd@redhat.com \
--to=eblake@redhat.com \
--cc=andrey.shinkevich@virtuozzo.com \
--cc=armbru@redhat.com \
--cc=den@openvz.org \
--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 \
--cc=vsementsov@virtuozzo.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).