Re: [Qemu-devel] [Qemu-block] [PATCH v3] LUKS: support preallocation

[Stefano Garzarella]

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.

Thanks,
Stefano