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 <[email protected]>
---
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)
>>> On 13.01.14 at 11:14, Olaf Hering <[email protected]> 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
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
>>> On 13.01.14 at 13:01, Olaf Hering <[email protected]> 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
On Mon, Jan 13, Jan Beulich wrote:
> >>> On 13.01.14 at 13:01, Olaf Hering <[email protected]> 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
On Mon, 2014-01-13 at 12:34 +0000, Jan Beulich wrote:
> >>> On 13.01.14 at 13:01, Olaf Hering <[email protected]> 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.
>>> On 13.01.14 at 14:00, Ian Campbell <[email protected]> wrote:
> On Mon, 2014-01-13 at 12:34 +0000, Jan Beulich wrote:
>> >>> On 13.01.14 at 13:01, Olaf Hering <[email protected]> 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
On Mon, 2014-01-13 at 13:16 +0000, Jan Beulich wrote:
> >>> On 13.01.14 at 14:00, Ian Campbell <[email protected]> wrote:
> > On Mon, 2014-01-13 at 12:34 +0000, Jan Beulich wrote:
> >> >>> On 13.01.14 at 13:01, Olaf Hering <[email protected]> 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.
On 13/01/14 13:16, Jan Beulich wrote:
>>>> On 13.01.14 at 14:00, Ian Campbell <[email protected]> wrote:
>> On Mon, 2014-01-13 at 12:34 +0000, Jan Beulich wrote:
>>>>>> On 13.01.14 at 13:01, Olaf Hering <[email protected]> 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
>>> On 13.01.14 at 14:45, David Vrabel <[email protected]> wrote:
> On 13/01/14 13:16, Jan Beulich wrote:
>>>>> On 13.01.14 at 14:00, Ian Campbell <[email protected]> wrote:
>>> On Mon, 2014-01-13 at 12:34 +0000, Jan Beulich wrote:
>>>>>>> On 13.01.14 at 13:01, Olaf Hering <[email protected]> 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
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
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
Jan Beulich <[email protected]> wrote:
>>>> On 13.01.14 at 14:45, David Vrabel <[email protected]> wrote:
>> On 13/01/14 13:16, Jan Beulich wrote:
>>>>>> On 13.01.14 at 14:00, Ian Campbell <[email protected]>
>wrote:
>>>> On Mon, 2014-01-13 at 12:34 +0000, Jan Beulich wrote:
>>>>>>>> On 13.01.14 at 13:01, Olaf Hering <[email protected]> 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
>[email protected]
>http://lists.xen.org/xen-devel
It is all 'may'. If there is just 'feature-discard' without any other options that is OK.