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 21, 2012, at 7:10 AM, Konrad Rzeszutek Wilk wrote:

> On Mon, Feb 20, 2012 at 11:07:23AM -0700, Justin T. Gibbs wrote:
>>  o Document the XenBus nodes used in this protocol.
>>  o Add a state diagram illustrating the roles and responsibilities
>>    of both the front and backend during startup.
>>  o Correct missed BLKIF_OP_TRIM => BLKIF_OP_DISCARD conversion in a comment.
>> 
>> No functional changes.
> 
> Some comments below.

…

>> + *------------------------- Backend Device Properties 
>> -------------------------
>> + *
>> + * discard-aligment
>> + *      Values:         <uint32_t>
>> + *      Default Value:  0
>> + *      Notes:          1, 2
>> + *
>> + *      The offset, in bytes from the beginning of the virtual block device,
>> + *      to the first, addressable, discard extent on the underlying device.
> 
> You are also missing the rest of the information about it. Please include the
> details that the previous comment had.

The strategy I have employed in all of this documentation is to
treat blkif.h as a formal spec.  This means that blkif needs to
describe how its terminology maps to industry standards (e.g. discard
means trim/unmap) and how these requests can be communicated between
front and back driver.  It should not, in my opinion, attempt to
document information that is managed in other specs (e.g. T10/T13)
and could easily become stale in blkif.h.  It also should avoid
stating information that someone reasonably proficient in writing
(storage) device drivers is expected to know (e.g. don't say you
support something that you don't).

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?

>> + *
>> + * discard-secure
>> + *      Values:         0/1 (boolean)
>> + *      Default Value:  0
>> + *
>> + *      A value of "1" indicates that the backend can process 
>> BLKIF_OP_DISCARD
>> + *      requests with the BLKIF_DISCARD_SECURE flag set.
> 
> 
> That is not very specific to what this does. It just says X will do Y.

Did you mean, "That is not very specific to what BLKIF_DISCARD_SECURE
does."?  BLKIF_DISCARD_SECURE is documented in the comment section
for BLKIF_OP_DISCARD.  The pertinent text is: "If  the BLKIF_DISCARD_SECURE
flag is set on the request, all copies of the discarded region on
the device must be rendered unrecoverable before the command returns."
Is there a compelling reason to document it here instead?

The comment block here mirrors the language of all the other feature
flags.  Do you believe they should be changed in some way too?

>> - * or a BLKIF_RSP_EOPNOTSUPP  should be returned.
>> - * More information about trim/unmap operations at:
>> + * This operation is analogous to performing a trim (ATA) or unamp (SCSI),
> 
> unmap.

Good catch.

> 
>> + * command on a native device.
> 
> And you might want to mention what the previous comment did - that this is
> a hint to the backend.

The proposed text already does this:

    "Indicate to the backend device that a region of storage is no
     longer in use, and may be discarded at any time without impact
     to the client."

If the backend was required to discard the region, I would have
used "shall" or "must", not "may".  This is the typical language
convention of standards documents.

--
Justin


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