Re: [Qemu-devel] [RFC] qed: Add QEMU Enhanced Disk format

[Alexander Graf]

On 06.09.2010, at 12:04, Stefan Hajnoczi wrote:

> QEMU Enhanced Disk format is a disk image format that forgoes features
> found in qcow2 in favor of better levels of performance and data
> integrity.  Due to its simpler on-disk layout, it is possible to safely
> perform metadata updates more efficiently.
> 
> Installations, suspend-to-disk, and other allocation-heavy I/O workloads
> will see increased performance due to fewer I/Os and syncs.  Workloads
> that do not cause new clusters to be allocated will perform similar to
> raw images due to in-memory metadata caching.
> 
> The format supports sparse disk images.  It does not rely on the host
> filesystem holes feature, making it a good choice for sparse disk images
> that need to be transferred over channels where holes are not supported.
> 
> Backing files are supported so only deltas against a base image can be
> stored.
> 
> The file format is extensible so that additional features can be added
> later with graceful compatibility handling.
> 
> Internal snapshots are not supported.  This eliminates the need for
> additional metadata to track copy-on-write clusters.
> 
> Compression and encryption are not supported.  They add complexity and
> can be implemented at other layers in the stack (i.e. inside the guest
> or on the host).
> 
> The format is currently functional with the following features missing:
> * Resizing the disk image.  The capability has been designed in but the
>   code has not been written yet.
> * Resetting the image after backing file commit completes.
> * Changing the backing filename.
> * Consistency check (fsck).  This is simple due to the on-disk layout.

Yippie - yet another disk format :). Let's hope this one survives.

> 
> Signed-off-by: Anthony Liguori <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
> This code is also available from git (for development and testing the tracing
> and blkverify features are pulled in, whereas this single squashed patch
> applies to mainline qemu.git):
> 
> http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/qed
> 

just looked at it and stumbled over two simple nits.

[snip]

> +/**
> + * Get the number of bits for a power of 2
> + *
> + * The following is true for powers of 2:
> + *   n == 1 << get_bits_from_size(n)
> + */
> +int get_bits_from_size(size_t size)
> +{
> +    int res = 0;
> +
> +    if (size == 0) {
> +        return -1;
> +    }
> +
> +    while (size != 1) {
> +        /* Not a power of two */
> +        if (size & 1) {
> +            return -1;
> +        }
> +
> +        size >>= 1;
> +        res++;
> +    }
> +
> +    return res;
> +}

Should be an extra patch - it doesn't hurt to send an RFC patch set. This thing 
is so big that it's no fun to review :).

> +
> +const char *bytes_to_str(uint64_t size)
> +{
> +    static char buffer[64];
> +
> +    if (size < (1ULL << 10)) {
> +        snprintf(buffer, sizeof(buffer), "%" PRIu64 " byte(s)", size);
> +    } else if (size < (1ULL << 20)) {
> +        snprintf(buffer, sizeof(buffer), "%" PRIu64 " KB(s)", size >> 10);
> +    } else if (size < (1ULL << 30)) {
> +        snprintf(buffer, sizeof(buffer), "%" PRIu64 " MB(s)", size >> 20);
> +    } else if (size < (1ULL << 40)) {
> +        snprintf(buffer, sizeof(buffer), "%" PRIu64 " GB(s)", size >> 30);
> +    } else {
> +        snprintf(buffer, sizeof(buffer), "%" PRIu64 " TB(s)", size >> 40);
> +    }
> +
> +    return buffer;

This returns a variable from the stack! Please make the target buffer caller 
defined.

> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index dfd3dc0..754b107 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -137,6 +137,8 @@ time_t mktimegm(struct tm *tm);
> int qemu_fls(int i);
> int qemu_fdatasync(int fd);
> int fcntl_setfl(int fd, int flag);
> +int get_bits_from_size(size_t size);
> +const char *bytes_to_str(uint64_t size);
> 
> /* path.c */
> void init_paths(const char *prefix);
> @@ -283,6 +285,7 @@ void qemu_iovec_destroy(QEMUIOVector *qiov);
> void qemu_iovec_reset(QEMUIOVector *qiov);
> void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf);
> void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t 
> count);
> +void qemu_iovec_zero(QEMUIOVector *qiov);

separate patch please.


Alex