Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Eric Blake]

On 01/20/2017 12:00 PM, Alex Bligh wrote:
> 
>> On 20 Jan 2017, at 17:04, Vladimir Sementsov-Ogievskiy 
>>  wrote:
>>
>> About extents: is 32bit length enough? We will have to send 4096 for empty 
>> 16tb disk..
> 
> The nbd protocol uniformly uses 32 bit lengths (for better or for worse). 
> This is baked into the specification heavily.
> 
> I'm not sure sending 4,096 items for an empty 16TB disk is any great hardship 
> to be honest.

If it truly matters, an idea that has already been floated on the list
is to add a new NBD_CMD_FLAG_SCALE (or some other spelling) that states
that a particular command is sending lengths scaled by a particular
factor (by the block size? obviously it would have to be better
specified).  Under this idea, the scaling factor would allow you to
report larger extents for fewer back-and-forth operations, but it gets
tricky if the scaling is allowed to get larger than the minimum
granularity between extent changes.

The other idea that has been floated is a way to report whether the
entire export is known to be all-zero content at the time the client
connects, which would trade 4096+ queries (you'd actually have to do
more than 4096 queries, since length is < 4G, not <= 4G) with a single
piece of information at the time the client connects.

Either way, discussion on such enhancements are probably worth a new thread.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Alex Bligh]

> On 20 Jan 2017, at 17:04, Vladimir Sementsov-Ogievskiy 
>  wrote:
> 
> About extents: is 32bit length enough? We will have to send 4096 for empty 
> 16tb disk..

The nbd protocol uniformly uses 32 bit lengths (for better or for worse). This 
is baked into the specification heavily.

I'm not sure sending 4,096 items for an empty 16TB disk is any great hardship 
to be honest.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Vladimir Sementsov-Ogievskiy]

About extents: is 32bit length enough? We will have to send 4096 for 
empty 16tb disk..



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Vladimir Sementsov-Ogievskiy]

13.01.2017 13:29, Alex Bligh wrote:

On 13 Jan 2017, at 09:48, Vladimir Sementsov-Ogievskiy 
 wrote:

12.01.2017 16:11, Alex Bligh wrote:

On 12 Jan 2017, at 07:05, Vladimir Sementsov-Ogievskiy 
 wrote:

Yes this is better. But is it actually needed to force contexts have some safe 
default? If context wants it may define such default without this requirement.. 
So, should it be requirement at all?

I've changed this to:

 of the file), a server MAY reply with a single block status
 descriptor with *length* matching the requested length, rather than
 reporting the error; in this case the context MAY mandate the
 status returned.



Hmm, I don't understand. So, it MAY mandate and therefore MAY NOT do it? And 
what client should think, if server replies with one chunk matching the request 
length and not mandate the status?

Some contexts may mandate a particular value (so for instance the allocation 
context might mandate 0).

Some contexts may not mandate a particular value, in which case the 
interpretation is dependent upon the context (just like any other status 
value). EG a context which returned an status of 7 if the range contained a 
prime number, and else 3, could carry on doing that.

As it doesn't make sense to interpret status returns without an understanding 
of the particular context, we might as well simply extend this to 'beyond the 
range' returns - as I think you pointed out!



>>> The status flags are intentionally defined so that a server MAY 
always safely report a status of 0 for any block


- Actually, status flags are _not_ defined. (each context defines it's 
own status flags)



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Alex Bligh]

> On 13 Jan 2017, at 09:48, Vladimir Sementsov-Ogievskiy 
>  wrote:
> 
> 12.01.2017 16:11, Alex Bligh wrote:
>>> On 12 Jan 2017, at 07:05, Vladimir Sementsov-Ogievskiy 
>>>  wrote:
>>> 
>>> Yes this is better. But is it actually needed to force contexts have some 
>>> safe default? If context wants it may define such default without this 
>>> requirement.. So, should it be requirement at all?
>> I've changed this to:
>> 
>> of the file), a server MAY reply with a single block status
>> descriptor with *length* matching the requested length, rather than
>> reporting the error; in this case the context MAY mandate the
>> status returned.
>> 
>> 
> 
> Hmm, I don't understand. So, it MAY mandate and therefore MAY NOT do it? And 
> what client should think, if server replies with one chunk matching the 
> request length and not mandate the status?

Some contexts may mandate a particular value (so for instance the allocation 
context might mandate 0).

Some contexts may not mandate a particular value, in which case the 
interpretation is dependent upon the context (just like any other status 
value). EG a context which returned an status of 7 if the range contained a 
prime number, and else 3, could carry on doing that.

As it doesn't make sense to interpret status returns without an understanding 
of the particular context, we might as well simply extend this to 'beyond the 
range' returns - as I think you pointed out!

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Vladimir Sementsov-Ogievskiy]

12.01.2017 16:11, Alex Bligh wrote:

On 12 Jan 2017, at 07:05, Vladimir Sementsov-Ogievskiy 
 wrote:

Yes this is better. But is it actually needed to force contexts have some safe 
default? If context wants it may define such default without this requirement.. 
So, should it be requirement at all?

I've changed this to:

 of the file), a server MAY reply with a single block status
 descriptor with *length* matching the requested length, rather than
 reporting the error; in this case the context MAY mandate the
 status returned.




Hmm, I don't understand. So, it MAY mandate and therefore MAY NOT do it? 
And what client should think, if server replies with one chunk matching 
the request length and not mandate the status?



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Alex Bligh]

> On 12 Jan 2017, at 11:43, Vladimir Sementsov-Ogievskiy 
>  wrote:
> 
> From "NBD_OPT_LIST_META_CONTEXT (9)":
>> The server MUST either reply with an error (for instance EINVAL if the 
>> option is not supported), or reply with a list of NBD_REP_META_CONTEXT 
>> replies followed by NBD_REP_ACK.:
> NBD_REP_ERR_UNSUP for the case in braces? Should it be normal option reply 
> packet?

Thanks, fixed.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Alex Bligh]

> On 12 Jan 2017, at 07:05, Vladimir Sementsov-Ogievskiy 
>  wrote:
> 
> Yes this is better. But is it actually needed to force contexts have some 
> safe default? If context wants it may define such default without this 
> requirement.. So, should it be requirement at all?

I've changed this to:

of the file), a server MAY reply with a single block status
descriptor with *length* matching the requested length, rather than
reporting the error; in this case the context MAY mandate the
status returned.


-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Vladimir Sementsov-Ogievskiy]

(Sorry, it may be a bit out of topic, but I hope it is comfortable for 
all that I just comment current version of the feature by answering 
cover letter of last related patch set)


From |"NBD_OPT_LIST_META_CONTEXT| (9)":
The server MUST either reply with an error (for instance |EINVAL| if 
the option is not supported), or reply with a list of 
|NBD_REP_META_CONTEXT| replies followed by |NBD_REP_ACK|.:


NBD_REP_ERR_UNSUP for the case in braces? Should it be normal option 
reply packet?




--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Vladimir Sementsov-Ogievskiy]

11.01.2017 22:00, Alex Bligh wrote:

On 11 Jan 2017, at 15:31, Vladimir Sementsov-Ogievskiy 
 wrote:


If an error occurs, the server SHOULD set the appropriate error code in the 
error field of an error chunk. However, if the error does not involve invalid 
usage (such as a request beyond the bounds of the file), a server MAY reply 
with a single block status descriptor with length matching the requested 
length, and status of 0 rather than reporting the error.

- single block status descriptor for each context? Isn't it implementation 
defined? Or we finally decided to force 0 status to be safe default for all 
contexts? If it is so, it would be better to describe this separately. However, 
personally, I'd prefer to not define contexts internal semantics at all.

I think this is Wouter's wording, but I think 'a status appropriate to the 
context' would be better. Each context then needs to define what that is. 
Either that or 'the context's default status' and that should be in the 
definition of the context.



Yes this is better. But is it actually needed to force contexts have 
some safe default? If context wants it may define such default without 
this requirement.. So, should it be requirement at all?



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Alex Bligh]

> On 11 Jan 2017, at 15:31, Vladimir Sementsov-Ogievskiy 
>  wrote:
> 
> >>> If an error occurs, the server SHOULD set the appropriate error code in 
> >>> the error field of an error chunk. However, if the error does not involve 
> >>> invalid usage (such as a request beyond the bounds of the file), a server 
> >>> MAY reply with a single block status descriptor with length matching the 
> >>> requested length, and status of 0 rather than reporting the error.
> - single block status descriptor for each context? Isn't it implementation 
> defined? Or we finally decided to force 0 status to be safe default for all 
> contexts? If it is so, it would be better to describe this separately. 
> However, personally, I'd prefer to not define contexts internal semantics at 
> all.

I think this is Wouter's wording, but I think 'a status appropriate to the 
context' would be better. Each context then needs to define what that is. 
Either that or 'the context's default status' and that should be in the 
definition of the context.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Vladimir Sementsov-Ogievskiy]

from current version:

>>> If an error occurs, the server SHOULD set the appropriate error 
code in the error field of an error chunk. However, if the error does 
not involve invalid usage (such as a request beyond the bounds of the 
file), a server MAY reply with a single block status descriptor with 
/length/ matching the requested length, and /status/ of 0 rather than 
reporting the error.


- single block status descriptor for each context? Isn't it 
implementation defined? Or we finally decided to force 0 status to be 
safe default for all contexts? If it is so, it would be better to 
describe this separately. However, personally, I'd prefer to not define 
contexts internal semantics at all.


--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Alex Bligh]

> On 27 Dec 2016, at 14:09, Vladimir Sementsov-Ogievskiy 
>  wrote:
> 
> What was the reason for it? Why not to negotiate forced structured read 
> separately? Actually, this spec forces any server, which wants to implement 
> structured reply implement structured read too. But what if it don't want to? 
> If it only wants to implement BLOCK_STATUS?
> 
> So, what about changing it, to allow BLOCK_STATUS (or other future structured 
> replies) without structured read? Structured read is good only for sparse 
> formats, when BLOCK_STATUS is more global. I understand, that servers may 
> implement simple (and useless) one-chunk structured read, but I think that it 
> is better to fix the spec, to not provoke servers use such workaround.

In essence the current reply format is broken, because it does not provide a 
means of delivering an error mid reply AND expects fixed length replies. Block 
Status is the poster child for something that benefits from structured replies.

So, BLOCK_STATUS requires structured replies. We thus have the choice between:
* Allowing structured replies to be implemented on a per-command basis
* Mandating that if structured replies are implemented, they are implemented 
universally

The second option is far simpler, and is what the current structured reply 
extension sets out. It's also a good nudge to server implementers (me included) 
to implement structured replies.

Your assumption that structured replies are only good for sparse formats is 
incorrect. It's good for anything that wants to include sensible error 
handling. As a server author, error handling at the moment is a pain. Most of 
us ALREADY break up large read requests (to avoid malloc() of huge amounts of 
memory), so we already have a read loop. If we don't then the 'one chunk' 
method is perfectly acceptable. If I implemented structured replies at all, I'd 
be sharing the code between multiple users in any case.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Eric Blake]

On 12/27/2016 08:09 AM, Vladimir Sementsov-Ogievskiy wrote:
> A bit out of topic, but...
> 
>> structured replies via `NBD_OPT_STRUCTURED_REPLY`.  Conversely, if
>> structured replies are negotiated, the server MUST use a
>> structured reply for any response with a payload, and MUST NOT use
>> a simple reply for `NBD_CMD_READ` (even for the case of an early
>> `EINVAL` due to bad flags), but MAY use either a simple reply or a
>> structured reply to all other requests.
> 
> What was the reason for it? Why not to negotiate forced structured read
> separately?

Because the whole reason we (want to) introduce structured replies IS to
fix the inability to do a partial read or an efficient read of zeroes.
The fact that structured reads make other extensions possible is icing
on the cake, but if you are going to implement structured replies at
all, you might as well make reads do it (since reads are mandatory,
while all other commands that utilize structured replies are optional).

> Actually, this spec forces any server, which wants to
> implement structured reply implement structured read too. But what if it
> don't want to? If it only wants to implement BLOCK_STATUS?

We intentionally do not want to permit such a server. Any server that
wants to implement BLOCK_STATUS must also implement structured reads.

> 
> So, what about changing it, to allow BLOCK_STATUS (or other future
> structured replies) without structured read? Structured read is good
> only for sparse formats,

Not true - it is also good for error recovery even on non-sparse
exports.  The existing read command is flawed in that it cannot be
implemented with partial read support - once the server has started
sending data, it MUST finish sending the number of bytes requested by
the client, which means the server MUST either buffer the read up front
(to ensure no read error is possible once the data sending is started),
or MUST disconnect if a read error is detected partway through.  With
structured reads, you can implement much more efficient servers that
start sending the reply right away without buffering, but which can
still error out on a read error partway through.

> when BLOCK_STATUS is more global. I understand,
> that servers may implement simple (and useless) one-chunk structured
> read, but I think that it is better to fix the spec, to not provoke
> servers use such workaround.

To date, we don't know of ANY servers that implement structured replies
at all, whether for structured reads or for BLOCK_STATUS.  I'm working
on qemu patches to make qemu implement both, and it will serve as an
example of how easy or hard it is to implement things.  But I see NO
reason to weaken the spec to allow structured BLOCK_STATUS without
structured reads.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Vladimir Sementsov-Ogievskiy]

A bit out of topic, but...


structured replies via `NBD_OPT_STRUCTURED_REPLY`.  Conversely, if
structured replies are negotiated, the server MUST use a
structured reply for any response with a payload, and MUST NOT use
a simple reply for `NBD_CMD_READ` (even for the case of an early
`EINVAL` due to bad flags), but MAY use either a simple reply or a
structured reply to all other requests.


What was the reason for it? Why not to negotiate forced structured read 
separately? Actually, this spec forces any server, which wants to 
implement structured reply implement structured read too. But what if it 
don't want to? If it only wants to implement BLOCK_STATUS?


So, what about changing it, to allow BLOCK_STATUS (or other future 
structured replies) without structured read? Structured read is good 
only for sparse formats, when BLOCK_STATUS is more global. I understand, 
that servers may implement simple (and useless) one-chunk structured 
read, but I think that it is better to fix the spec, to not provoke 
servers use such workaround.



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Vladimir Sementsov-Ogievskiy]

14.12.2016 21:17, Alex Bligh wrote:

On 14 Dec 2016, at 17:55, Vladimir Sementsov-Ogievskiy 
 wrote:


Hmmm... Well in the '*' case, it's up to the namespace as to how it parses 
things, so '*' could be prohibited entirely or mean 'return the first 20 
matching' or similar. So that's less of a problem. And 'set all' doesn't exist 
(unlike 'list all').

I think in the LIST case we should allow the server to omit contexts under 
certain circumstances, and allow SET to return ETOOBIG.


We can't prohibit '*' as query string as implementation defined. And we 
shouldn't (I think) define its meaning.

Those two statements appear to me to contradict each other. The current 
documentation *does* define the query string (to the right of the colon) as 
implementation defined. I'm thus not sure what you mean.


Opposite, we can define, that query must not select more than 20 contexts, but 
I'm not sure that this is good.

Each query can select from only one namespace in the current document (Wouter 
explained why). However, you can specify multiple queries.

I don't think we need to define a hard limit of a particular number.


So, do you mean

  list('backup:modtime>*') -> empty list

The result of that would depend on however the 'backup' context was defined, 
meaning it had the options of:
* ETOOBIG
* Listing some subset of available contexts (arguably 'none' is a subset)


  set('backup:modtime>*') -> ETOOBIG

Again, the result of that would depend on however the 'backup' context was 
defined, meaning it had the options of:
* ETOOBIG
* Listing some subset of available contexts (arguably 'none' is a subset)
... and it need not make the same choice as above, but I agree it would be 
logical for it to do so.

As any form of wildcarding within a query refers to one particular namespace 
(as a query by its nature specifies a single namespace), we don't have to worry 
about the way wildcarding works in the standard, as Wouter pointed out. We 
merely need to provide that ETOOBIG and listing a subset are acceptable 
responses.

What we do need to decide is what the response to list() (i.e. a list with a 
zero length list of queries) does. It's currently defined to return every 
context from every namespace. Options would include
* ETOOBIG
* Listing some subset of available contexts (arguably 'none' is a subset)
* Allowing abbreviation of algorithmically specified contexts (e.g. in this 
case just returning 'backup:' to represent all available backup contexts)


Shouldn't we add some flags to REP_META_CONTEXT, for client to be 
insure, is returned string a direct context name or some kind of 
wildcard?|| Just a flags field, with one flag defined for now: 
|NBD_REP_META_CONTEXT_LEAF and others reserved.|



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Alex Bligh]

> On 14 Dec 2016, at 17:55, Vladimir Sementsov-Ogievskiy 
>  wrote:
> 
>> Hmmm... Well in the '*' case, it's up to the namespace as to how it parses 
>> things, so '*' could be prohibited entirely or mean 'return the first 20 
>> matching' or similar. So that's less of a problem. And 'set all' doesn't 
>> exist (unlike 'list all').
>> 
>> I think in the LIST case we should allow the server to omit contexts under 
>> certain circumstances, and allow SET to return ETOOBIG.
>> 
> 
> We can't prohibit '*' as query string as implementation defined. And we 
> shouldn't (I think) define its meaning.

Those two statements appear to me to contradict each other. The current 
documentation *does* define the query string (to the right of the colon) as 
implementation defined. I'm thus not sure what you mean.

> Opposite, we can define, that query must not select more than 20 contexts, 
> but I'm not sure that this is good.

Each query can select from only one namespace in the current document (Wouter 
explained why). However, you can specify multiple queries.

I don't think we need to define a hard limit of a particular number.

> So, do you mean
> 
>  list('backup:modtime>*') -> empty list

The result of that would depend on however the 'backup' context was defined, 
meaning it had the options of:
* ETOOBIG
* Listing some subset of available contexts (arguably 'none' is a subset)

>  set('backup:modtime>*') -> ETOOBIG

Again, the result of that would depend on however the 'backup' context was 
defined, meaning it had the options of:
* ETOOBIG
* Listing some subset of available contexts (arguably 'none' is a subset)
... and it need not make the same choice as above, but I agree it would be 
logical for it to do so.

As any form of wildcarding within a query refers to one particular namespace 
(as a query by its nature specifies a single namespace), we don't have to worry 
about the way wildcarding works in the standard, as Wouter pointed out. We 
merely need to provide that ETOOBIG and listing a subset are acceptable 
responses.

What we do need to decide is what the response to list() (i.e. a list with a 
zero length list of queries) does. It's currently defined to return every 
context from every namespace. Options would include
* ETOOBIG
* Listing some subset of available contexts (arguably 'none' is a subset)
* Allowing abbreviation of algorithmically specified contexts (e.g. in this 
case just returning 'backup:' to represent all available backup contexts)

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Vladimir Sementsov-Ogievskiy]

14.12.2016 20:36, Alex Bligh wrote:

On 14 Dec 2016, at 17:05, Vladimir Sementsov-Ogievskiy 
 wrote:

However, this raises another question. Wouter deliberately made the
query format freeform so that you could e.g. set a context like:

backup:modtime>201612081034

which might (in theory) return a list of blocks which are newer than
the given timestamp. It would clearly be impossible to return all such
contexts. I wonder if we should carve out an exception here.



Interesting. Even query 'backup:modtime>*' would be a problem, not only empty 
query list.

Actually, we do not need to 'list contexts', as it is about management, not 
about data transfer. We only need a way to check, that particular query selects 
all needed contexts and no others. Just to be sure that we are know, what we 
will select by NBD_OPT_SET_META_CONTEXT with _same_ query.

So, I suggest just to say, that _LIST_ can return error if too much contexts 
are selected. And same should be done for _SET_. And it should be documented 
that this result of query (list or error) should be equal for these two 
commands.

(two CCs that always bounce removed)

Hmmm... Well in the '*' case, it's up to the namespace as to how it parses 
things, so '*' could be prohibited entirely or mean 'return the first 20 
matching' or similar. So that's less of a problem. And 'set all' doesn't exist 
(unlike 'list all').

I think in the LIST case we should allow the server to omit contexts under 
certain circumstances, and allow SET to return ETOOBIG.



We can't prohibit '*' as query string as implementation defined. And we 
shouldn't (I think) define its meaning. Opposite, we can define, that 
query must not select more than 20 contexts, but I'm not sure that this 
is good.


So, do you mean

  list('backup:modtime>*') -> empty list
  set('backup:modtime>*') -> ETOOBIG

? For me this looks strange.

--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Vladimir Sementsov-Ogievskiy]

14.12.2016 19:38, Alex Bligh wrote:

Vladimir,


+non-zero number of metadata contexts during negotiation. Servers SHOULD
+reply to clients sending `NBD_CMD_BLOCK_STATUS without

backquote

Fixed


+If zero queries are sent, then the server MUST return all
+the metadata contexts it knows about.

I think that 'all .. it knows about' is too much. What about 'return all 
available ..'? Anyway 'all ... it knows about' actually equals to 'all ... it 
wants'. There may be some private, or unrelated contexts, for example..

This was not my wording, but I've changed it anyway to:

 If zero queries are sent, then the server MUST return all
 the metadata contexts that are available tothe client to select
 on the given export with `NBD_OPT_SET_META_CONTEXT`.

I think if they are available to select, we should list them. Thanks
also for reminding me to document why I put the export name into the
_LIST_ data (as it is for _SET_).

However, this raises another question. Wouter deliberately made the
query format freeform so that you could e.g. set a context like:

backup:modtime>201612081034

which might (in theory) return a list of blocks which are newer than
the given timestamp. It would clearly be impossible to return all such
contexts. I wonder if we should carve out an exception here.



Interesting. Even query 'backup:modtime>*' would be a problem, not only 
empty query list.


Actually, we do not need to 'list contexts', as it is about management, 
not about data transfer. We only need a way to check, that particular 
query selects all needed contexts and no others. Just to be sure that we 
are know, what we will select by NBD_OPT_SET_META_CONTEXT with _same_ query.


So, I suggest just to say, that _LIST_ can return error if too much 
contexts are selected.|And same should be done for _SET_. And it should 
be documented that this result of query (list or error) should be equal 
for these two commands.

|


--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Alex Bligh]

> On 14 Dec 2016, at 17:05, Vladimir Sementsov-Ogievskiy 
>  wrote:
>> 
>> However, this raises another question. Wouter deliberately made the
>> query format freeform so that you could e.g. set a context like:
>> 
>>backup:modtime>201612081034
>> 
>> which might (in theory) return a list of blocks which are newer than
>> the given timestamp. It would clearly be impossible to return all such
>> contexts. I wonder if we should carve out an exception here.
>> 
>> 
> 
> Interesting. Even query 'backup:modtime>*' would be a problem, not only empty 
> query list.
> 
> Actually, we do not need to 'list contexts', as it is about management, not 
> about data transfer. We only need a way to check, that particular query 
> selects all needed contexts and no others. Just to be sure that we are know, 
> what we will select by NBD_OPT_SET_META_CONTEXT with _same_ query.
> 
> So, I suggest just to say, that _LIST_ can return error if too much contexts 
> are selected. And same should be done for _SET_. And it should be documented 
> that this result of query (list or error) should be equal for these two 
> commands.

(two CCs that always bounce removed)

Hmmm... Well in the '*' case, it's up to the namespace as to how it parses 
things, so '*' could be prohibited entirely or mean 'return the first 20 
matching' or similar. So that's less of a problem. And 'set all' doesn't exist 
(unlike 'list all').

I think in the LIST case we should allow the server to omit contexts under 
certain circumstances, and allow SET to return ETOOBIG.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Alex Bligh]

> On 14 Dec 2016, at 16:58, Eric Blake  wrote:
> 
> s/botht he/both the/

Thanks - fixed

--
Alex Bligh






signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Eric Blake]

On 12/14/2016 09:08 AM, Alex Bligh wrote:
> (NB: I've already applied this and pushed it)
> 
> * Change NBD_OPT_LIST_METADATA etc. to explicitly send a list of queries
>   and add a count of queries so we can extend the command later (rather than
>   rely on the length of option)
> 
> * For NBD_OPT_LIST_METADATA make absence of any query (as opposed to zero
>   length query) list all contexts, as absence of any query is now simple.
> 
> * Move definition of namespaces in the document to somewhere more appopriate.
> 
> * Various other minor changes as discussed on the mailing list
> 

> +Metadata contexts are identified by their names. The name MUST
> +consist of a namespace, followed by a colon, followed by a leaf-name.
> +The namespace and the leaf-name must each consist entirely of
> +printable non-whitespace UTF-8 characters other than colons,
> +and be non-empty. The entire name (namespace, colon and leaf-name)
> +MUST NOT exceed 255 bytes (and therefore botht he namespace and

s/botht he/both the/


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Alex Bligh]

Vladimir,

>> +non-zero number of metadata contexts during negotiation. Servers SHOULD
>> +reply to clients sending `NBD_CMD_BLOCK_STATUS without
> 
> backquote

Fixed

>> +If zero queries are sent, then the server MUST return all
>> +the metadata contexts it knows about.
> 
> I think that 'all .. it knows about' is too much. What about 'return all 
> available ..'? Anyway 'all ... it knows about' actually equals to 'all ... it 
> wants'. There may be some private, or unrelated contexts, for example..

This was not my wording, but I've changed it anyway to:

If zero queries are sent, then the server MUST return all
the metadata contexts that are available to the client to select
on the given export with `NBD_OPT_SET_META_CONTEXT`.

I think if they are available to select, we should list them. Thanks
also for reminding me to document why I put the export name into the
_LIST_ data (as it is for _SET_).

However, this raises another question. Wouter deliberately made the
query format freeform so that you could e.g. set a context like:

   backup:modtime>201612081034

which might (in theory) return a list of blocks which are newer than
the given timestamp. It would clearly be impossible to return all such
contexts. I wonder if we should carve out an exception here.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] Further tidy-up on block status

[Vladimir Sementsov-Ogievskiy]

14.12.2016 18:08, Alex Bligh wrote:

(NB: I've already applied this and pushed it)

* Change NBD_OPT_LIST_METADATA etc. to explicitly send a list of queries
   and add a count of queries so we can extend the command later (rather than
   rely on the length of option)

* For NBD_OPT_LIST_METADATA make absence of any query (as opposed to zero
   length query) list all contexts, as absence of any query is now simple.

* Move definition of namespaces in the document to somewhere more appopriate.

* Various other minor changes as discussed on the mailing list

Signed-off-by: Alex Bligh 
---
  doc/proto.md | 179 +--
  1 file changed, 112 insertions(+), 67 deletions(-)

diff --git a/doc/proto.md b/doc/proto.md
index e03f434..6955c76 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -683,15 +683,20 @@ by other users, so has been moved out of the 
"experimental" section.
  
  ## Metadata querying
  
-With the availability of sparse storage formats, it is often needed to

-query the status of a particular range and read only those blocks of
-data that are actually present on the block device.
+It is often helpful for the client to be able to query the status
+of a range of blocks. The nature of the status that can be
+queried is in part implementation dependent. For instance,
+the status might represent:
  
-Some storage formats and operations over such formats express a

-concept of data dirtiness. Whether the operation is block device
-mirroring, incremental block device backup or any other operation with
-a concept of data dirtiness, they all share a need to provide a list
-of ranges that this particular operation treats as dirty.
+* in a sparse storage format, whether the relevant blocks are
+  actually present on the backing device for the export; or
+
+* whether the relevant blocks are 'dirty'; some storage formats
+  and operations over such formats express a concept of data dirtiness.
+  Whether the operation is block device mirroring, incremental block
+  device backup or any other operation with a concept of data dirtiness,
+  they all share a need to provide a list of ranges that this
+  particular operation treats as dirty.
  
  To provide such classes of information, the NBD protocol has a generic

  framework for querying metadata; however, its use must first be
@@ -699,9 +704,11 @@ negotiated, and one or more metadata contexts must be 
selected.
  
  The procedure works as follows:
  
-- First, during negotiation, the client MUST select one or more metadata

+- First, during negotiation, if the client wishes to query metadata
+  during transmission, the client MUST select one or more metadata
contexts with the `NBD_OPT_SET_META_CONTEXT` command. If needed, the client
-  can use `NBD_OPT_LIST_META_CONTEXT` to list contexts.
+  can use `NBD_OPT_LIST_META_CONTEXT` to list contexts that the server
+  supports.
  - During transmission, a client can then indicate interest in metadata
for a given region by way of the `NBD_CMD_BLOCK_STATUS` command, where
*offset* and *length* indicate the area of interest. The server MUST
@@ -715,13 +722,37 @@ The procedure works as follows:
of type `NBD_REPLY_TYPE_BLOCK_STATUS`.
  
  A client MUST NOT use `NBD_CMD_BLOCK_STATUS` unless it selected a

-nonzero number of metadata contexts during negotiation. Servers SHOULD
-reply to clients doing so anyway with `EINVAL`.
+non-zero number of metadata contexts during negotiation. Servers SHOULD
+reply to clients sending `NBD_CMD_BLOCK_STATUS without


backquote


+selecting metadata contexts with `EINVAL`.
  
-The reply to the `NBD_CMD_BLOCK_STATUS` request MUST be sent by a

+The reply to the `NBD_CMD_BLOCK_STATUS` request MUST be sent as a
  structured reply; this implies that in order to use metadata querying,
  structured replies MUST be negotiated first.
  
+Metadata contexts are identified by their names. The name MUST

+consist of a namespace, followed by a colon, followed by a leaf-name.
+The namespace and the leaf-name must each consist entirely of
+printable non-whitespace UTF-8 characters other than colons,
+and be non-empty. The entire name (namespace, colon and leaf-name)
+MUST NOT exceed 255 bytes (and therefore botht he namespace and
+leaf-name are guaranteed to be smaller than 255 bytes).
+
+Namespaces MUST be consist of one of the following:
+- `base`, for metadata contexts defined by this document;
+- `nbd-server`, for metadata contexts defined by the
+   implementation that accompanies this document (none
+   currently);
+- `x-*`, where `*` can be replaced by any random string not
+   containing colons, for local experiments. This SHOULD NOT be
+   used by metadata contexts that are expected to be widely used.
+- A third-party namespace from the list below.
+
+Third-party implementations can register additional namespaces by simple
+request to the mailing-list. The following additional third-party namespaces
+are