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

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

[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

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

[Stefan Hajnoczi]

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

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 <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

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

[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

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

[Kevin Wolf]

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

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

[Laurent Vivier]

>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