nbd: fix NBD_CMD_CACHE negitiation according to the NBD specification
PATCH 1/1 nbd: fix NBD_CMD_CACHE negitiation according to the NBD specification
Commit bc37b06a5 was made very bad thing, it has been added NBD_FLAG_SEND_CACHE flag for negotiation. The problem is that the value of the flag was taken wrong and directly violates NBD specification. This value (bit 8) is used at least in the Linux kernel as a part of stable userspace-kernel API since 4.10 as NBD_FLAG_CAN_MULTI_CONN as defined in the specification:
"bit 8, NBD_FLAG_CAN_MULTI_CONN: Indicates that the server operates entirely without cache, or that the cache it uses is shared among all connections to the given device. In particular, if this flag is present, then the effects of NBD_CMD_FLUSH and NBD_CMD_FLAG_FUA MUST be visible across all connections when the server sends its reply to that command to the client. In the absense of this flag, clients SHOULD NOT multiplex their commands over more than one connection to the export.
...
bit 10, NBD_FLAG_SEND_CACHE: documents that the server understands NBD_CMD_CACHE; however, note that server implementations exist which support the command without advertising this bit, and conversely that this bit does not guarantee that the command will succeed or have an impact."
Personally I do not see any option that we will be allowed to fix the specification in favor of QEMU and apply the fix to the kernel. This is a big-big problem. Let us fix this in QEMU as soon as possible.
Thus the commit fixes negotiation flag in QEMU accoring to the specification. Fortunately enough the bit has been added by Virtuozzo and there are no released products in the field with this bit used so far.
--- include/block/nbd.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/block/nbd.h +1 -1
@@ -135,7 +135,7 @@ typedef struct NBDExtent {#define NBD_FLAG_SEND_TRIM (1 << 5)/* Send TRIM (discard) */ #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6)/* Send WRITE_ZEROES */ #define NBD_FLAG_SEND_DF (1 << 7)/* Send DF (Do not Fragment) */ -#define NBD_FLAG_SEND_CACHE (1 << 8)/* Send CACHE (prefetch) */ +#define NBD_FLAG_SEND_CACHE (1 << 10)/* Send CACHE (prefetch) */ /* New-style handshake (global) flags, sent from server to client, and control what will happen during handshake phase. */
Signature · 1 line
2.17.1
Quoted 2 lines · Denis V. Lunev
Commit
bc37b06a5was made very bad thing, it has been addedNBD_FLAG_SEND_CACHEflag for negotiation.
Oof. Probably my fault for not doing a careful review against the upstream spec.
Quoted 5 lines
The problem is that the value
of the flag was taken wrong and directly violates NBD specification. This value (bit 8) is used at least in the Linux kernel as a part of stable userspace-kernel API since 4.10 asNBD_FLAG_CAN_MULTI_CONNas defined in the specification:
And a kernel that honors that bit can cause qemu to misbehave. Yeah, that's definitely undesirable.
Quoted 7 lines
"bit 8,
NBD_FLAG_CAN_MULTI_CONN: Indicates that the server operates entirely without cache, or that the cache it uses is shared among all connections to the given device. In particular, if this flag is present, then the effects ofNBD_CMD_FLUSHandNBD_CMD_FLAG_FUAMUST be visible across all connections when the server sends its reply to that command to the client. In the absense of this flag, clients
Oh fun - a typo in the NBD spec (should be absence). I'll fix that separately.
Quoted 9 lines
SHOULD NOT multiplex their commands over more than one connection to the export.
...
bit 10,NBD_FLAG_SEND_CACHE: documents that the server understandsNBD_CMD_CACHE; however, note that server implementations exist which support the command without advertising this bit, and conversely that this bit does not guarantee that the command will succeed or have an impact."
Quoted 3 lines
Personally I do not see any option that we will be allowed to fix the specification in favor of QEMU and apply the fix to the kernel. This is a big-big problem. Let us fix this in QEMU as soon as possible.
Correct. Since the kernel is already one client that supports CAN_MULTI_CONN, qemu 3.0 was wrong for declaring the wrong bit value.
Quoted 2 lines
Thus the commit fixes negotiation flag in QEMU accoring to the
s/according/according/
Quoted 18 lines
specification. Fortunately enough the bit has been added by Virtuozzo and there are no released products in the field with this bit used so far.
Signed-off-by Denis V. Lunev <[email protected]>CC Vladimir Sementsov-Ogievskiy <[email protected]>CC Valery Vdovin <[email protected]>CC Eric Blake <[email protected]>CC Paolo Bonzini <[email protected]>--- include/block/nbd.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)include/block/nbd.h +0
-0
include/block/nbd.h typedef struct NBDExtent { · line 138
Quoted 5 lines
include/block/nbd.h +1
-1@@ -135,7 +135,7 @@ typedef struct NBDExtent {#define NBD_FLAG_SEND_TRIM (1 << 5)/* Send TRIM (discard) */ #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6)/* Send WRITE_ZEROES */ #define NBD_FLAG_SEND_DF (1 << 7)/* Send DF (Do not Fragment) */ -#define NBD_FLAG_SEND_CACHE (1 << 8)/* Send CACHE (prefetch) */ +#define NBD_FLAG_SEND_CACHE (1 << 10)/* Send CACHE (prefetch) */
I'll probably amend this to list all NBD_FLAG_ values in the spec (even if qemu doesn't implement them yet), just to make it easier to avoid making this mistake in the future.
Will queue through my NBD tree.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Vladimir Sementsov-Ogievskiy
Reviewed-byQuoted 6 lines · Eric Blake
Quoted 2 lines · Denis V. Lunev
Commit
bc37b06a5was made very bad thing, it has been addedNBD_FLAG_SEND_CACHEflag for negotiation.Oof. Probably my fault for not doing a careful review against the upstream spec.
mostly my, to introduce this =(. really too bad
Quoted 9 lines
Quoted 6 lines
The problem is that the value
of the flag was taken wrong and directly violates NBD specification. This value (bit 8) is used at least in the Linux kernel as a part of stable userspace-kernel API since 4.10 asNBD_FLAG_CAN_MULTI_CONNas defined in the specification:And a kernel that honors that bit can cause qemu to misbehave. Yeah, that's definitely undesirable.
As I understand opposite: kernel client will assume support for multi_conn feature which declares some guarantees about FLUSH, but qemu don't give these guarantees.
Quoted 63 lines
Quoted 8 lines
"bit 8,
NBD_FLAG_CAN_MULTI_CONN: Indicates that the server operates entirely without cache, or that the cache it uses is shared among all connections to the given device. In particular, if this flag is present, then the effects ofNBD_CMD_FLUSHandNBD_CMD_FLAG_FUAMUST be visible across all connections when the server sends its reply to that command to the client. In the absense of this flag, clientsOh fun - a typo in the NBD spec (should be absence). I'll fix that separately.
Quoted 9 lines
SHOULD NOT multiplex their commands over more than one connection to the export.
...
bit 10,NBD_FLAG_SEND_CACHE: documents that the server understandsNBD_CMD_CACHE; however, note that server implementations exist which support the command without advertising this bit, and conversely that this bit does not guarantee that the command will succeed or have an impact."Quoted 3 lines
Personally I do not see any option that we will be allowed to fix the specification in favor of QEMU and apply the fix to the kernel. This is a big-big problem. Let us fix this in QEMU as soon as possible.
Correct. Since the kernel is already one client that supports
CAN_MULTI_CONN, qemu 3.0 was wrong for declaring the wrong bit value.Quoted 2 lines
Thus the commit fixes negotiation flag in QEMU accoring to the
s/according/according/
Quoted 24 lines
specification. Fortunately enough the bit has been added by Virtuozzo and there are no released products in the field with this bit used so far.
Signed-off-by Denis V. Lunev <[email protected]>CC Vladimir Sementsov-Ogievskiy <[email protected]>CC Valery Vdovin <[email protected]>CC Eric Blake <[email protected]>CC Paolo Bonzini <[email protected]>---
include/block/nbd.h| 2 +-1 file changed, 1 insertion(+), 1 deletion(-)include/block/nbd.h +0
-0@@ -135,7 +135,7 @@ typedef struct NBDExtent { #define NBD_FLAG_SEND_TRIM (1 << 5) /* Send TRIM (discard) */ #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6) /* Send WRITE_ZEROES */ #define NBD_FLAG_SEND_DF (1 << 7) /* Send DF (Do notFragment) */
include/block/nbd.h +1
-1-#define NBD_FLAG_SEND_CACHE (1 << 8)/* Send CACHE (prefetch) */ +#define NBD_FLAG_SEND_CACHE (1 << 10)/* Send CACHE Quoted 1 line
(prefetch) */
I'll probably amend this to list all
NBD_FLAG_values in the spec (even if qemu doesn't implement them yet), just to make it easier to avoid making this mistake in the future.
Quoted 4 lines
Reviewed-by Eric Blake <[email protected]>Will queue through my NBD tree.
I vote for adding all values at least as a comment
Signature · 2 lines
Best regards, Vladimir
PATCH 2/2 nbd: add all possible transmission flags supported by NBD
This is done to avoid silly mistakes like one with wrong value of NBD_FLAG_SEND_CACHE flag.
--- include/block/nbd.h | 3 +++ 1 file changed, 3 insertions(+)
include/block/nbd.h +3 -0
@@ -135,6 +135,9 @@ typedef struct NBDExtent {#define NBD_FLAG_SEND_TRIM (1 << 5)/* Send TRIM (discard) */ #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6)/* Send WRITE_ZEROES */ #define NBD_FLAG_SEND_DF (1 << 7)/* Send DF (Do not Fragment) */ +#define NBD_FLAG_CAN_MULTI_CONN (1 << 8)/* Shared cache over multiple +connections per export */ +#define NBD_FLAG_SEND_RESIZE (1 << 9)/* Support RESIZE extension */ #define NBD_FLAG_SEND_CACHE (1 << 10)/* Send CACHE (prefetch) */ /* New-style handshake (global) flags, sent from server to client, and
Signature · 1 line
2.17.1
Quoted 17 lines · Vladimir Sementsov-Ogievskiy
Quoted 6 lines · Eric Blake
Quoted 2 lines · Denis V. Lunev
Commit
bc37b06a5was made very bad thing, it has been addedNBD_FLAG_SEND_CACHEflag for negotiation.Oof. Probably my fault for not doing a careful review against the upstream spec.
mostly my, to introduce this =(. really too bad
Quoted 7 lines
Quoted 6 lines
The problem is that the value
of the flag was taken wrong and directly violates NBD specification. This value (bit 8) is used at least in the Linux kernel as a part of stable userspace-kernel API since 4.10 asNBD_FLAG_CAN_MULTI_CONNas defined in the specification:
Quoted 6 lines
Quoted 2 lines
And a kernel that honors that bit can cause qemu to misbehave. Yeah, that's definitely undesirable.
As I understand opposite: kernel client will assume support for
multi_connfeature which declares some guarantees about FLUSH, but qemu don't give these guarantees.
Yeah, that's my biggest worry - data corruption when the kernel assumes it can make multiple connections with consistent caching, but the caching is not consistent, leading to data corruption when connected to an unpatched qemu 3.0 server. I'm rewording the commit message along those lines.
Quoted 14 lines
Quoted 8 lines
I'll probably amend this to list all
NBD_FLAG_values in the spec (even if qemu doesn't implement them yet), just to make it easier to avoid making this mistake in the future.Reviewed-by Eric Blake <[email protected]>Will queue through my NBD tree.
I vote for adding all values at least as a comment
Reviewed-by Vladimir Sementsov-Ogievskiy <[email protected]>
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
s/negitiation/negotiation/
Quoted 53 lines · Denis V. Lunev
Commit
bc37b06a5was made very bad thing, it has been addedNBD_FLAG_SEND_CACHEflag for negotiation. The problem is that the value of the flag was taken wrong and directly violates NBD specification. This value (bit 8) is used at least in the Linux kernel as a part of stable userspace-kernel API since 4.10 asNBD_FLAG_CAN_MULTI_CONNas defined in the specification:"bit 8,
NBD_FLAG_CAN_MULTI_CONN: Indicates that the server operates entirely without cache, or that the cache it uses is shared among all connections to the given device. In particular, if this flag is present, then the effects ofNBD_CMD_FLUSHandNBD_CMD_FLAG_FUAMUST be visible across all connections when the server sends its reply to that command to the client. In the absense of this flag, clients SHOULD NOT multiplex their commands over more than one connection to the export.
...
bit 10,NBD_FLAG_SEND_CACHE: documents that the server understandsNBD_CMD_CACHE; however, note that server implementations exist which support the command without advertising this bit, and conversely that this bit does not guarantee that the command will succeed or have an impact."Personally I do not see any option that we will be allowed to fix the specification in favor of QEMU and apply the fix to the kernel. This is a big-big problem. Let us fix this in QEMU as soon as possible.
Thus the commit fixes negotiation flag in QEMU accoring to the specification. Fortunately enough the bit has been added by Virtuozzo and there are no released products in the field with this bit used so far.
Signed-off-by Denis V. Lunev <[email protected]>CC Vladimir Sementsov-Ogievskiy <[email protected]>CC Valery Vdovin <[email protected]>CC Eric Blake <[email protected]>CC Paolo Bonzini <[email protected]>--- include/block/nbd.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)include/block/nbd.h +1
-1@@ -135,7 +135,7 @@ typedef struct NBDExtent {#define NBD_FLAG_SEND_TRIM (1 << 5)/* Send TRIM (discard) */ #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6)/* Send WRITE_ZEROES */ #define NBD_FLAG_SEND_DF (1 << 7)/* Send DF (Do not Fragment) */ -#define NBD_FLAG_SEND_CACHE (1 << 8)/* Send CACHE (prefetch) */ +#define NBD_FLAG_SEND_CACHE (1 << 10)/* Send CACHE (prefetch) */ /* New-style handshake (global) flags, sent from server to client, and control what will happen during handshake phase. */