Re: [Qemu-devel] [Nbd] [PATCH 2/2] NBD proto: add GET_LBA_STATUS extension

[Wouter Verhelst <w@uter.be>]

[-- Attachment #1: Type: text/plain, Size: 3362 bytes --]

On Thu, Mar 24, 2016 at 11:43:18AM +0300, Pavel Borzenkov wrote:
> On Wed, Mar 23, 2016 at 06:58:34PM +0100, Wouter Verhelst wrote:
> > On Wed, Mar 23, 2016 at 05:16:02PM +0300, Denis V. Lunev wrote:
> > > +    2. Block dirtiness state
> > > +
> > > +    Upon receiving an `NBD_CMD_GET_LBA_STATUS` command with command flags
> > > +    field set to `NBD_FLAG_GET_DIRTY` (0x1), the server MUST return
> > > +    the dirtiness status of the device. The following dirtiness states
> > > +    are defined for the command:
> > > +
> > > +      - `NBD_STATE_DIRTY` (0x0), LBA extent is dirty;
> > > +      - `NBD_STATE_CLEAN` (0x1), LBA extent is clean.
> > > +
> > > +    Generic NBD client implementation without knowledge of a particular NBD
> > > +    server operation MUST NOT make any assumption on the meaning of the
> > > +    NBD_STATE_DIRTY or NBD_STATE_CLEAN states.
> > 
> > That makes it a useless call. A server can read /dev/random to decide
> > whether to send STATE_DIRTY or STATE_CLEAN, and still be compliant with
> > this spec.
> > 
> > Either the spec should define what it means for a block to be in a dirty
> > state, or it should not talk about it.
> 
> What I was trying to say with this sentence is that without knowing what
> is currently going on on the server side, the DIRTY state has no
> meaning. If we are doing incremental backup DIRTY state might represent blocks
> that have changed since last backup. For mirroring - blocks that are yet
> to be migrated. And for the same block device different sets of DIRTY
> ranges might be returned in response to this command. Basically, the
> meaning of DIRTY depends on the context. 
> 
> Should I state in the spec, that the meaning of DIRTY state is
> implementation-specific? I see that NBD_REP_SERVER already uses this
> approach.

That's still meaningless without a way for the client to detect what the
server-side implementation actually is. The protocol doesn't have that.

This is okay for NBD_REP_SERVER, because the extra information there is
*also* defined to be "UTF-8 encoded data suitable for direct display to
a human being", which means it just allows the server to pass on some
extra info (e.g., qemu could use it to state whether it's in use by a
live VM, or what the backend storage pool is, etc), but the data there
is not going to be critical.

For this proposed message, however, that is not the case; the whole
point of this part of your proposal seems to be to tell the client
whether some part of the export is "dirty" or not. Now I'm not saying we
need to fully define what it means for a part of the backend to be
"dirty" or not.  It's okay to leave part of the meaning in the dark,
leaving that implementation-defined. But saying "must not make any
assumption" basically means "give me some random metadata, and I'll
throw it away then because there's nothing I can do with it".

Alternatively, we could define more states than just "dirty" and
"clean", and have those be more strictly defined than what your current
proposal says.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]