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 <address@hidden>
> CC: Vladimir Sementsov-Ogievskiy <address@hidden>
> CC: Valery Vdovin <address@hidden>
> CC: Eric Blake <address@hidden>
> CC: Paolo Bonzini <address@hidden>
> ---
>   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 <address@hidden>

Will queue through my NBD tree.

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