| Message ID | 1569958040-697220-5-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcow2: advanced compression options | expand |
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.
01.10.2019 23:47, Eric Blake wrote: > 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? Hmm, any way, creating flags parameter for only one flag seems useless. I think, I'd prefer to cover all nbd_export_new parameters to a structure with boolean fields, to avoid extra &/| arithmetic. nbd_export_new is called from qmp_nbd_server_add and qemu-nbd main(), and both places seems to benefit, if they will fill structure instead of local variables.
diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 6a8b206..2a01a04 100644 --- a/blockdev-nbd.c +++ 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); if (!exp) { goto out; } diff --git a/include/block/nbd.h b/include/block/nbd.h index 316fd70..0032f73 100644 --- a/include/block/nbd.h +++ 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 */ +}; + /* Handshake phase structs - this struct is passed on the wire */ struct NBDOption { @@ -330,7 +334,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, + const char *bitmap, bool readonly, + bool shared, uint32_t iflags, void (*close)(NBDExport *), bool writethrough, BlockBackend *on_eject_blk, Error **errp); void nbd_export_close(NBDExport *exp); diff --git a/nbd/server.c b/nbd/server.c index d8d1e62..96d581d 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,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) { @@ -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; + } 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" "\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:CrsnP: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), @@ -1175,7 +1189,7 @@ int main(int argc, char **argv) export = nbd_export_new(bs, dev_offset, fd_size, export_name, export_description, bitmap, readonly, shared > 1, - nbd_export_closed, writethrough, NULL, + iflags, nbd_export_closed, writethrough, NULL, &error_fatal); if (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