Re: [Qemu-devel] [PATCH v2] LUKS: support preallocation in qemu-img

[Maxim Levitsky <mlevitsk@redhat.com>]

On Thu, 2019-07-11 at 15:43 +0200, Max Reitz wrote:
> On 11.07.19 11:11, 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>
> > ---
> >  block/crypto.c | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> FWIW, do you see the implementation of block_crypto_co_truncate()?
> Like, how it just passes preallocation requests through to the
> underlying layer?  How I said it shouldn’t be done?
> 
> Yes, that was me, in commit 7ea37c30660.
> 
> So, er, yeah.
> 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 8237424ae6..cbc291301e 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> 
> [...]
> 
> > @@ -534,12 +537,28 @@ 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);
> 
> Please align such lines to the opening parenthesis.
True - I really need to invest some time to update the checkpatch.pl
in qemu source tree to be up to date, or find a way to use the kernel one,
it is so useful to let it catch these things instead of wasting your time.

> 
> > +    g_free(buf);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return -EINVAL;
> > +    }
> > +
> > +    if (prealloc == PREALLOC_MODE_METADATA) {
> > +        prealloc  = PREALLOC_MODE_OFF;
> 
> There is one space too many here.
Oops, same thing as above.

> 
> > +    }
> > +
> 
> I think you also need to add a @preallocation parameter to
> BlockdevCreateOptionsLUKS and handle it in block_crypto_co_create_luks().

I was under impression that with new qmp based blockdev-create api, the user
should pretty much do the preallocation itself on the underlying block file,
and then create the luks on it.

However I do see that qcow2 has a preallocation mode there, so I guess I was wrong.
Will do it now.

Best regards,
	Maxim Levitsky