[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3] LUKS: support preallocation
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v3] LUKS: support preallocation |
Date: |
Mon, 15 Jul 2019 10:38:04 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 |
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 <address@hidden>
>>>>
>>>> ---
>>>>
>>>> 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