[Qemu-devel] [PATCH 1/1] nbd: fix NBD_CMD_CACHE negitiation according to the NBD specification

[Qemu-devel] [PATCH 1/1] nbd: fix NBD_CMD_CACHE negitiation according to the NBD specification

[Denis V. Lunev]

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.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
CC: Valery Vdovin <valery.vdovin@acronis.com>
CC: Eric Blake <eblake@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 include/block/nbd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 4638c839f5..4377fa502c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -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. */
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/1] nbd: fix NBD_CMD_CACHE negitiation according to the NBD specification

[Eric Blake]

On 10/4/18 5:03 AM, Denis V. Lunev wrote:
> Commit bc37b06a5 was made very bad thing, it has been added
> NBD_FLAG_SEND_CACHE flag for negotiation.

Oof. Probably my fault for not doing a careful review against the 
upstream spec.

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

And a kernel that honors that bit can cause qemu to misbehave. Yeah, 
that's definitely undesirable.

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

Oh fun - a typo in the NBD spec (should be absence). I'll fix that 
separately.

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

Correct. Since the kernel is already one client that supports 
CAN_MULTI_CONN, qemu 3.0 was wrong for declaring the wrong bit value.

> 
> Thus the commit fixes negotiation flag in QEMU accoring to the

s/according/according/

> 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 <den@openvz.org>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> CC: Valery Vdovin <valery.vdovin@acronis.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   include/block/nbd.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 4638c839f5..4377fa502c 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -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.

Reviewed-by: Eric Blake <eblake@redhat.com>

Will queue through my NBD tree.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 1/1] nbd: fix NBD_CMD_CACHE negitiation according to the NBD specification

[Vladimir Sementsov-Ogievskiy]

04.10.2018 15:27, Eric Blake wrote:
> On 10/4/18 5:03 AM, Denis V. Lunev wrote:
>> Commit bc37b06a5 was made very bad thing, it has been added
>> NBD_FLAG_SEND_CACHE flag for negotiation.
>
> Oof. Probably my fault for not doing a careful review against the 
> upstream spec.

mostly my, to introduce this =(. really too bad

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

>
>>
>> "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
>
> Oh fun - a typo in the NBD spec (should be absence). I'll fix that 
> separately.
>
>> 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.
>
> Correct. Since the kernel is already one client that supports 
> CAN_MULTI_CONN, qemu 3.0 was wrong for declaring the wrong bit value.
>
>>
>> Thus the commit fixes negotiation flag in QEMU accoring to the
>
> s/according/according/
>
>> 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 <den@openvz.org>
>> CC: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> CC: Valery Vdovin <valery.vdovin@acronis.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   include/block/nbd.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/block/nbd.h b/include/block/nbd.h
>> index 4638c839f5..4377fa502c 100644
>> --- a/include/block/nbd.h
>> +++ b/include/block/nbd.h
>> @@ -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.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Will queue through my NBD tree.
>

I vote for adding all values at least as a comment

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/1] nbd: fix NBD_CMD_CACHE negitiation according to the NBD specification

[Denis V.Lunev]

s/negitiation/negotiation/

On 10/04/2018 01:03 PM, Denis V. Lunev wrote:
> 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.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> CC: Valery Vdovin <valery.vdovin@acronis.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/block/nbd.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 4638c839f5..4377fa502c 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -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. */

[Qemu-devel] [PATCH 2/2] nbd: add all possible transmission flags supported by NBD

[Denis V. Lunev]

This is done to avoid silly mistakes like one with wrong value of
NBD_FLAG_SEND_CACHE flag.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
CC: Eric Blake <eblake@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 include/block/nbd.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 4377fa502c..ea8e792de0 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -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
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/1] nbd: fix NBD_CMD_CACHE negitiation according to the NBD specification

[Eric Blake]

On 10/4/18 7:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> 04.10.2018 15:27, Eric Blake wrote:
>> On 10/4/18 5:03 AM, Denis V. Lunev wrote:
>>> Commit bc37b06a5 was made very bad thing, it has been added
>>> NBD_FLAG_SEND_CACHE flag for negotiation.
>>
>> Oof. Probably my fault for not doing a careful review against the
>> upstream spec.
> 
> mostly my, to introduce this =(. really too bad
> 
>>
>>> 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:
>>
>> 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.

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.


>> 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 <eblake@redhat.com>
>>
>> Will queue through my NBD tree.
>>
> 
> I vote for adding all values at least as a comment
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org