[PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[Olaf Hering]

In its initial implementation a check for "type" was added, but only phy
and file are handled. This breaks advertised discard support for other
type values such as qdisk.

Fix and simplify this function: remove the check for "type" as the
frontend is not supposed to care about this backend detail. Expect
backends to provide discard-aligment, discard-granularity and
discard-secure properties because interface specification in blkif.h
does not list these properties as optional.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 drivers/block/xen-blkfront.c | 33 +++++++++------------------------
 1 file changed, 9 insertions(+), 24 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index c4a4c90..6ef63eb 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1634,37 +1634,22 @@ blkfront_closing(struct blkfront_info *info)
 
 static void blkfront_setup_discard(struct blkfront_info *info)
 {
-	int err;
-	char *type;
 	unsigned int discard_granularity;
 	unsigned int discard_alignment;
 	unsigned int discard_secure;
 
-	type = xenbus_read(XBT_NIL, info->xbdev->otherend, "type", NULL);
-	if (IS_ERR(type))
+	if (xenbus_gather(XBT_NIL, info->xbdev->otherend,
+		"discard-granularity", "%u", &discard_granularity,
+		"discard-alignment", "%u", &discard_alignment,
+		"discard-secure", "%u", &discard_secure,
+		NULL))
 		return;
 
-	info->feature_secdiscard = 0;
-	if (strncmp(type, "phy", 3) == 0) {
-		err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
-			"discard-granularity", "%u", &discard_granularity,
-			"discard-alignment", "%u", &discard_alignment,
-			NULL);
-		if (!err) {
-			info->feature_discard = 1;
-			info->discard_granularity = discard_granularity;
-			info->discard_alignment = discard_alignment;
-		}
-		err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
-			    "discard-secure", "%d", &discard_secure,
-			    NULL);
-		if (!err)
-			info->feature_secdiscard = discard_secure;
-
-	} else if (strncmp(type, "file", 4) == 0)
-		info->feature_discard = 1;
+	info->discard_granularity = discard_granularity;
+	info->discard_alignment = discard_alignment;
+	info->feature_secdiscard = !!discard_secure;
 
-	kfree(type);
+	info->feature_discard = 1;
 }
 
 static int blkfront_setup_indirect(struct blkfront_info *info)

Re: [Xen-devel] [PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[Jan Beulich]

>>> On 13.01.14 at 11:14, Olaf Hering <olaf@aepfle.de> wrote:
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1634,37 +1634,22 @@ blkfront_closing(struct blkfront_info *info)
>  
>  static void blkfront_setup_discard(struct blkfront_info *info)
>  {
> -	int err;
> -	char *type;
>  	unsigned int discard_granularity;
>  	unsigned int discard_alignment;
>  	unsigned int discard_secure;
>  
> -	type = xenbus_read(XBT_NIL, info->xbdev->otherend, "type", NULL);
> -	if (IS_ERR(type))
> +	if (xenbus_gather(XBT_NIL, info->xbdev->otherend,
> +		"discard-granularity", "%u", &discard_granularity,
> +		"discard-alignment", "%u", &discard_alignment,
> +		"discard-secure", "%u", &discard_secure,
> +		NULL))
>  		return;

You can't do this in one go - the first two and the last one may be
set independently (and are independent in their meaning), and
hence need to be queried independently (xenbus_gather() fails
on the first absent value).

Jan

Re: [Xen-devel] [PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[Olaf Hering]

On Mon, Jan 13, Jan Beulich wrote:

> You can't do this in one go - the first two and the last one may be
> set independently (and are independent in their meaning), and
> hence need to be queried independently (xenbus_gather() fails
> on the first absent value).

Yes, thats the purpose. Since the properties are required its an all or
nothing thing. If they are truly optional then blkif.h should be updated
to say that.

Olaf

Re: [Xen-devel] [PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[Jan Beulich]

>>> On 13.01.14 at 13:01, Olaf Hering <olaf@aepfle.de> wrote:
> On Mon, Jan 13, Jan Beulich wrote:
> 
>> You can't do this in one go - the first two and the last one may be
>> set independently (and are independent in their meaning), and
>> hence need to be queried independently (xenbus_gather() fails
>> on the first absent value).
> 
> Yes, thats the purpose. Since the properties are required its an all or
> nothing thing. If they are truly optional then blkif.h should be updated
> to say that.

They _are_ optional.

Jan

Re: [Xen-devel] [PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[Olaf Hering]

On Mon, Jan 13, Jan Beulich wrote:

> >>> On 13.01.14 at 13:01, Olaf Hering <olaf@aepfle.de> wrote:
> > On Mon, Jan 13, Jan Beulich wrote:
> > 
> >> You can't do this in one go - the first two and the last one may be
> >> set independently (and are independent in their meaning), and
> >> hence need to be queried independently (xenbus_gather() fails
> >> on the first absent value).
> > 
> > Yes, thats the purpose. Since the properties are required its an all or
> > nothing thing. If they are truly optional then blkif.h should be updated
> > to say that.
> 
> They _are_ optional.

In this case my first patch is correct and should be used.

Olaf

Re: [Xen-devel] [PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[Ian Campbell]

On Mon, 2014-01-13 at 12:34 +0000, Jan Beulich wrote:
> >>> On 13.01.14 at 13:01, Olaf Hering <olaf@aepfle.de> wrote:
> > On Mon, Jan 13, Jan Beulich wrote:
> > 
> >> You can't do this in one go - the first two and the last one may be
> >> set independently (and are independent in their meaning), and
> >> hence need to be queried independently (xenbus_gather() fails
> >> on the first absent value).
> > 
> > Yes, thats the purpose. Since the properties are required its an all or
> > nothing thing. If they are truly optional then blkif.h should be updated
> > to say that.
> 
> They _are_ optional.

But is it true that either they are all present or they are all absent?

Ian.

Re: [Xen-devel] [PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[Jan Beulich]

>>> On 13.01.14 at 14:00, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-01-13 at 12:34 +0000, Jan Beulich wrote:
>> >>> On 13.01.14 at 13:01, Olaf Hering <olaf@aepfle.de> wrote:
>> > On Mon, Jan 13, Jan Beulich wrote:
>> > 
>> >> You can't do this in one go - the first two and the last one may be
>> >> set independently (and are independent in their meaning), and
>> >> hence need to be queried independently (xenbus_gather() fails
>> >> on the first absent value).
>> > 
>> > Yes, thats the purpose. Since the properties are required its an all or
>> > nothing thing. If they are truly optional then blkif.h should be updated
>> > to say that.
>> 
>> They _are_ optional.
> 
> But is it true that either they are all present or they are all absent?

No, it's not. discard-secure is independent of the other two (but
those other two are tied together).

Jan

Re: [Xen-devel] [PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[Ian Campbell]

On Mon, 2014-01-13 at 13:16 +0000, Jan Beulich wrote:
> >>> On 13.01.14 at 14:00, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-01-13 at 12:34 +0000, Jan Beulich wrote:
> >> >>> On 13.01.14 at 13:01, Olaf Hering <olaf@aepfle.de> wrote:
> >> > On Mon, Jan 13, Jan Beulich wrote:
> >> > 
> >> >> You can't do this in one go - the first two and the last one may be
> >> >> set independently (and are independent in their meaning), and
> >> >> hence need to be queried independently (xenbus_gather() fails
> >> >> on the first absent value).
> >> > 
> >> > Yes, thats the purpose. Since the properties are required its an all or
> >> > nothing thing. If they are truly optional then blkif.h should be updated
> >> > to say that.
> >> 
> >> They _are_ optional.
> > 
> > But is it true that either they are all present or they are all absent?
> 
> No, it's not. discard-secure is independent of the other two (but
> those other two are tied together).

Thanks for clarifying.

Re: [Xen-devel] [PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[David Vrabel]

On 13/01/14 13:16, Jan Beulich wrote:
>>>> On 13.01.14 at 14:00, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> On Mon, 2014-01-13 at 12:34 +0000, Jan Beulich wrote:
>>>>>> On 13.01.14 at 13:01, Olaf Hering <olaf@aepfle.de> wrote:
>>>> On Mon, Jan 13, Jan Beulich wrote:
>>>>
>>>>> You can't do this in one go - the first two and the last one may be
>>>>> set independently (and are independent in their meaning), and
>>>>> hence need to be queried independently (xenbus_gather() fails
>>>>> on the first absent value).
>>>>
>>>> Yes, thats the purpose. Since the properties are required its an all or
>>>> nothing thing. If they are truly optional then blkif.h should be updated
>>>> to say that.
>>>
>>> They _are_ optional.
>>
>> But is it true that either they are all present or they are all absent?
> 
> No, it's not. discard-secure is independent of the other two (but
> those other two are tied together).

Can we have a patch to blkif.h that clarifies this?

e.g.,

feature-discard

   ...

   discard-granularity and discard-offset must also be present if
   feature-discard is enabled

   discard-secure may also be present if feature-discard is enabled.

David

Re: [Xen-devel] [PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[Jan Beulich]

>>> On 13.01.14 at 14:45, David Vrabel <david.vrabel@citrix.com> wrote:
> On 13/01/14 13:16, Jan Beulich wrote:
>>>>> On 13.01.14 at 14:00, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>> On Mon, 2014-01-13 at 12:34 +0000, Jan Beulich wrote:
>>>>>>> On 13.01.14 at 13:01, Olaf Hering <olaf@aepfle.de> wrote:
>>>>> On Mon, Jan 13, Jan Beulich wrote:
>>>>>
>>>>>> You can't do this in one go - the first two and the last one may be
>>>>>> set independently (and are independent in their meaning), and
>>>>>> hence need to be queried independently (xenbus_gather() fails
>>>>>> on the first absent value).
>>>>>
>>>>> Yes, thats the purpose. Since the properties are required its an all or
>>>>> nothing thing. If they are truly optional then blkif.h should be updated
>>>>> to say that.
>>>>
>>>> They _are_ optional.
>>>
>>> But is it true that either they are all present or they are all absent?
>> 
>> No, it's not. discard-secure is independent of the other two (but
>> those other two are tied together).
> 
> Can we have a patch to blkif.h that clarifies this?
> 
> e.g.,
> 
> feature-discard
> 
>    ...
> 
>    discard-granularity and discard-offset must also be present if
>    feature-discard is enabled

It would be "may" here too afaict. But I'll defer to Konrad, who
has done more work in this area...

Jan

>    discard-secure may also be present if feature-discard is enabled.
> 
> David

Re: [Xen-devel] [PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[Konrad Rzeszutek Wilk]

Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 13.01.14 at 14:45, David Vrabel <david.vrabel@citrix.com> wrote:
>> On 13/01/14 13:16, Jan Beulich wrote:
>>>>>> On 13.01.14 at 14:00, Ian Campbell <Ian.Campbell@citrix.com>
>wrote:
>>>> On Mon, 2014-01-13 at 12:34 +0000, Jan Beulich wrote:
>>>>>>>> On 13.01.14 at 13:01, Olaf Hering <olaf@aepfle.de> wrote:
>>>>>> On Mon, Jan 13, Jan Beulich wrote:
>>>>>>
>>>>>>> You can't do this in one go - the first two and the last one may
>be
>>>>>>> set independently (and are independent in their meaning), and
>>>>>>> hence need to be queried independently (xenbus_gather() fails
>>>>>>> on the first absent value).
>>>>>>
>>>>>> Yes, thats the purpose. Since the properties are required its an
>all or
>>>>>> nothing thing. If they are truly optional then blkif.h should be
>updated
>>>>>> to say that.
>>>>>
>>>>> They _are_ optional.
>>>>
>>>> But is it true that either they are all present or they are all
>absent?
>>> 
>>> No, it's not. discard-secure is independent of the other two (but
>>> those other two are tied together).
>> 
>> Can we have a patch to blkif.h that clarifies this?
>> 
>> e.g.,
>> 
>> feature-discard
>> 
>>    ...
>> 
>>    discard-granularity and discard-offset must also be present if
>>    feature-discard is enabled
>
>It would be "may" here too afaict. But I'll defer to Konrad, who
>has done more work in this area...
>
>Jan
>
>>    discard-secure may also be present if feature-discard is enabled.
>> 
>> David
>
>
>
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xen.org
>http://lists.xen.org/xen-devel

It is all 'may'. If there is just 'feature-discard' without any other options that is OK.

Re: [Xen-devel] [PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[Olaf Hering]

On Mon, Jan 13, David Vrabel wrote:

> Can we have a patch to blkif.h that clarifies this?

What about this change?

diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
index 84eb7fd..56e2faa 100644
--- a/xen/include/public/io/blkif.h
+++ b/xen/include/public/io/blkif.h
@@ -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,10 @@
  *     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. The properties discard-granularity and discard-alignment may
+ *     be present if the backing device has such requirments.
  * (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 +346,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.
  */
 
 /*


Olaf

Re: [Xen-devel] [PATCH v2] xen-blkfront: remove type check from blkfront_setup_discard

[David Vrabel]

On 14/01/14 14:53, Olaf Hering wrote:
> On Mon, Jan 13, David Vrabel wrote:
> 
>> Can we have a patch to blkif.h that clarifies this?
> 
> What about this change?
> 
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 84eb7fd..56e2faa 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -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,10 @@
>   *     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. The properties discard-granularity and discard-alignment may
> + *     be present if the backing device has such requirments.

Clarify that both discard-granularity and discard-alignment must be
present if non-sector-sized granularity is required. e.g.,

"If the backing device has such discardable extents the backend must
provide both discard-granularity and discard-alignment."

You find it useful to add these recommendations:

"Backends supporting discard should include discard-granularity and
discard-alignment even if it supports discarding individual sectors.
Frontends should assume discard-aligment == 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 +346,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.
>   */

David