[PATCH] blkif.h: enhance comments related to the discard feature

[PATCH] blkif.h: enhance comments related to the discard feature

[Olaf Hering]

Also fix the name of the discard-alignment property, add the missing 'n'.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 xen/include/public/io/blkif.h | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
index 84eb7fd..515ea90 100644
--- a/xen/include/public/io/blkif.h
+++ b/xen/include/public/io/blkif.h
@@ -175,7 +175,7 @@
  *
  *------------------------- Backend Device Properties -------------------------
  *
- * discard-aligment
+ * discard-alignment
  *      Values:         <uint32_t>
  *      Default Value:  0
  *      Notes:          4, 5
@@ -194,6 +194,7 @@
  * discard-secure
  *      Values:         0/1 (boolean)
  *      Default Value:  0
+ *      Notes:          10
  *
  *      A value of "1" indicates that the backend can process BLKIF_OP_DISCARD
  *      requests with the BLKIF_DISCARD_SECURE flag set.
@@ -323,9 +324,14 @@
  *     For full interoperability, block front and backends should publish
  *     identical ring parameters, adjusted for unit differences, to the
  *     XenStore nodes used in both schemes.
- * (4) Devices that support discard functionality may internally allocate
- *     space (discardable extents) in units that are larger than the
- *     exported logical block size.
+ * (4) Devices that support discard functionality may internally allocate space
+ *     (discardable extents) in units that are larger than the exported logical
+ *     block size. If the backing device has such discardable extents the
+ *     backend must provide both discard-granularity and discard-alignment.
+ *     Backends supporting discard should include discard-granularity and
+ *     discard-alignment even if it supports discarding individual sectors.
+ *     Frontends should assume discard-alignment == 0 and discard-granularity ==
+ *     sector size if these keys are missing.
  * (5) The discard-alignment parameter allows a physical device to be
  *     partitioned into virtual devices that do not necessarily begin or
  *     end on a discardable extent boundary.
@@ -344,6 +350,8 @@
  *     grants that can be persistently mapped in the frontend driver, but
  *     due to the frontent driver implementation it should never be bigger
  *     than RING_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST.
+ *(10) The discard-secure property may be present and will be set to 1 if the
+ *     backing device supports secure discard.
  */
 
 /*

Re: [PATCH] blkif.h: enhance comments related to the discard feature

[Jan Beulich]

>>> On 14.01.14 at 22:57, Olaf Hering <olaf@aepfle.de> wrote:
> Also fix the name of the discard-alignment property, add the missing 'n'.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Konrad,

you have been working on the discard stuff quite a bit iirc - any
chance you could take a look and send and ack/review?

Jan

> ---
>  xen/include/public/io/blkif.h | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 84eb7fd..515ea90 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -175,7 +175,7 @@
>   *
>   *------------------------- Backend Device Properties -------------------------
>   *
> - * discard-aligment
> + * discard-alignment
>   *      Values:         <uint32_t>
>   *      Default Value:  0
>   *      Notes:          4, 5
> @@ -194,6 +194,7 @@
>   * discard-secure
>   *      Values:         0/1 (boolean)
>   *      Default Value:  0
> + *      Notes:          10
>   *
>   *      A value of "1" indicates that the backend can process 
> BLKIF_OP_DISCARD
>   *      requests with the BLKIF_DISCARD_SECURE flag set.
> @@ -323,9 +324,14 @@
>   *     For full interoperability, block front and backends should publish
>   *     identical ring parameters, adjusted for unit differences, to the
>   *     XenStore nodes used in both schemes.
> - * (4) Devices that support discard functionality may internally allocate
> - *     space (discardable extents) in units that are larger than the
> - *     exported logical block size.
> + * (4) Devices that support discard functionality may internally allocate 
> space
> + *     (discardable extents) in units that are larger than the exported 
> logical
> + *     block size. If the backing device has such discardable extents the
> + *     backend must provide both discard-granularity and discard-alignment.
> + *     Backends supporting discard should include discard-granularity and
> + *     discard-alignment even if it supports discarding individual sectors.
> + *     Frontends should assume discard-alignment == 0 and discard-granularity 
> ==
> + *     sector size if these keys are missing.
>   * (5) The discard-alignment parameter allows a physical device to be
>   *     partitioned into virtual devices that do not necessarily begin or
>   *     end on a discardable extent boundary.
> @@ -344,6 +350,8 @@
>   *     grants that can be persistently mapped in the frontend driver, but
>   *     due to the frontent driver implementation it should never be bigger
>   *     than RING_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST.
> + *(10) The discard-secure property may be present and will be set to 1 if 
> the
> + *     backing device supports secure discard.
>   */
>  
>  /*
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH] blkif.h: enhance comments related to the discard feature

[Konrad Rzeszutek Wilk]

Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 14.01.14 at 22:57, Olaf Hering <olaf@aepfle.de> wrote:
>> Also fix the name of the discard-alignment property, add the missing
>'n'.
>> 
>> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>
>Konrad,
>
>you have been working on the discard stuff quite a bit iirc - any
>chance you could take a look and send and ack/review?

Sure - next week I will dig through the patches.
>
>Jan
>
>> ---
>>  xen/include/public/io/blkif.h | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>> 
>> diff --git a/xen/include/public/io/blkif.h
>b/xen/include/public/io/blkif.h
>> index 84eb7fd..515ea90 100644
>> --- a/xen/include/public/io/blkif.h
>> +++ b/xen/include/public/io/blkif.h
>> @@ -175,7 +175,7 @@
>>   *
>>   *------------------------- Backend Device Properties
>-------------------------
>>   *
>> - * discard-aligment
>> + * discard-alignment
>>   *      Values:         <uint32_t>
>>   *      Default Value:  0
>>   *      Notes:          4, 5
>> @@ -194,6 +194,7 @@
>>   * discard-secure
>>   *      Values:         0/1 (boolean)
>>   *      Default Value:  0
>> + *      Notes:          10
>>   *
>>   *      A value of "1" indicates that the backend can process 
>> BLKIF_OP_DISCARD
>>   *      requests with the BLKIF_DISCARD_SECURE flag set.
>> @@ -323,9 +324,14 @@
>>   *     For full interoperability, block front and backends should
>publish
>>   *     identical ring parameters, adjusted for unit differences, to
>the
>>   *     XenStore nodes used in both schemes.
>> - * (4) Devices that support discard functionality may internally
>allocate
>> - *     space (discardable extents) in units that are larger than the
>> - *     exported logical block size.
>> + * (4) Devices that support discard functionality may internally
>allocate 
>> space
>> + *     (discardable extents) in units that are larger than the
>exported 
>> logical
>> + *     block size. If the backing device has such discardable
>extents the
>> + *     backend must provide both discard-granularity and
>discard-alignment.
>> + *     Backends supporting discard should include
>discard-granularity and
>> + *     discard-alignment even if it supports discarding individual
>sectors.
>> + *     Frontends should assume discard-alignment == 0 and
>discard-granularity 
>> ==
>> + *     sector size if these keys are missing.
>>   * (5) The discard-alignment parameter allows a physical device to
>be
>>   *     partitioned into virtual devices that do not necessarily
>begin or
>>   *     end on a discardable extent boundary.
>> @@ -344,6 +350,8 @@
>>   *     grants that can be persistently mapped in the frontend
>driver, but
>>   *     due to the frontent driver implementation it should never be
>bigger
>>   *     than RING_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST.
>> + *(10) The discard-secure property may be present and will be set to
>1 if 
>> the
>> + *     backing device supports secure discard.
>>   */
>>  
>>  /*
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org 
>> http://lists.xen.org/xen-devel 

Re: [PATCH] blkif.h: enhance comments related to the discard feature

[Konrad Rzeszutek Wilk]

On Fri, Jan 17, 2014 at 03:09:13PM +0000, Jan Beulich wrote:
> >>> On 14.01.14 at 22:57, Olaf Hering <olaf@aepfle.de> wrote:
> > Also fix the name of the discard-alignment property, add the missing 'n'.
> > 
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> Konrad,
> 
> you have been working on the discard stuff quite a bit iirc - any
> chance you could take a look and send and ack/review?
> 
> Jan
> 
> > ---
> >  xen/include/public/io/blkif.h | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> > index 84eb7fd..515ea90 100644
> > --- a/xen/include/public/io/blkif.h
> > +++ b/xen/include/public/io/blkif.h
> > @@ -175,7 +175,7 @@
> >   *
> >   *------------------------- Backend Device Properties -------------------------
> >   *
> > - * discard-aligment
> > + * discard-alignment
> >   *      Values:         <uint32_t>
> >   *      Default Value:  0
> >   *      Notes:          4, 5
> > @@ -194,6 +194,7 @@
> >   * discard-secure
> >   *      Values:         0/1 (boolean)
> >   *      Default Value:  0
> > + *      Notes:          10
> >   *
> >   *      A value of "1" indicates that the backend can process 
> > BLKIF_OP_DISCARD
> >   *      requests with the BLKIF_DISCARD_SECURE flag set.
> > @@ -323,9 +324,14 @@
> >   *     For full interoperability, block front and backends should publish
> >   *     identical ring parameters, adjusted for unit differences, to the
> >   *     XenStore nodes used in both schemes.
> > - * (4) Devices that support discard functionality may internally allocate
> > - *     space (discardable extents) in units that are larger than the
> > - *     exported logical block size.
> > + * (4) Devices that support discard functionality may internally allocate 
> > space
> > + *     (discardable extents) in units that are larger than the exported 
> > logical
> > + *     block size. If the backing device has such discardable extents the
> > + *     backend must provide both discard-granularity and discard-alignment.
                    ^^^^ - MAY

> > + *     Backends supporting discard should include discard-granularity and
                                        ^^^^^ - MAY
> > + *     discard-alignment even if it supports discarding individual sectors.
> > + *     Frontends should assume discard-alignment == 0 and discard-granularity 
> > ==
> > + *     sector size if these keys are missing.
> >   * (5) The discard-alignment parameter allows a physical device to be
> >   *     partitioned into virtual devices that do not necessarily begin or
> >   *     end on a discardable extent boundary.
> > @@ -344,6 +350,8 @@
> >   *     grants that can be persistently mapped in the frontend driver, but
> >   *     due to the frontent driver implementation it should never be bigger
> >   *     than RING_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST.
> > + *(10) The discard-secure property may be present and will be set to 1 if 
> > the
> > + *     backing device supports secure discard.
> >   */
> >  
> >  /*
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org 
> > http://lists.xen.org/xen-devel 
> 
> 
> 

Re: [PATCH] blkif.h: enhance comments related to the discard feature

[Jan Beulich]

>>> On 22.01.14 at 22:14, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Fri, Jan 17, 2014 at 03:09:13PM +0000, Jan Beulich wrote:
>> >>> On 14.01.14 at 22:57, Olaf Hering <olaf@aepfle.de> wrote:
>> > @@ -323,9 +324,14 @@
>> >   *     For full interoperability, block front and backends should publish
>> >   *     identical ring parameters, adjusted for unit differences, to the
>> >   *     XenStore nodes used in both schemes.
>> > - * (4) Devices that support discard functionality may internally allocate
>> > - *     space (discardable extents) in units that are larger than the
>> > - *     exported logical block size.
>> > + * (4) Devices that support discard functionality may internally allocate 
>> > space
>> > + *     (discardable extents) in units that are larger than the exported 
>> > logical
>> > + *     block size. If the backing device has such discardable extents the
>> > + *     backend must provide both discard-granularity and discard-alignment.
>                     ^^^^ - MAY

I think the intention is to say that these two should go together,
i.e. specifying just one of them is a mistake.

Jan

>> > + *     Backends supporting discard should include discard-granularity and
>                                         ^^^^^ - MAY
>> > + *     discard-alignment even if it supports discarding individual sectors.
>> > + *     Frontends should assume discard-alignment == 0 and discard-granularity 
>> > ==
>> > + *     sector size if these keys are missing.
>> >   * (5) The discard-alignment parameter allows a physical device to be
>> >   *     partitioned into virtual devices that do not necessarily begin or
>> >   *     end on a discardable extent boundary.

Re: [PATCH] blkif.h: enhance comments related to the discard feature

[Konrad Rzeszutek Wilk]

On Thu, Jan 23, 2014 at 08:19:40AM +0000, Jan Beulich wrote:
> >>> On 22.01.14 at 22:14, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > On Fri, Jan 17, 2014 at 03:09:13PM +0000, Jan Beulich wrote:
> >> >>> On 14.01.14 at 22:57, Olaf Hering <olaf@aepfle.de> wrote:
> >> > @@ -323,9 +324,14 @@
> >> >   *     For full interoperability, block front and backends should publish
> >> >   *     identical ring parameters, adjusted for unit differences, to the
> >> >   *     XenStore nodes used in both schemes.
> >> > - * (4) Devices that support discard functionality may internally allocate
> >> > - *     space (discardable extents) in units that are larger than the
> >> > - *     exported logical block size.
> >> > + * (4) Devices that support discard functionality may internally allocate 
> >> > space
> >> > + *     (discardable extents) in units that are larger than the exported 
> >> > logical
> >> > + *     block size. If the backing device has such discardable extents the
> >> > + *     backend must provide both discard-granularity and discard-alignment.
> >                     ^^^^ - MAY
> 
> I think the intention is to say that these two should go together,
> i.e. specifying just one of them is a mistake.

The 'and' in that sentence covers that I think?

My reading with 'must' is that 'features-discard' MUST have both
discard-granularity and discard-alignment. But that is not the case
- even if the device does support them - it does not have to
expose them.

> 
> Jan
> 
> >> > + *     Backends supporting discard should include discard-granularity and
> >                                         ^^^^^ - MAY
> >> > + *     discard-alignment even if it supports discarding individual sectors.
> >> > + *     Frontends should assume discard-alignment == 0 and discard-granularity 
> >> > ==
> >> > + *     sector size if these keys are missing.
> >> >   * (5) The discard-alignment parameter allows a physical device to be
> >> >   *     partitioned into virtual devices that do not necessarily begin or
> >> >   *     end on a discardable extent boundary.
> 
> 

Re: [PATCH] blkif.h: enhance comments related to the discard feature

[Jan Beulich]

>>> On 24.01.14 at 15:30, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Thu, Jan 23, 2014 at 08:19:40AM +0000, Jan Beulich wrote:
>> >>> On 22.01.14 at 22:14, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> > On Fri, Jan 17, 2014 at 03:09:13PM +0000, Jan Beulich wrote:
>> >> >>> On 14.01.14 at 22:57, Olaf Hering <olaf@aepfle.de> wrote:
>> >> > @@ -323,9 +324,14 @@
>> >> >   *     For full interoperability, block front and backends should publish
>> >> >   *     identical ring parameters, adjusted for unit differences, to the
>> >> >   *     XenStore nodes used in both schemes.
>> >> > - * (4) Devices that support discard functionality may internally allocate
>> >> > - *     space (discardable extents) in units that are larger than the
>> >> > - *     exported logical block size.
>> >> > + * (4) Devices that support discard functionality may internally allocate 
> 
>> >> > space
>> >> > + *     (discardable extents) in units that are larger than the exported 
>> >> > logical
>> >> > + *     block size. If the backing device has such discardable extents the
>> >> > + *     backend must provide both discard-granularity and discard-alignment.
>> >                     ^^^^ - MAY
>> 
>> I think the intention is to say that these two should go together,
>> i.e. specifying just one of them is a mistake.
> 
> The 'and' in that sentence covers that I think?

Not with my reading of it - when using "must", it's clear that both
need to be present. When using "may", one can read it that
providing just one is fine too. But I agree that the wording with
"must" isn't fully correct either. I'd go for "should", and extend the
sentence by "; providing just one of the two may be considered an
error by the frontend".

Jan

> My reading with 'must' is that 'features-discard' MUST have both
> discard-granularity and discard-alignment. But that is not the case
> - even if the device does support them - it does not have to
> expose them.
> 
>> 
>> Jan
>> 
>> >> > + *     Backends supporting discard should include discard-granularity and
>> >                                         ^^^^^ - MAY
>> >> > + *     discard-alignment even if it supports discarding individual 
> sectors.
>> >> > + *     Frontends should assume discard-alignment == 0 and 
> discard-granularity 
>> >> > ==
>> >> > + *     sector size if these keys are missing.
>> >> >   * (5) The discard-alignment parameter allows a physical device to be
>> >> >   *     partitioned into virtual devices that do not necessarily begin or
>> >> >   *     end on a discardable extent boundary.
>> 
>> 

Re: [PATCH] blkif.h: enhance comments related to the discard feature

[Olaf Hering]

On Fri, Jan 24, Jan Beulich wrote:

> Not with my reading of it - when using "must", it's clear that both
> need to be present. When using "may", one can read it that
> providing just one is fine too. But I agree that the wording with
> "must" isn't fully correct either. I'd go for "should", and extend the
> sentence by "; providing just one of the two may be considered an
> error by the frontend".

I will make that change.

Olaf

Re: [PATCH] blkif.h: enhance comments related to the discard feature

[Keir Fraser]

On 14/01/2014 21:57, "Olaf Hering" <olaf@aepfle.de> wrote:

> Also fix the name of the discard-alignment property, add the missing 'n'.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Acked-by: Keir Fraser <keir@xen.org>

Although I am not an authority on this interface...

> ---
>  xen/include/public/io/blkif.h | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 84eb7fd..515ea90 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -175,7 +175,7 @@
>   *
>   *------------------------- Backend Device Properties
> -------------------------
>   *
> - * discard-aligment
> + * discard-alignment
>   *      Values:         <uint32_t>
>   *      Default Value:  0
>   *      Notes:          4, 5
> @@ -194,6 +194,7 @@
>   * discard-secure
>   *      Values:         0/1 (boolean)
>   *      Default Value:  0
> + *      Notes:          10
>   *
>   *      A value of "1" indicates that the backend can process
> BLKIF_OP_DISCARD
>   *      requests with the BLKIF_DISCARD_SECURE flag set.
> @@ -323,9 +324,14 @@
>   *     For full interoperability, block front and backends should publish
>   *     identical ring parameters, adjusted for unit differences, to the
>   *     XenStore nodes used in both schemes.
> - * (4) Devices that support discard functionality may internally allocate
> - *     space (discardable extents) in units that are larger than the
> - *     exported logical block size.
> + * (4) Devices that support discard functionality may internally allocate
> space
> + *     (discardable extents) in units that are larger than the exported
> logical
> + *     block size. If the backing device has such discardable extents the
> + *     backend must provide both discard-granularity and discard-alignment.
> + *     Backends supporting discard should include discard-granularity and
> + *     discard-alignment even if it supports discarding individual sectors.
> + *     Frontends should assume discard-alignment == 0 and discard-granularity
> ==
> + *     sector size if these keys are missing.
>   * (5) The discard-alignment parameter allows a physical device to be
>   *     partitioned into virtual devices that do not necessarily begin or
>   *     end on a discardable extent boundary.
> @@ -344,6 +350,8 @@
>   *     grants that can be persistently mapped in the frontend driver, but
>   *     due to the frontent driver implementation it should never be bigger
>   *     than RING_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST.
> + *(10) The discard-secure property may be present and will be set to 1 if the
> + *     backing device supports secure discard.
>   */
>  
>  /*