* [Qemu-devel] [PATCH] Improve qemu-nbd performance by 4400 %
@ 2010-09-16 18:54 Laurent Vivier
2010-09-17 8:33 ` Stefan Hajnoczi
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Laurent Vivier @ 2010-09-16 18:54 UTC (permalink / raw)
To: qemu-devel; +Cc: 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>
---
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);
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;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] Improve qemu-nbd performance by 4400 %
2010-09-16 18:54 [Qemu-devel] [PATCH] Improve qemu-nbd performance by 4400 % Laurent Vivier
@ 2010-09-17 8:33 ` Stefan Hajnoczi
2010-09-17 10:24 ` Kevin Wolf
2010-09-17 16:13 ` [Qemu-devel] [PATCH][v2] " Laurent Vivier
2 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2010-09-17 8:33 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] Improve qemu-nbd performance by 4400 %
2010-09-16 18:54 [Qemu-devel] [PATCH] Improve qemu-nbd performance by 4400 % Laurent Vivier
2010-09-17 8:33 ` Stefan Hajnoczi
@ 2010-09-17 10:24 ` Kevin Wolf
2010-09-17 16:13 ` [Qemu-devel] [PATCH][v2] " Laurent Vivier
2 siblings, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2010-09-17 10:24 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH][v2] Improve qemu-nbd performance by 4400 %
2010-09-16 18:54 [Qemu-devel] [PATCH] Improve qemu-nbd performance by 4400 % Laurent Vivier
2010-09-17 8:33 ` Stefan Hajnoczi
2010-09-17 10:24 ` Kevin Wolf
@ 2010-09-17 16:13 ` Laurent Vivier
2010-09-17 16:39 ` Kevin Wolf
2 siblings, 1 reply; 6+ messages in thread
From: Laurent Vivier @ 2010-09-17 16:13 UTC (permalink / raw)
To: qemu-devel; +Cc: 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
This patch combines the reply header and payload send operation.
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
v2: use NBD_REPLY_SIZE instead of sizeof(struct nbd_reply)
nbd.c | 25 ++++++++++++++++++-------
1 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/nbd.c b/nbd.c
index 011b50f..4bf2eb7 100644
--- a/nbd.c
+++ b/nbd.c
@@ -49,6 +49,7 @@
/* This is all part of the "official" NBD API */
+#define NBD_REPLY_SIZE (4 + 4 + 8)
#define NBD_REQUEST_MAGIC 0x25609513
#define NBD_REPLY_MAGIC 0x67446698
@@ -588,7 +589,7 @@ static int nbd_receive_request(int csock, struct nbd_request *request)
int nbd_receive_reply(int csock, struct nbd_reply *reply)
{
- uint8_t buf[4 + 4 + 8];
+ uint8_t buf[NBD_REPLY_SIZE];
uint32_t magic;
memset(buf, 0xAA, sizeof(buf));
@@ -655,9 +656,9 @@ 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 + NBD_REPLY_SIZE > data_size) {
LOG("len (%u) is larger than max len (%u)",
- request.len, data_size);
+ request.len + NBD_REPLY_SIZE, data_size);
errno = EINVAL;
return -1;
}
@@ -687,7 +688,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 + NBD_REPLY_SIZE,
request.len / 512) == -1) {
LOG("reading from file failed");
errno = EINVAL;
@@ -697,12 +699,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 + NBD_REPLY_SIZE) !=
+ request.len + NBD_REPLY_SIZE) {
LOG("writing to socket failed");
errno = EINVAL;
return -1;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH][v2] Improve qemu-nbd performance by 4400 %
2010-09-17 16:13 ` [Qemu-devel] [PATCH][v2] " Laurent Vivier
@ 2010-09-17 16:39 ` Kevin Wolf
0 siblings, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2010-09-17 16:39 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel
Am 17.09.2010 18:13, 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
>
> This patch combines the reply header and payload send operation.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
> v2: use NBD_REPLY_SIZE instead of sizeof(struct nbd_reply)
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] Improve qemu-nbd performance by 4400 %
@ 2010-09-17 12:58 Laurent Vivier
0 siblings, 0 replies; 6+ messages in thread
From: Laurent Vivier @ 2010-09-17 12:58 UTC (permalink / raw)
To: kwolf; +Cc: qemu-devel
>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.
OK.
>> ---
>> 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.
>
Yes, it's pure luck, I will add a NBD_REPLY_SIZE defined to 16 and will replace the sizeof() by it.
Regards,
Laurent
--
--------------------- Laurent@vivier.eu ---------------------
"Tout ce qui est impossible reste à accomplir" Jules Verne
"Things are only impossible until they're not" Jean-Luc Picard
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-09-17 16:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-16 18:54 [Qemu-devel] [PATCH] Improve qemu-nbd performance by 4400 % Laurent Vivier
2010-09-17 8:33 ` Stefan Hajnoczi
2010-09-17 10:24 ` Kevin Wolf
2010-09-17 16:13 ` [Qemu-devel] [PATCH][v2] " Laurent Vivier
2010-09-17 16:39 ` Kevin Wolf
2010-09-17 12:58 [Qemu-devel] [PATCH] " Laurent Vivier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.