Re: [Qemu-devel] [PATCH v4 4/6] introduce new vma archive format

[Dietmar Maurer]

> > +This format contains a header which includes the VM configuration as
> > +binary blobs, and a list of devices (dev_id, name).
> 
> Is there a magic number, for quickly identifying whether a file is likely to 
> be a
> vma?  

Yes  ('VMA')

> What endianness are multi-byte numbers interpreted with?  

BE

>Does the
> overall header file leave ample room for adding later extensions in a manner
> that is reliably detected as an unsupported feature in older tools?

yes

> > +The actual VM image data is stored inside extents. An extent contains
> > +up to 64 clusters, and start with a 512 byte header containing
> > +additional information for those clusters.
> 
> Doesn't this create alignment slowdowns on modern disks that prefer 4k
> alignment?  

No, because it is meant to be read sequentially. Direct access to specific 
blocks
is not needed.

> Wouldn't it be better to have the header be 4096 bytes, so that each
> of the 64 clusters in an extent is also 4096-aligned?  If you _do_ go with a 
> 4096
> header per extent, then it might be better to go with 16 bytes per cluster and
> 256 clusters per extent, instead of 8 bytes per cluster and 512 clusters per
> extent.

That would increase extend size to 16MB, and would increase memory footprint
because we need to have at least 2 extends in RAM. 

But again, there is no need to align to 4096 bytes.

> > +We use a cluster size of 65536, and use 8 bytes for each cluster in
> > +the header to store the following information:
> > +
> > +* 1 byte dev_id (to identity the drive)
> > +* 2 bytes zero indicator (mark zero regions (16x4096))
> > +* 4 bytes cluster number
> 
> Is that sufficient, or are we artificially limiting the maximum size of a 
> disk image
> that can be stored to 64k*4G = 128T? Again, going with 16-bytes per cluster
> instead of 8 bytes per cluster, to get up to a 4096-byte header alignment so 
> that
> all clusters fall on nice alignment boundaries, would leave you room to 
> supply a
> 64-bit offset instead of a 32-bit cluster number.  Don't know if that would be
> helpful or not, but food for thought.

Honestly, 64k*4G = 128T is not a limit for me. And we still have one byte 
reserved,
so we can have up to 1P per image, and up to 253 images.

For now I have no plans to backup such large VMs.
 
> > +* 1 byte not used (reserved)
> 
> Can these be rearranged to 'dev_id, reserved, zero indicator, cluster number' 
> to
> achieve natural alignment when reading the cluster number?

It is already ordered that way in the code. I will fix the order in the text.
 
> > +We only store non-zero blocks (such block is 4096 bytes).
> 
> So if I understand correctly, your current layout is divided into extents of  
> up to
> 4194816 bytes each (512 header, then 4M divided into
> 64 clusters of 64k each).  Then, if the zero indicator is all 0, then the
> corresponding cluster will be 64k bytes on the wire; if it is 0x0001, then 
> the first
> 4096 bytes of the corresponding cluster will be all zeros, and the cluster 
> itself
> will occupy only 60k of the vma file?

yes

> > +Each archive is marked with a uuid. The archive header and all extent
> > +headers includes that uuid and a MD5 checksum (over header data).
> 
> Layout of this header?

see vma.h
 
> Hint - look at how the qcow2 file format is specified.  You need a lot more
> details - enough that someone could independently implement a program to
> read and create vma images that would be compatible with what your
> implementation produces.

The format is really simple, you have the definitions in the header file,
and a reference implementation. I am quite sure any experienced developer 
can write an implementation in a few hours.

We do not talk about an extremely complex format like 'qcow2' here.

> > +++ b/vma-reader.c
> > @@ -0,0 +1,799 @@
> > +/*
> > + * VMA: Virtual Machine Archive
> > + *
> > + * Copyright (C) 2012 Proxmox Server Solutions
> 
> It's 2013.
> 
> > +
> > +#define BITS_PER_LONG  (sizeof(unsigned long) * 8)
> 
> 8 is a magic number; you should be using CHAR_BIT from limits.h instead.

Ok, will change that.