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
