Re: [Qemu-devel] [PATCH] Improve qemu-nbd performance by 4400 %

[Kevin Wolf]

Am 16.09.2010 20:54, schrieb Laurent Vivier:
> This patch allows to reduce the boot time from an NBD server from 225 seconds 
> to
> 5 seconds (time between the "boot cd:0" and the kernel init) for the
> following command lines:
> 
> ./qemu-nbd -t ../ISO/debian-500-powerpc-netinst.iso
> and
> ./ppc-softmmu/qemu-system-ppc -cdrom nbd:localhost:1024
> 
> Signed-off-by: Laurent Vivier <address@hidden>

I agree with Stefan. It's good to have a description of the results in
the commit message, but describing what has actually changed from a
technical perspective would be helpful, too.

> ---
>  nbd.c |   20 +++++++++++++++-----
>  1 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/nbd.c b/nbd.c
> index 011b50f..5d7c758 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -655,7 +655,7 @@ int nbd_trip(BlockDriverState *bs, int csock, off_t size, 
> uint64_t dev_offset,
>       if (nbd_receive_request(csock, &request) == -1)
>               return -1;
>  
> -     if (request.len > data_size) {
> +     if (request.len + sizeof(struct nbd_reply) > data_size) {
>               LOG("len (%u) is larger than max len (%u)",
>                   request.len, data_size);
>               errno = EINVAL;
> @@ -687,7 +687,8 @@ int nbd_trip(BlockDriverState *bs, int csock, off_t size, 
> uint64_t dev_offset,
>       case NBD_CMD_READ:
>               TRACE("Request type is READ");
>  
> -             if (bdrv_read(bs, (request.from + dev_offset) / 512, data,
> +             if (bdrv_read(bs, (request.from + dev_offset) / 512,
> +                           data + sizeof(struct nbd_reply),
>                             request.len / 512) == -1) {
>                       LOG("reading from file failed");
>                       errno = EINVAL;
> @@ -697,12 +698,21 @@ int nbd_trip(BlockDriverState *bs, int csock, off_t 
> size, uint64_t dev_offset,
>  
>               TRACE("Read %u byte(s)", request.len);
>  
> -             if (nbd_send_reply(csock, &reply) == -1)
> -                     return -1;
> +             /* Reply
> +                [ 0 ..  3]    magic   (NBD_REPLY_MAGIC)
> +                [ 4 ..  7]    error   (0 == no error)
> +                [ 7 .. 15]    handle
> +              */
> +
> +             cpu_to_be32w((uint32_t*)data, NBD_REPLY_MAGIC);
> +             cpu_to_be32w((uint32_t*)(data + 4), reply.error);
> +             cpu_to_be64w((uint64_t*)(data + 8), reply.handle);

Hm, if I understand this right, you rely on the compiler padding out
structs here. You reserved sizeof(struct nbd_reply) bytes and the struct
is defined like this:

struct nbd_reply {
    uint32_t error;
    uint64_t handle;
};

So isn't it pure luck that the compiler does the right thing and gives
you 16 bytes? If you want to use the struct for this, you should add a
uint32_t magic to it and make it packed.

Kevin