IMGEDIT loader.img fails on Ubuntu 12.04LTS

[Takuya ASADA]

Hi,

I've got error while building OSv on Ubuntu 12.04LTS.

Looks like qemu-nbd is failing, but I'm not really sure.

Is there any solution?

syuu@ubuntu:~/uosv/osv$ sudo make all

[sudo] password for syuu: 

  ANT tests/bench

make -r -C build/release/ all

make[1]: Entering directory `/home/syuu/uosv/osv/build/release'

  DD loader.img boot.bin

  DD loader.img loader.elf

  IMGEDIT loader.img

nbd.c:nbd_trip():L692: invalid request type (3) received

Traceback (most recent call last):

  File "../../scripts/imgedit.py", line 152, in <module>

    f.write(struct.pack('H', blocks))

  File "../../scripts/imgedit.py", line 124, in write

    self._client.flush()

  File "/home/syuu/uosv/osv/scripts/nbd_client.py", line 102, in flush

    (data, errno) = self._parse_reply()

  File "/home/syuu/uosv/osv/scripts/nbd_client.py", line 63, in _parse_reply

    (magic, errno, handle) = struct.unpack(">LLQ", reply)

struct.error: unpack requires a string argument of length 16

Exception struct.error: 'unpack requires a string argument of length 16' in <bound method nbd_file.__del__ of <__main__.nbd_file object at 0x2aaaaac85590>> ignored

Exception socket.error: (32, 'Broken pipe') in <bound method nbd_client.__del__ of <nbd_client.nbd_client object at 0x2aaaaac85990>> ignored

make[1]: *** [loader.img] Error 1

make[1]: *** Deleting file `loader.img'

make[1]: Leaving directory `/home/syuu/uosv/osv/build/release'

make: *** [all] Error 2

syuu@ubuntu:~/uosv/osv$ qemu-nbd --version

qemu-nbd version 0.0.1

Written by Anthony Liguori.

Copyright (C) 2006 Anthony Liguori <ant...@codemonkey.ws>.

This is free software; see the source for copying conditions.  There is NO

warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[Pekka Enberg]

I have the exact same version on Fedora 19 so AFAICT it should work.

Benoit, Avi?

Pekka

[Takuya ASADA]

Ubuntu 13.10 also has same version of qemu-nbd, but it able to build without error.

2013年11月15日金曜日 18時40分39秒 UTC+9 Pekka Enberg:

[Pekka Enberg]

On Fri, Nov 15, 2013 at 12:41 PM, Takuya ASADA <sy...@dokukino.com> wrote:
> Ubuntu 13.10 also has same version of qemu-nbd, but it able to build without
> error.

Maybe it's related to Python version?

[Benoît Canet]

Le Friday 15 Nov 2013 � 12:56:14 (+0200), Pekka Enberg a �crit :

I have 2.7.5

benoit@Laure:~/qemu (quorum)$ python -V
Python 2.7.5+

>
> --
> You received this message because you are subscribed to the Google Groups "OSv Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.

[Takuya ASADA]

Ubuntu 12.04LTS has Python 2.7.3, 13.10 has Python 2.7.5+.

2013/11/15 Pekka Enberg <pen...@cloudius-systems.com>

[Benoît Canet]

Le Friday 15 Nov 2013 � 11:40:39 (+0200), Pekka Enberg a �crit :

Maybe it's a short read I don't check for that.
I will write a patch.

Best regards

Beno�t

[Benoît Canet]

Hello Takuya,

Please could you replace nbd_client.py by the version attached to this mail and
give it a try ?

Best regards

Beno�t

Le Friday 15 Nov 2013 � 11:40:39 (+0200), Pekka Enberg a �crit :

[Takuya ASADA]

Hi,

It still doesn't work with attached nbd_client.py.

It still says "nbd.c:nbd_trip():L692: invalid request type (3) received", but now I don't get trackback from python.

imgedit.py is eating 100% cpu usage, it seems just freezing.

I could break the program with ctrl+c, here's a trackback:

Traceback (most recent call last):

  File "../../scripts/imgedit.py", line 152, in <module>

    f.write(struct.pack('H', blocks))

  File "../../scripts/imgedit.py", line 124, in write

    self._client.flush()

  File "/home/syuu/osv/scripts/nbd_client.py", line 108, in flush

    (data, errno) = self._parse_reply()

  File "/home/syuu/osv/scripts/nbd_client.py", line 68, in _parse_reply

    reply = self._receive(4 + 4 + 8)

  File "/home/syuu/osv/scripts/nbd_client.py", line 48, in _receive

    result += self._s.recv(length - len(result))

KeyboardInterrupt

^Cmake[1]: *** [loader.img] Interrupt

make: *** [all] Interrupt

2013年11月15日金曜日 21時06分07秒 UTC+9 Benoît Canet:

Hello Takuya,

Please could you replace nbd_client.py by the version attached to this mail and
give it a try ?

Best regards

Beno�t

Le Friday 15 Nov 2013 � 11:40:39 (+0200), Pekka Enberg a �crit :

[Benoît Canet]

Le Friday 15 Nov 2013 à 13:13:53 (-0800), Takuya ASADA a écrit :
> Hi,
>
> It still doesn't work with attached nbd_client.py.
> It still says "nbd.c:nbd_trip():L692: invalid request type (3) received",
> but now I don't get trackback from python.
> imgedit.py is eating 100% cpu usage, it seems just freezing.
> I could break the program with ctrl+c, here's a trackback:
> Traceback (most recent call last):
> File "../../scripts/imgedit.py", line 152, in <module>
> f.write(struct.pack('H', blocks))
> File "../../scripts/imgedit.py", line 124, in write
> self._client.flush()
> File "/home/syuu/osv/scripts/nbd_client.py", line 108, in flush
> (data, errno) = self._parse_reply()
> File "/home/syuu/osv/scripts/nbd_client.py", line 68, in _parse_reply
> reply = self._receive(4 + 4 + 8)
> File "/home/syuu/osv/scripts/nbd_client.py", line 48, in _receive
> result += self._s.recv(length - len(result))
> KeyboardInterrupt
> ^Cmake[1]: *** [loader.img] Interrupt
> make: *** [all] Interrupt

I suspect that your version of qemu-nbd is tearing down the connection before
sending the disconnect response.

Please could you git bisect qemu-nbd behavior between your qemu version and the
upstream qemu ?

Best regards

Benoît

>
> 2013年11月15日金曜日 21時06分07秒 UTC+9 Benoît Canet:
> >
> >
> > Hello Takuya,
> >
> > Please could you replace nbd_client.py by the version attached to this
> > mail and
> > give it a try ?
> >
> > Best regards
> >
> > Beno�t
> >
> > Le Friday 15 Nov 2013 � 11:40:39 (+0200), Pekka Enberg a �crit :

> > > On Fri, Nov 15, 2013 at 11:18 AM, Takuya ASADA <sy...@dokukino.com<javascript:>>

> > > > Copyright (C) 2006 Anthony Liguori <ant...@codemonkey.ws <javascript:>>.

> >
> > > > This is free software; see the source for copying conditions. There
> > is NO
> > > > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
> > PURPOSE.
> > >
> > > I have the exact same version on Fedora 19 so AFAICT it should work.
> > >
> > > Benoit, Avi?
> > >
> > > Pekka
> > >
> > > --
> > > You received this message because you are subscribed to the Google
> > Groups "OSv Development" group.
> > > To unsubscribe from this group and stop receiving emails from it, send

> > an email to osv-dev+u...@googlegroups.com <javascript:>.

[Takuya ASADA]

I realized Ubuntu 12.04's qemu-uitls package(whcih provides qemu-nbd) is based on qemu-kvm 1.0, Ubuntu 13.10's qemu-utils package is based on qemu-1.5.0.

qemu-nbd command on both packages have same version number, but the implementation is slightly different.

Error message was "nbd.c:nbd_trip():L692: invalid request type (3) received", type 3 is NBD_CMD_FLUSH, which is defined on nbd.h.

enum {

    NBD_CMD_READ = 0,

    NBD_CMD_WRITE = 1,

    NBD_CMD_DISC = 2,

    NBD_CMD_FLUSH = 3,

    NBD_CMD_TRIM = 4

};

On qemu-kvm 1.0's nbd.c, it even doesn't have NBD_CMD_FLUSH handling code on nbd_trip() function.

qemu-nbd command also linked other .c files, but the error is causing on nbd.c, so I attached nbd.c diff. 

2013年11月16日土曜日 8時28分27秒 UTC+9 Benoît Canet:

[Takuya ASADA]

I'm not sure I can just comment out flush command on nbd_client, but it worked fine on Ubuntu 12.04.

Quick hack diff attached.

2013年11月18日月曜日 4時33分50秒 UTC+9 Takuya ASADA:

[Pekka Enberg]

On Sun, Nov 17, 2013 at 9:49 PM, Takuya ASADA <sy...@dokukino.com> wrote:
> I'm not sure I can just comment out flush command on nbd_client, but it
> worked fine on Ubuntu 12.04.

Interesting. Benoit, Avi?

[Benoît Canet]

Le Monday 18 Nov 2013 � 12:24:59 (+0200), Pekka Enberg a �crit :

I think flush is still required because we want to make sure that the data has
reached stable storage.
Maybe this debian derivative should fix it's qemu package.

Best regards

Beno�t

[Pekka Enberg]

On Mon, Nov 18, 2013 at 1:26 PM, Benoît Canet <benoit...@irqsave.net> wrote:
> I think flush is still required because we want to make sure that the data has
> reached stable storage.
> Maybe this debian derivative should fix it's qemu package.

It's Ubuntu 12.04 so seems unlikely they'll fix it...

Can we catch the exception on flush() and just ignore it?

[Benoît Canet]

Le Monday 18 Nov 2013 � 13:29:58 (+0200), Pekka Enberg a �crit :

> On Mon, Nov 18, 2013 at 1:26 PM, Beno�t Canet <benoit...@irqsave.net> wrote:
> > I think flush is still required because we want to make sure that the data has
> > reached stable storage.
> > Maybe this debian derivative should fix it's qemu package.
>
> It's Ubuntu 12.04 so seems unlikely they'll fix it...

It's the LTS version of ubuntu some support is suposed to run for multiple years
on those.

>
> Can we catch the exception on flush() and just ignore it?

I think that the nbd server is taken by surprise when it receive the flush and
that it's state must be messed up.

Best regards

Beno�t

[Takuya ASADA]

Hi,

Looks like NBD_CMD_FLUSH cmd is added with "nbd asynchronous operation" patches.

http://lists.gnu.org/archive/html/qemu-devel/2011-12/msg00734.html

Maybe old qemu-nbd acts sync operation, doesn't it mean we don't have to flush on the version?

2013/11/18 Pekka Enberg <pen...@cloudius-systems.com>

[Takuya ASADA]

I found that nbd protocl has negotiation stage, and in the stage it send capability flag. And it has "NBD_FLAG_SEND_FLUSH".

Maybe it's self._flags on script/nbd_client.py, we can decide send FLUSH or not based on the flag, I guess.

2013/11/19 Takuya ASADA <sy...@dokukino.com>

[Benoît Canet]

Le Tuesday 19 Nov 2013 � 02:15:20 (+0900), Takuya ASADA a �crit :

> I found that nbd protocl has negotiation stage, and in the stage it send
> capability flag. And it has "NBD_FLAG_SEND_FLUSH".
> Maybe it's self._flags on script/nbd_client.py, we can decide send FLUSH or
> not based on the flag, I guess.

I will write a patch to do this.

Best regards

Beno�t

>
>
> 2013/11/19 Takuya ASADA <sy...@dokukino.com>
>
> > Hi,
> >
> > Looks like NBD_CMD_FLUSH cmd is added with "nbd asynchronous operation"
> > patches.
> > http://lists.gnu.org/archive/html/qemu-devel/2011-12/msg00734.html
> >
> > Maybe old qemu-nbd acts sync operation, doesn't it mean we don't have to
> > flush on the version?
> >
> >
> > 2013/11/18 Pekka Enberg <pen...@cloudius-systems.com>
> >

> >> On Mon, Nov 18, 2013 at 1:26 PM, Beno�t Canet <benoit...@irqsave.net>

[Takuya ASADA]

well I just wrote it, will send soon

2013/11/19 Benoît Canet <benoit...@irqsave.net>

Le Tuesday 19 Nov 2013 à 02:15:20 (+0900), Takuya ASADA a écrit :

> I found that nbd protocl has negotiation stage, and in the stage it send
> capability flag. And it has "NBD_FLAG_SEND_FLUSH".
> Maybe it's self._flags on script/nbd_client.py, we can decide send FLUSH or
> not based on the flag, I guess.

I will write a patch to do this.

Best regards

Benoît

>
>
> 2013/11/19 Takuya ASADA <sy...@dokukino.com>
>
> > Hi,
> >
> > Looks like NBD_CMD_FLUSH cmd is added with "nbd asynchronous operation"
> > patches.
> > http://lists.gnu.org/archive/html/qemu-devel/2011-12/msg00734.html
> >
> > Maybe old qemu-nbd acts sync operation, doesn't it mean we don't have to
> > flush on the version?
> >
> >
> > 2013/11/18 Pekka Enberg <pen...@cloudius-systems.com>
> >

> >> On Mon, Nov 18, 2013 at 1:26 PM, Benoît Canet <benoit...@irqsave.net>

[Takuya ASADA]

With this patch nbd_client checks qmeu-nbd has FLUSH command.
If it doesn't have, just ignore flush().
---
scripts/nbd_client.py | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/scripts/nbd_client.py b/scripts/nbd_client.py
index 2c7d09e..9dcdd16 100644
--- a/scripts/nbd_client.py
+++ b/scripts/nbd_client.py
@@ -22,6 +22,9 @@ class nbd_client(object):
DISCONNECT = 2
FLUSH = 3

+ FLAG_HAS_FLAGS = (1 << 0)
+ FLAG_SEND_FLUSH = (1 << 2)
+
def __init__(self, hostname, port = 10809):
self._flushed = True
self._closed = True
@@ -95,8 +98,18 @@ class nbd_client(object):
assert(errno == 0)
return data

+ def need_flush(self):
+ if self._flags & self.FLAG_HAS_FLAGS != 0 and \
+ self._flags & self.FLAG_SEND_FLUSH != 0:
+ return True
+ else:
+ return False
+
def flush(self):
self._is_read = False
+ if self.need_flush() == False:
+ self._flushed = True
+ return True
header = self._build_header(self.FLUSH, 0, 0)
self._s.send(header)
(data, errno) = self._parse_reply()
--
1.8.3.2

[Benoît Canet]

Le Monday 18 Nov 2013 � 22:37:36 (-0800), Takuya ASADA a �crit :

Looks good

Beno�t


> --
> 1.8.3.2

>
> --
> You received this message because you are subscribed to the Google Groups "OSv Development" group.

> To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.

[Pekka Enberg]

On Tue, Nov 19, 2013 at 8:37 AM, Takuya ASADA <sy...@dokukino.com> wrote:
> With this patch nbd_client checks qmeu-nbd has FLUSH command.
> If it doesn't have, just ignore flush().

Sign-off and explanation of the bug this fixes in the changelog, please.

[Takuya ASADA]

Some older version of qemu-nbd cuases error exit with nbd_client.py.
(Look at: https://groups.google.com/d/msg/osv-dev/EW5BtNFNfzs/I33BeFXg2f0J)
This is because nbd_client.py is sending FLUSH command unconditionally, but it's extended feature, nbd client should check nbd server has the capability to accept FLUSH.
nbd server sends capability flags on negotiation stage, it sends HAS_FLAGS(0x1) and SEND_FLUSH(0x4) when server supports FLUSH.

This patch adds these capability check, and skips to send FLUSH if server doesn't support it.

Signed-off-by: Takuya ASADA <sy...@dokukino.com>

--
1.8.3.2

[Avi Kivity]

On 11/18/2013 10:24 PM, Takuya ASADA wrote:
> Some older version of qemu-nbd cuases error exit with nbd_client.py.
> (Look at: https://groups.google.com/d/msg/osv-dev/EW5BtNFNfzs/I33BeFXg2f0J)
> This is because nbd_client.py is sending FLUSH command unconditionally, but it's extended feature, nbd client should check nbd server has the capability to accept FLUSH.
> nbd server sends capability flags on negotiation stage, it sends HAS_FLAGS(0x1) and SEND_FLUSH(0x4) when server supports FLUSH.
>
> This patch adds these capability check, and skips to send FLUSH if server doesn't support it.
>
>

Thanks, applied.

[Commit Bot]

From: Takuya ASADA <sy...@dokukino.com>

Add negotiation flag check for FLUSH

Signed-off-by: Takuya ASADA <sy...@dokukino.com>
Reviewed-by: Benoît Canet <benoit...@irqsave.net>
Signed-off-by: Avi Kivity <a...@cloudius-systems.com>

---
diff --git a/scripts/nbd_client.py b/scripts/nbd_client.py

--- a/scripts/nbd_client.py
+++ b/scripts/nbd_client.py
@@ -22,6 +22,9 @@ class nbd_client(object):
DISCONNECT = 2
FLUSH = 3

+ FLAG_HAS_FLAGS = (1 << 0)
+ FLAG_SEND_FLUSH = (1 << 2)
+
def __init__(self, hostname, port = 10809):
self._flushed = True
self._closed = True

@@ -95,8 +98,18 @@ def read(self, offset, length):