Re: [Xen-devel] [PATCH 2 of 4 v3] blkif.h: Provide more complete documentation of the blkif interface

["Justin T. Gibbs" <justing@xxxxxxxxxxxxxxxx>]

On Feb 23, 2012, at 11:50 AM, Konrad Rzeszutek Wilk wrote:

> On Wed, Feb 22, 2012 at 11:14:31AM +0000, Ian Campbell wrote:
>> On Tue, 2012-02-21 at 21:36 +0000, Konrad Rzeszutek Wilk wrote:
>> [...]
>>>> With that in mind, I believe that all of the required information
>>>> about discard is still present in blkif.h, but perhaps presented
>>>> differently or moved to different sections.  Can you be more
>>>> specific about the information that you feel is missing here and
>>>> in the other places you noted below?
>>> 
>>> The original statement about how the backend would determine how
>>> to expose this information.
>> 
>> Please can you quote the actual text of the statements which you think
>> are missing and/or should be retained so we know precisely what you are
>> talking about.
> 
> Ah, it is in the Notes section. However the information that is not present 
> in 4) is
> 
>       The discard-alignment parameter indicates how many bytes
>       the beginning of the partition is offset from the internal allocation 
> unit's
>       natural alignment. Do not confuse this with natural disk alignment 
> offset.
> 
> [granted we don't expose the natural disk alignment offset right now, so maybe
> that comment is useless]

I removed the old text because it was imprecise (what is "natural
disk alignment" and how is it different from "discard natural
alignment"?) and, even if you understand its intended meaning, there
is no information provided in the interface to allow for this type
of confusion.

I think there would be tremendous value in exporting a XenBus node
to indicate the physical sector size.  I would be happy to propose
this feature in a different patch set and clarify the three different
allocation sizes that are published.

This email did cause me to go review the description for sector-size.
I have now changed it to be:

* sector-size
 *      Values:         <uint32_t>
 *
 *      The size, in bytes, of the individually addressible data blocks
 *      on the backend device.

It used to say "The native sector size, in bytes, of the backend
device." which it isn't.  "sector-size"'d access may be emulated.

> and 6):
>       Devices that support discard functionality may
>       internally allocate space using units that are bigger than the logical 
> block
>       size.  The discard-granularity parameter indicates the size of the 
> internal
>       allocation unit in bytes if reported by the device. Otherwise the
>       discard-granularity will be set to match the device's physical block 
> size.
>       It is the minimum size you can discard.
> 
> [though parts of that comment are there]

Just the last sentence here is not directly replicated in the patch set.
To my mind, the definition of this value is clear and it is up to the
implementor to publish a valid value or not claim support for this
feature.  How this is achieved does not belong in the blkif spec.

>> Perhaps details which aren't part of the formal spec, e.g.
>> implementation tips, details of various OSes implementation choices etc
>> could be kept elsewhere, e.g. on the wiki?
> 
> On the Document day when I started documenting the PSE != PAT and the page
> pinning procedure - I had no idea where to stick that explanation of what 
> goes behind
> the scene and how to properly do it so Ian Jackson suggested in the header of
> the function. That is an implementation tip of how to properly do it
> so you don't shoot yourself in the foot.
> 
> Where should something similar for blkback be so it can grow along with the 
> source?

Now that they are documented, the features aren't an area ripe for
foot shooting.  The state transitions are, and I would support
adding more state diagrams to this header file (e.g.  tool stack
initiated closes, the impact of the "online" attribute, etc.).

--
Justin
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel