Re: [Qemu-block] [PATCH v10 3/9] qcow2: Make sizes more humanly readable

[Leonid Bloch]

On 9/24/18 7:59 PM, Eric Blake wrote:
> On 9/21/18 12:23 PM, Leonid Bloch wrote:
> > Signed-off-by: Leonid Bloch <address@hidden>
> > Reviewed-by: Alberto Garcia <address@hidden>
> > ---
> >   block/qcow2.c | 2 +-
> >   block/qcow2.h | 9 +++++----
> >   2 files changed, 6 insertions(+), 5 deletions(-)
>
> > +++ b/block/qcow2.h
> > @@ -27,6 +27,7 @@
> >   #include "crypto/block.h"
> >   #include "qemu/coroutine.h"
> > +#include "qemu/units.h"
> >   //#define DEBUG_ALLOC
> >   //#define DEBUG_ALLOC2
> > @@ -43,11 +44,11 @@
> >   /* 8 MB refcount table is enough for 2 PB images at 64k cluster size
> >    * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */
> > -#define QCOW_MAX_REFTABLE_SIZE 0x800000
> > +#define QCOW_MAX_REFTABLE_SIZE S_8MiB
>
> Why not (8 * MiB)?  Where does this get stringified that you have to 
> introduce a new macro? Can we instead fix the stringification location 
> to call a convenience helper that converts arbitrary values into 
> convenient strings at runtime, rather than having to worry about a 
> separate macro just for stringification purposes?

- Why not (8 * MiB)?
Because we have the lookup table from the previous patch.
- Why do we need this table?
Because some values (not this one specifically, but for example 
DEFAULT_CLUSTER_SIZE) get stringified at compile time (not during 
runtime), and for these values the only two options are to use such a 
table for convenient writing, or to write the number of bytes explicitly 
in the definition.
Again, while the necessity for such a table comes from stringification, 
it has an added value of short and clear size definitions.

> >   /* 32 MB L1 table is enough for 2 PB images at 64k cluster size
> >    * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */
> > -#define QCOW_MAX_L1_SIZE 0x2000000
> > +#define QCOW_MAX_L1_SIZE S_32MiB
>
> Again, why not (32 * MiB)?

Same as above.

> >   /* Allow for an average of 1k per snapshot table entry, should be 
> > plenty of
> >    * space for snapshot names and IDs */
> > @@ -75,9 +76,9 @@
> >   /* Whichever is more */
> >   #define DEFAULT_L2_CACHE_CLUSTERS 8 /* clusters */
> > -#define DEFAULT_L2_CACHE_BYTE_SIZE 1048576 /* bytes */
> > +#define DEFAULT_L2_CACHE_SIZE S_1MiB
>
> and (1 * MiB)

Same as above.

> > -#define DEFAULT_CLUSTER_SIZE 65536
> > +#define DEFAULT_CLUSTER_SIZE S_64KiB
>
> and (64 * KiB)

Because:

block/qcow2.c
4726:            .def_value_str = stringify(DEFAULT_CLUSTER_SIZE)

(and similar in some other files).

Leonid.