Message ID | 1284663278-7487-1-git-send-email-laurent@vivier.eu |
---|---|
State | New |
Headers | show |
On Thu, Sep 16, 2010 at 7:54 PM, Laurent Vivier <laurent@vivier.eu> wrote: > 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 <laurent@vivier.eu> Hey nice commit message :). It would be useful to mention that this patch combines the reply header and payload send operation. Stefan
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 <laurent@vivier.eu> 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
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); TRACE("Sending data to client"); - if (write_sync(csock, data, request.len) != request.len) { + if (write_sync(csock, data, + request.len + sizeof(struct nbd_reply)) != + request.len + sizeof(struct nbd_reply)) { LOG("writing to socket failed"); errno = EINVAL; return -1;
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 <laurent@vivier.eu> --- nbd.c | 20 +++++++++++++++----- 1 files changed, 15 insertions(+), 5 deletions(-)