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

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

[Maxim Levitsky]

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
 #
 # Since: 2.12
 ##
 { 'struct': 'BlockdevCreateOptionsLUKS',
   'base': 'QCryptoBlockCreateOptionsLUKS',
   'data': { 'file':             'BlockdevRef',
-            'size':             'size' } }
+            'size':             'size',
+            '*preallocation':   'PreallocMode' } }
 
 ##
 # @BlockdevCreateOptionsNfs:
-- 
2.17.2

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

[Stefano Garzarella]

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

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

[Maxim Levitsky]

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

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 <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

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

[Maxim Levitsky]

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

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 7104 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[no-reply]

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