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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Feb 21, 2012 at 01:59:15PM -0700, Justin T. Gibbs wrote:
> 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

OK.
> 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).

And that is what I hate about specs. They don't provide enough details
on the corner cases or what an implentation could look like.

> 
> 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.

> 
> >> + *
> >> + * 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?

I just think that having more information (even duplicate) is
better. But that has the disadvantage that it can get out of sync.

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

Well, the desire (mine) is to include as much details (or even more) in the
spec so that it can be read as a novel if desired.

But I will defer to Ian - if he is OK then this shouldn't gate
the acceptance of this patch which is a step forward.

.. snip..
> >> + * 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.

Fair enough.

> 
> --
> Justin

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