| Message ID | 20190711150940.17483-1-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] LUKS: support preallocation | expand |
On Thu, Jul 11, 2019 at 06:09:40PM +0300, Maxim Levitsky wrote: > preallocation=off and preallocation=metadata > both allocate luks header only, and preallocation=falloc/full > is passed to underlying file. > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534951 > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > Note that QMP support was only compile tested, since I am still learning > on how to use it. > > If there is some library/script/etc which makes it more high level, > I would more that glad to hear about it. So far I used the qmp-shell > > Also can I use qmp's blockdev-create outside a vm running? > > block/crypto.c | 29 ++++++++++++++++++++++++++--- > qapi/block-core.json | 5 ++++- > 2 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/block/crypto.c b/block/crypto.c > index 8237424ae6..034a645652 100644 > --- a/block/crypto.c > +++ b/block/crypto.c > @@ -74,6 +74,7 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block, > struct BlockCryptoCreateData { > BlockBackend *blk; > uint64_t size; > + PreallocMode prealloc; > }; > > > @@ -112,7 +113,7 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block, > * available to the guest, so we must take account of that > * which will be used by the crypto header > */ > - return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF, > + return blk_truncate(data->blk, data->size + headerlen, data->prealloc, > errp); > } > > @@ -251,6 +252,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format, > static int block_crypto_co_create_generic(BlockDriverState *bs, > int64_t size, > QCryptoBlockCreateOptions *opts, > + PreallocMode prealloc, > Error **errp) > { > int ret; > @@ -266,9 +268,14 @@ static int block_crypto_co_create_generic(BlockDriverState *bs, > goto cleanup; > } > > + if (prealloc == PREALLOC_MODE_METADATA) { > + prealloc = PREALLOC_MODE_OFF; > + } > + > data = (struct BlockCryptoCreateData) { > .blk = blk, > .size = size, > + .prealloc = prealloc, > }; > > crypto = qcrypto_block_create(opts, NULL, > @@ -500,6 +507,7 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) > BlockdevCreateOptionsLUKS *luks_opts; > BlockDriverState *bs = NULL; > QCryptoBlockCreateOptions create_opts; > + PreallocMode preallocation = PREALLOC_MODE_OFF; > int ret; > > assert(create_options->driver == BLOCKDEV_DRIVER_LUKS); > @@ -515,8 +523,11 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) > .u.luks = *qapi_BlockdevCreateOptionsLUKS_base(luks_opts), > }; > > + if (luks_opts->has_preallocation) > + preallocation = luks_opts->preallocation; > + > ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts, > - errp); > + preallocation, errp); > if (ret < 0) { > goto fail; > } > @@ -534,12 +545,24 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, > QCryptoBlockCreateOptions *create_opts = NULL; > BlockDriverState *bs = NULL; > QDict *cryptoopts; > + PreallocMode prealloc; > + char *buf = NULL; > int64_t size; > int ret; > + Error *local_err = NULL; > > /* Parse options */ > size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0); > > + buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); > + prealloc = qapi_enum_parse(&PreallocMode_lookup, buf, > + PREALLOC_MODE_OFF, &local_err); > + g_free(buf); > + if (local_err) { > + error_propagate(errp, local_err); > + return -EINVAL; > + } > + > cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL, > &block_crypto_create_opts_luks, > true); > @@ -565,7 +588,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, > } > > /* Create format layer */ > - ret = block_crypto_co_create_generic(bs, size, create_opts, errp); > + ret = block_crypto_co_create_generic(bs, size, create_opts, prealloc, errp); > if (ret < 0) { > goto fail; > } > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 0d43d4f37c..ebcfc9f903 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -4205,13 +4205,16 @@ > # > # @file Node to create the image format on > # @size Size of the virtual disk in bytes > +# @preallocation Preallocation mode for the new image (default: off; > +# allowed values: off/falloc/full Should we add also "metadata" to allowed values? and "since: 4.2"? I'd like to have (just to have similar documentation with others preallocation parameters): # @preallocation Preallocation mode for the new image (default: off; # allowed values: off, falloc, full, metadata; since: 4.2) Thanks, Stefano
Patchew URL: https://patchew.org/QEMU/20190711150940.17483-1-mlevitsk@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20190711150940.17483-1-mlevitsk@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH v3] LUKS: support preallocation
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Switched to a new branch 'test'
f1b0906cf3 LUKS: support preallocation
=== OUTPUT BEGIN ===
ERROR: braces {} are necessary for all arms of this statement
#73: FILE: block/crypto.c:526:
+ if (luks_opts->has_preallocation)
[...]
total: 1 errors, 0 warnings, 104 lines checked
Commit f1b0906cf3c9 (LUKS: support preallocation) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20190711150940.17483-1-mlevitsk@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Thu, 2019-07-11 at 18:27 +0200, Stefano Garzarella wrote: > On Thu, Jul 11, 2019 at 06:09:40PM +0300, Maxim Levitsky wrote: > > preallocation=off and preallocation=metadata > > both allocate luks header only, and preallocation=falloc/full > > is passed to underlying file. > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534951 > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > > --- > > > > Note that QMP support was only compile tested, since I am still learning > > on how to use it. > > > > If there is some library/script/etc which makes it more high level, > > I would more that glad to hear about it. So far I used the qmp-shell > > > > Also can I use qmp's blockdev-create outside a vm running? > > > > block/crypto.c | 29 ++++++++++++++++++++++++++--- > > qapi/block-core.json | 5 ++++- > > 2 files changed, 30 insertions(+), 4 deletions(-) > > > > diff --git a/block/crypto.c b/block/crypto.c > > index 8237424ae6..034a645652 100644 > > --- a/block/crypto.c > > +++ b/block/crypto.c > > @@ -74,6 +74,7 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block, > > struct BlockCryptoCreateData { > > BlockBackend *blk; > > uint64_t size; > > + PreallocMode prealloc; > > }; > > > > > > @@ -112,7 +113,7 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block, > > * available to the guest, so we must take account of that > > * which will be used by the crypto header > > */ > > - return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF, > > + return blk_truncate(data->blk, data->size + headerlen, data->prealloc, > > errp); > > } > > > > @@ -251,6 +252,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format, > > static int block_crypto_co_create_generic(BlockDriverState *bs, > > int64_t size, > > QCryptoBlockCreateOptions *opts, > > + PreallocMode prealloc, > > Error **errp) > > { > > int ret; > > @@ -266,9 +268,14 @@ static int block_crypto_co_create_generic(BlockDriverState *bs, > > goto cleanup; > > } > > > > + if (prealloc == PREALLOC_MODE_METADATA) { > > + prealloc = PREALLOC_MODE_OFF; > > + } > > + > > data = (struct BlockCryptoCreateData) { > > .blk = blk, > > .size = size, > > + .prealloc = prealloc, > > }; > > > > crypto = qcrypto_block_create(opts, NULL, > > @@ -500,6 +507,7 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) > > BlockdevCreateOptionsLUKS *luks_opts; > > BlockDriverState *bs = NULL; > > QCryptoBlockCreateOptions create_opts; > > + PreallocMode preallocation = PREALLOC_MODE_OFF; > > int ret; > > > > assert(create_options->driver == BLOCKDEV_DRIVER_LUKS); > > @@ -515,8 +523,11 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) > > .u.luks = *qapi_BlockdevCreateOptionsLUKS_base(luks_opts), > > }; > > > > + if (luks_opts->has_preallocation) > > + preallocation = luks_opts->preallocation; > > + > > ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts, > > - errp); > > + preallocation, errp); > > if (ret < 0) { > > goto fail; > > } > > @@ -534,12 +545,24 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, > > QCryptoBlockCreateOptions *create_opts = NULL; > > BlockDriverState *bs = NULL; > > QDict *cryptoopts; > > + PreallocMode prealloc; > > + char *buf = NULL; > > int64_t size; > > int ret; > > + Error *local_err = NULL; > > > > /* Parse options */ > > size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0); > > > > + buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); > > + prealloc = qapi_enum_parse(&PreallocMode_lookup, buf, > > + PREALLOC_MODE_OFF, &local_err); > > + g_free(buf); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return -EINVAL; > > + } > > + > > cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL, > > &block_crypto_create_opts_luks, > > true); > > @@ -565,7 +588,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, > > } > > > > /* Create format layer */ > > - ret = block_crypto_co_create_generic(bs, size, create_opts, errp); > > + ret = block_crypto_co_create_generic(bs, size, create_opts, prealloc, errp); > > if (ret < 0) { > > goto fail; > > } > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > index 0d43d4f37c..ebcfc9f903 100644 > > --- a/qapi/block-core.json > > +++ b/qapi/block-core.json > > @@ -4205,13 +4205,16 @@ > > # > > # @file Node to create the image format on > > # @size Size of the virtual disk in bytes > > +# @preallocation Preallocation mode for the new image (default: off; > > +# allowed values: off/falloc/full > > Should we add also "metadata" to allowed values? and "since: 4.2"? > I'd like to have (just to have similar documentation with others > preallocation parameters): It it support but preallocation=off is the same as preallocation=metadata in luks, as luks only metadata is the header which is created anyway. In some sense I should throw a error for preallocation=off, but I suspect that that will break userspace api. What do you think? I forgot to add 'since: 4.2', will do in the next revision of that patch series. > > # @preallocation Preallocation mode for the new image (default: off; > # allowed values: off, falloc, full, metadata; since: 4.2) > > > Thanks, > Stefano Best regards, Maxim Levitsky
On Sun, Jul 14, 2019 at 05:51:51PM +0300, Maxim Levitsky wrote: > On Thu, 2019-07-11 at 18:27 +0200, Stefano Garzarella wrote: > > On Thu, Jul 11, 2019 at 06:09:40PM +0300, Maxim Levitsky wrote: > > > preallocation=off and preallocation=metadata > > > both allocate luks header only, and preallocation=falloc/full > > > is passed to underlying file. > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534951 > > > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > > > > --- > > > > > > Note that QMP support was only compile tested, since I am still learning > > > on how to use it. > > > > > > If there is some library/script/etc which makes it more high level, > > > I would more that glad to hear about it. So far I used the qmp-shell > > > > > > Also can I use qmp's blockdev-create outside a vm running? > > > > > > block/crypto.c | 29 ++++++++++++++++++++++++++--- > > > qapi/block-core.json | 5 ++++- > > > 2 files changed, 30 insertions(+), 4 deletions(-) > > > > > > diff --git a/block/crypto.c b/block/crypto.c > > > index 8237424ae6..034a645652 100644 > > > --- a/block/crypto.c > > > +++ b/block/crypto.c > > > @@ -74,6 +74,7 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block, > > > struct BlockCryptoCreateData { > > > BlockBackend *blk; > > > uint64_t size; > > > + PreallocMode prealloc; > > > }; > > > > > > > > > @@ -112,7 +113,7 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block, > > > * available to the guest, so we must take account of that > > > * which will be used by the crypto header > > > */ > > > - return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF, > > > + return blk_truncate(data->blk, data->size + headerlen, data->prealloc, > > > errp); > > > } > > > > > > @@ -251,6 +252,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format, > > > static int block_crypto_co_create_generic(BlockDriverState *bs, > > > int64_t size, > > > QCryptoBlockCreateOptions *opts, > > > + PreallocMode prealloc, > > > Error **errp) > > > { > > > int ret; > > > @@ -266,9 +268,14 @@ static int block_crypto_co_create_generic(BlockDriverState *bs, > > > goto cleanup; > > > } > > > > > > + if (prealloc == PREALLOC_MODE_METADATA) { > > > + prealloc = PREALLOC_MODE_OFF; > > > + } > > > + > > > data = (struct BlockCryptoCreateData) { > > > .blk = blk, > > > .size = size, > > > + .prealloc = prealloc, > > > }; > > > > > > crypto = qcrypto_block_create(opts, NULL, > > > @@ -500,6 +507,7 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) > > > BlockdevCreateOptionsLUKS *luks_opts; > > > BlockDriverState *bs = NULL; > > > QCryptoBlockCreateOptions create_opts; > > > + PreallocMode preallocation = PREALLOC_MODE_OFF; > > > int ret; > > > > > > assert(create_options->driver == BLOCKDEV_DRIVER_LUKS); > > > @@ -515,8 +523,11 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) > > > .u.luks = *qapi_BlockdevCreateOptionsLUKS_base(luks_opts), > > > }; > > > > > > + if (luks_opts->has_preallocation) > > > + preallocation = luks_opts->preallocation; > > > + > > > ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts, > > > - errp); > > > + preallocation, errp); > > > if (ret < 0) { > > > goto fail; > > > } > > > @@ -534,12 +545,24 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, > > > QCryptoBlockCreateOptions *create_opts = NULL; > > > BlockDriverState *bs = NULL; > > > QDict *cryptoopts; > > > + PreallocMode prealloc; > > > + char *buf = NULL; > > > int64_t size; > > > int ret; > > > + Error *local_err = NULL; > > > > > > /* Parse options */ > > > size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0); > > > > > > + buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); > > > + prealloc = qapi_enum_parse(&PreallocMode_lookup, buf, > > > + PREALLOC_MODE_OFF, &local_err); > > > + g_free(buf); > > > + if (local_err) { > > > + error_propagate(errp, local_err); > > > + return -EINVAL; > > > + } > > > + > > > cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL, > > > &block_crypto_create_opts_luks, > > > true); > > > @@ -565,7 +588,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, > > > } > > > > > > /* Create format layer */ > > > - ret = block_crypto_co_create_generic(bs, size, create_opts, errp); > > > + ret = block_crypto_co_create_generic(bs, size, create_opts, prealloc, errp); > > > if (ret < 0) { > > > goto fail; > > > } > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > > index 0d43d4f37c..ebcfc9f903 100644 > > > --- a/qapi/block-core.json > > > +++ b/qapi/block-core.json > > > @@ -4205,13 +4205,16 @@ > > > # > > > # @file Node to create the image format on > > > # @size Size of the virtual disk in bytes > > > +# @preallocation Preallocation mode for the new image (default: off; > > > +# allowed values: off/falloc/full > > > > Should we add also "metadata" to allowed values? and "since: 4.2"? > > I'd like to have (just to have similar documentation with others > > preallocation parameters): > > It it support but preallocation=off is the same as preallocation=metadata in luks, > as luks only metadata is the header which is created anyway. > In some sense I should throw a error for preallocation=off, but I suspect that > that will break userspace api. > What do you think? I don't know very well the details, but I agree with you that some user APIs could expect that preallocation=off is always available. Maybe we can write something in the comment (explaining that off and metadata are the same) and make the preallocation=metadata the default choice. Thanks, Stefano
On Mon, 2019-07-15 at 10:30 +0200, Stefano Garzarella wrote: > On Sun, Jul 14, 2019 at 05:51:51PM +0300, Maxim Levitsky wrote: > > On Thu, 2019-07-11 at 18:27 +0200, Stefano Garzarella wrote: > > > On Thu, Jul 11, 2019 at 06:09:40PM +0300, Maxim Levitsky wrote: > > > > preallocation=off and preallocation=metadata > > > > both allocate luks header only, and preallocation=falloc/full > > > > is passed to underlying file. > > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534951 > > > > > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > > > > > > --- > > > > > > > > Note that QMP support was only compile tested, since I am still learning > > > > on how to use it. > > > > > > > > If there is some library/script/etc which makes it more high level, > > > > I would more that glad to hear about it. So far I used the qmp-shell > > > > > > > > Also can I use qmp's blockdev-create outside a vm running? > > > > > > > > block/crypto.c | 29 ++++++++++++++++++++++++++--- > > > > qapi/block-core.json | 5 ++++- > > > > 2 files changed, 30 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/block/crypto.c b/block/crypto.c > > > > index 8237424ae6..034a645652 100644 > > > > --- a/block/crypto.c > > > > +++ b/block/crypto.c > > > > @@ -74,6 +74,7 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block, > > > > struct BlockCryptoCreateData { > > > > BlockBackend *blk; > > > > uint64_t size; > > > > + PreallocMode prealloc; > > > > }; > > > > > > > > > > > > @@ -112,7 +113,7 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block, > > > > * available to the guest, so we must take account of that > > > > * which will be used by the crypto header > > > > */ > > > > - return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF, > > > > + return blk_truncate(data->blk, data->size + headerlen, data->prealloc, > > > > errp); > > > > } > > > > > > > > @@ -251,6 +252,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format, > > > > static int block_crypto_co_create_generic(BlockDriverState *bs, > > > > int64_t size, > > > > QCryptoBlockCreateOptions *opts, > > > > + PreallocMode prealloc, > > > > Error **errp) > > > > { > > > > int ret; > > > > @@ -266,9 +268,14 @@ static int block_crypto_co_create_generic(BlockDriverState *bs, > > > > goto cleanup; > > > > } > > > > > > > > + if (prealloc == PREALLOC_MODE_METADATA) { > > > > + prealloc = PREALLOC_MODE_OFF; > > > > + } > > > > + > > > > data = (struct BlockCryptoCreateData) { > > > > .blk = blk, > > > > .size = size, > > > > + .prealloc = prealloc, > > > > }; > > > > > > > > crypto = qcrypto_block_create(opts, NULL, > > > > @@ -500,6 +507,7 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) > > > > BlockdevCreateOptionsLUKS *luks_opts; > > > > BlockDriverState *bs = NULL; > > > > QCryptoBlockCreateOptions create_opts; > > > > + PreallocMode preallocation = PREALLOC_MODE_OFF; > > > > int ret; > > > > > > > > assert(create_options->driver == BLOCKDEV_DRIVER_LUKS); > > > > @@ -515,8 +523,11 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) > > > > .u.luks = *qapi_BlockdevCreateOptionsLUKS_base(luks_opts), > > > > }; > > > > > > > > + if (luks_opts->has_preallocation) > > > > + preallocation = luks_opts->preallocation; > > > > + > > > > ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts, > > > > - errp); > > > > + preallocation, errp); > > > > if (ret < 0) { > > > > goto fail; > > > > } > > > > @@ -534,12 +545,24 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, > > > > QCryptoBlockCreateOptions *create_opts = NULL; > > > > BlockDriverState *bs = NULL; > > > > QDict *cryptoopts; > > > > + PreallocMode prealloc; > > > > + char *buf = NULL; > > > > int64_t size; > > > > int ret; > > > > + Error *local_err = NULL; > > > > > > > > /* Parse options */ > > > > size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0); > > > > > > > > + buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); > > > > + prealloc = qapi_enum_parse(&PreallocMode_lookup, buf, > > > > + PREALLOC_MODE_OFF, &local_err); > > > > + g_free(buf); > > > > + if (local_err) { > > > > + error_propagate(errp, local_err); > > > > + return -EINVAL; > > > > + } > > > > + > > > > cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL, > > > > &block_crypto_create_opts_luks, > > > > true); > > > > @@ -565,7 +588,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, > > > > } > > > > > > > > /* Create format layer */ > > > > - ret = block_crypto_co_create_generic(bs, size, create_opts, errp); > > > > + ret = block_crypto_co_create_generic(bs, size, create_opts, prealloc, errp); > > > > if (ret < 0) { > > > > goto fail; > > > > } > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > > > index 0d43d4f37c..ebcfc9f903 100644 > > > > --- a/qapi/block-core.json > > > > +++ b/qapi/block-core.json > > > > @@ -4205,13 +4205,16 @@ > > > > # > > > > # @file Node to create the image format on > > > > # @size Size of the virtual disk in bytes > > > > +# @preallocation Preallocation mode for the new image (default: off; > > > > +# allowed values: off/falloc/full > > > > > > Should we add also "metadata" to allowed values? and "since: 4.2"? > > > I'd like to have (just to have similar documentation with others > > > preallocation parameters): > > > > It it support but preallocation=off is the same as preallocation=metadata in luks, > > as luks only metadata is the header which is created anyway. > > In some sense I should throw a error for preallocation=off, but I suspect that > > that will break userspace api. > > What do you think? > > I don't know very well the details, but I agree with you that some > user APIs could expect that preallocation=off is always available. > > Maybe we can write something in the comment (explaining that off and metadata > are the same) and make the preallocation=metadata the default choice. > > Thanks, > Stefano All right! Best regards, Maxim Levitsky
On 15.07.19 10:30, Stefano Garzarella wrote: > On Sun, Jul 14, 2019 at 05:51:51PM +0300, Maxim Levitsky wrote: >> On Thu, 2019-07-11 at 18:27 +0200, Stefano Garzarella wrote: >>> On Thu, Jul 11, 2019 at 06:09:40PM +0300, Maxim Levitsky wrote: >>>> preallocation=off and preallocation=metadata >>>> both allocate luks header only, and preallocation=falloc/full >>>> is passed to underlying file. >>>> >>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534951 >>>> >>>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> >>>> >>>> --- >>>> >>>> Note that QMP support was only compile tested, since I am still learning >>>> on how to use it. >>>> >>>> If there is some library/script/etc which makes it more high level, >>>> I would more that glad to hear about it. So far I used the qmp-shell >>>> >>>> Also can I use qmp's blockdev-create outside a vm running? >>>> >>>> block/crypto.c | 29 ++++++++++++++++++++++++++--- >>>> qapi/block-core.json | 5 ++++- >>>> 2 files changed, 30 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/block/crypto.c b/block/crypto.c >>>> index 8237424ae6..034a645652 100644 >>>> --- a/block/crypto.c >>>> +++ b/block/crypto.c >>>> @@ -74,6 +74,7 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block, >>>> struct BlockCryptoCreateData { >>>> BlockBackend *blk; >>>> uint64_t size; >>>> + PreallocMode prealloc; >>>> }; >>>> >>>> >>>> @@ -112,7 +113,7 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block, >>>> * available to the guest, so we must take account of that >>>> * which will be used by the crypto header >>>> */ >>>> - return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF, >>>> + return blk_truncate(data->blk, data->size + headerlen, data->prealloc, >>>> errp); >>>> } >>>> >>>> @@ -251,6 +252,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format, >>>> static int block_crypto_co_create_generic(BlockDriverState *bs, >>>> int64_t size, >>>> QCryptoBlockCreateOptions *opts, >>>> + PreallocMode prealloc, >>>> Error **errp) >>>> { >>>> int ret; >>>> @@ -266,9 +268,14 @@ static int block_crypto_co_create_generic(BlockDriverState *bs, >>>> goto cleanup; >>>> } >>>> >>>> + if (prealloc == PREALLOC_MODE_METADATA) { >>>> + prealloc = PREALLOC_MODE_OFF; >>>> + } >>>> + >>>> data = (struct BlockCryptoCreateData) { >>>> .blk = blk, >>>> .size = size, >>>> + .prealloc = prealloc, >>>> }; >>>> >>>> crypto = qcrypto_block_create(opts, NULL, >>>> @@ -500,6 +507,7 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) >>>> BlockdevCreateOptionsLUKS *luks_opts; >>>> BlockDriverState *bs = NULL; >>>> QCryptoBlockCreateOptions create_opts; >>>> + PreallocMode preallocation = PREALLOC_MODE_OFF; >>>> int ret; >>>> >>>> assert(create_options->driver == BLOCKDEV_DRIVER_LUKS); >>>> @@ -515,8 +523,11 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) >>>> .u.luks = *qapi_BlockdevCreateOptionsLUKS_base(luks_opts), >>>> }; >>>> >>>> + if (luks_opts->has_preallocation) >>>> + preallocation = luks_opts->preallocation; >>>> + >>>> ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts, >>>> - errp); >>>> + preallocation, errp); >>>> if (ret < 0) { >>>> goto fail; >>>> } >>>> @@ -534,12 +545,24 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, >>>> QCryptoBlockCreateOptions *create_opts = NULL; >>>> BlockDriverState *bs = NULL; >>>> QDict *cryptoopts; >>>> + PreallocMode prealloc; >>>> + char *buf = NULL; >>>> int64_t size; >>>> int ret; >>>> + Error *local_err = NULL; >>>> >>>> /* Parse options */ >>>> size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0); >>>> >>>> + buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); >>>> + prealloc = qapi_enum_parse(&PreallocMode_lookup, buf, >>>> + PREALLOC_MODE_OFF, &local_err); >>>> + g_free(buf); >>>> + if (local_err) { >>>> + error_propagate(errp, local_err); >>>> + return -EINVAL; >>>> + } >>>> + >>>> cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL, >>>> &block_crypto_create_opts_luks, >>>> true); >>>> @@ -565,7 +588,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, >>>> } >>>> >>>> /* Create format layer */ >>>> - ret = block_crypto_co_create_generic(bs, size, create_opts, errp); >>>> + ret = block_crypto_co_create_generic(bs, size, create_opts, prealloc, errp); >>>> if (ret < 0) { >>>> goto fail; >>>> } >>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>> index 0d43d4f37c..ebcfc9f903 100644 >>>> --- a/qapi/block-core.json >>>> +++ b/qapi/block-core.json >>>> @@ -4205,13 +4205,16 @@ >>>> # >>>> # @file Node to create the image format on >>>> # @size Size of the virtual disk in bytes >>>> +# @preallocation Preallocation mode for the new image (default: off; >>>> +# allowed values: off/falloc/full >>> >>> Should we add also "metadata" to allowed values? and "since: 4.2"? >>> I'd like to have (just to have similar documentation with others >>> preallocation parameters): >> >> It it support but preallocation=off is the same as preallocation=metadata in luks, >> as luks only metadata is the header which is created anyway. >> In some sense I should throw a error for preallocation=off, but I suspect that >> that will break userspace api. >> What do you think? > > I don't know very well the details, but I agree with you that some > user APIs could expect that preallocation=off is always available. > > Maybe we can write something in the comment (explaining that off and metadata > are the same) and make the preallocation=metadata the default choice. preallocation=off does not mean “Do not allocate any metadata”. For example, qcow2 still creates the qcow2 header and a minimal refcount structure. It just means not to allocate any unnecessary metadata. For LUKS, all metadata is necessary, so preallocation=off and preallocation=metadata are the same. I don‘t think that means we only need to support one or the other. I think just making preallocation=metadata an alias for preallocation=off here is fine – and preallocation=off should stay the default, because it’s the default for everything else. Max
diff --git a/block/crypto.c b/block/crypto.c index 8237424ae6..034a645652 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -74,6 +74,7 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block, struct BlockCryptoCreateData { BlockBackend *blk; uint64_t size; + PreallocMode prealloc; }; @@ -112,7 +113,7 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block, * available to the guest, so we must take account of that * which will be used by the crypto header */ - return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF, + return blk_truncate(data->blk, data->size + headerlen, data->prealloc, errp); } @@ -251,6 +252,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format, static int block_crypto_co_create_generic(BlockDriverState *bs, int64_t size, QCryptoBlockCreateOptions *opts, + PreallocMode prealloc, Error **errp) { int ret; @@ -266,9 +268,14 @@ static int block_crypto_co_create_generic(BlockDriverState *bs, goto cleanup; } + if (prealloc == PREALLOC_MODE_METADATA) { + prealloc = PREALLOC_MODE_OFF; + } + data = (struct BlockCryptoCreateData) { .blk = blk, .size = size, + .prealloc = prealloc, }; crypto = qcrypto_block_create(opts, NULL, @@ -500,6 +507,7 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) BlockdevCreateOptionsLUKS *luks_opts; BlockDriverState *bs = NULL; QCryptoBlockCreateOptions create_opts; + PreallocMode preallocation = PREALLOC_MODE_OFF; int ret; assert(create_options->driver == BLOCKDEV_DRIVER_LUKS); @@ -515,8 +523,11 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp) .u.luks = *qapi_BlockdevCreateOptionsLUKS_base(luks_opts), }; + if (luks_opts->has_preallocation) + preallocation = luks_opts->preallocation; + ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts, - errp); + preallocation, errp); if (ret < 0) { goto fail; } @@ -534,12 +545,24 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, QCryptoBlockCreateOptions *create_opts = NULL; BlockDriverState *bs = NULL; QDict *cryptoopts; + PreallocMode prealloc; + char *buf = NULL; int64_t size; int ret; + Error *local_err = NULL; /* Parse options */ size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0); + buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); + prealloc = qapi_enum_parse(&PreallocMode_lookup, buf, + PREALLOC_MODE_OFF, &local_err); + g_free(buf); + if (local_err) { + error_propagate(errp, local_err); + return -EINVAL; + } + cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL, &block_crypto_create_opts_luks, true); @@ -565,7 +588,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, } /* Create format layer */ - ret = block_crypto_co_create_generic(bs, size, create_opts, errp); + ret = block_crypto_co_create_generic(bs, size, create_opts, prealloc, errp); if (ret < 0) { goto fail; } diff --git a/qapi/block-core.json b/qapi/block-core.json index 0d43d4f37c..ebcfc9f903 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -4205,13 +4205,16 @@ # # @file Node to create the image format on # @size Size of the virtual disk in bytes +# @preallocation Preallocation mode for the new image (default: off; +# allowed values: off/falloc/full # # Since: 2.12 ## { 'struct': 'BlockdevCreateOptionsLUKS', 'base': 'QCryptoBlockCreateOptionsLUKS', 'data': { 'file': 'BlockdevRef', - 'size': 'size' } } + 'size': 'size', + '*preallocation': 'PreallocMode' } } ## # @BlockdevCreateOptionsNfs:
preallocation=off and preallocation=metadata both allocate luks header only, and preallocation=falloc/full is passed to underlying file. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534951 Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- Note that QMP support was only compile tested, since I am still learning on how to use it. If there is some library/script/etc which makes it more high level, I would more that glad to hear about it. So far I used the qmp-shell Also can I use qmp's blockdev-create outside a vm running? block/crypto.c | 29 ++++++++++++++++++++++++++--- qapi/block-core.json | 5 ++++- 2 files changed, 30 insertions(+), 4 deletions(-)