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

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