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

[Maxim Levitsky]

On Wed, 2019-07-10 at 23:52 +0200, Max Reitz wrote:
> On 10.07.19 23:24, Max Reitz wrote:
> > On 10.07.19 19:03, Maxim Levitsky wrote:
> > > preallocation=off and preallocation=metadata
> > > both allocate luks header only, and preallocation=falloc/full
> > > is passed to underlying file, with the given image size.
> > > 
> > > Note that the actual preallocated size is a bit smaller due
> > > to luks header.
> > 
> > Couldn’t you just preallocate it after creating the crypto header so
> > qcrypto_block_get_payload_offset(crypto->block) + size is the actual
> > file size?

I kind of thought of the same thing after I send the patch. I'll see now it I 
can make it work.


> > 
> > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534951
> > > 
> > > Signed-off-by: Maxim Levitsky <address@hidden>
> > > ---
> > >  block/crypto.c | 28 ++++++++++++++++++++++++++--
> > >  1 file changed, 26 insertions(+), 2 deletions(-)
> > 
> > Hm.  I would expect a preallocated image to read 0.  But if you just
> > pass this through to the protocol layer, it won’t read 0.
> > 
> > (In fact, I don’t even quite see the point of having LUKS as an own
> > format still.  It was useful when qcow2 didn’t have LUKS support, but
> > now it does, so...  I suppose everyone using the LUKS format should
> > actually be using qcow2 with LUKS?)
> 
> Kevin just pointed out to me that our LUKS format is compatible to the
> actual layout cryptsetup uses.  OK, that is an important use case.
> 
> Hm.  Unfortunately, that doesn’t really necessitate preallocation.
> 
> Well, whatever.  If it’s simple enough, that shouldn’t stop us from
> implementing preallocation anyway.
Exactly. Since I already know the area of qemu-img relatively well, and
this bug is on my backlog, I thought why not to do it.


> 
> 
> Now I found that qapi/block-core.json defines PreallocMode’s falloc and
> full values as follows:
> 
> > # @falloc: like @full preallocation but allocate disk space by
> > #          posix_fallocate() rather than writing zeros.
> > # @full: preallocate all data by writing zeros to device to ensure disk
> > #        space is really available. @full preallocation also sets up
> > #        metadata correctly.
> 
> So it isn’t just me who expects these to pre-initialize the image to 0.
>  Hm, although...  I suppose @falloc technically does not specify whether
> the data reads as zeroes.  I kind of find it to be implied, but, well...

I personally don't really think that zeros are important, but rather the level 
of allocation.
posix_fallocate probably won't write the data blocks but rather only the inode 
metadata / used block bitmap/etc.

On the other hand writing zeros (or anything else) will force the block layer 
to actually write to the underlying
storage which could trigger lower layer allocation if the underlying storage is 
thin-provisioned.

In fact IMHO, instead of writing zeros, it would be better to write random 
garbage instead (or have that as an even 'fuller'
preallocation mode), since underlying storage might 'compress' the zeros. 

In this version I do have a bug that I mentioned, about not preallocation some 
data at the end of the image, and I will
fix it, so that all image is zeros as expected

Best regards,
        Maxim Levitsky


> 
> Max
>