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 <mlevi...@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
signature.asc
Description: OpenPGP digital signature