* [PATCH] xen-blkfront: don't make discard-alignment mandatory
@ 2021-01-18 15:15 Roger Pau Monne
2021-01-19 7:43 ` Jürgen Groß
0 siblings, 1 reply; 5+ messages in thread
From: Roger Pau Monne @ 2021-01-18 15:15 UTC (permalink / raw)
To: linux-kernel
Cc: Roger Pau Monne, Arthur Borsboom, Boris Ostrovsky, Juergen Gross,
Stefano Stabellini, Konrad Rzeszutek Wilk, Jens Axboe, xen-devel,
linux-block
Don't require the discard-alignment xenstore node to be present in
order to correctly setup the feature. This can happen with versions of
QEMU that only write the discard-granularity but not the
discard-alignment node.
Assume discard-alignment is 0 if not present. While there also fix the
logic to not enable the discard feature if discard-granularity is not
present.
Reported-by: Arthur Borsboom <arthurborsboom@gmail.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: xen-devel@lists.xenproject.org
Cc: linux-block@vger.kernel.org
Cc: Arthur Borsboom <arthurborsboom@gmail.com>
---
drivers/block/xen-blkfront.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 5265975b3fba..5a93f7cc2939 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -2179,22 +2179,23 @@ static void blkfront_closing(struct blkfront_info *info)
static void blkfront_setup_discard(struct blkfront_info *info)
{
- int err;
- unsigned int discard_granularity;
- unsigned int discard_alignment;
+ unsigned int discard_granularity = 0;
+ unsigned int discard_alignment = 0;
+ unsigned int discard_secure = 0;
- info->feature_discard = 1;
- err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
+ xenbus_gather(XBT_NIL, info->xbdev->otherend,
"discard-granularity", "%u", &discard_granularity,
"discard-alignment", "%u", &discard_alignment,
+ "discard-secure", "%u", &discard_secure,
NULL);
- if (!err) {
- info->discard_granularity = discard_granularity;
- info->discard_alignment = discard_alignment;
- }
- info->feature_secdiscard =
- !!xenbus_read_unsigned(info->xbdev->otherend, "discard-secure",
- 0);
+
+ if (!discard_granularity)
+ return;
+
+ info->feature_discard = 1;
+ info->discard_granularity = discard_granularity;
+ info->discard_alignment = discard_alignment;
+ info->feature_secdiscard = !!discard_secure;
}
static int blkfront_setup_indirect(struct blkfront_ring_info *rinfo)
--
2.29.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xen-blkfront: don't make discard-alignment mandatory
2021-01-18 15:15 [PATCH] xen-blkfront: don't make discard-alignment mandatory Roger Pau Monne
@ 2021-01-19 7:43 ` Jürgen Groß
2021-01-19 10:06 ` Roger Pau Monné
0 siblings, 1 reply; 5+ messages in thread
From: Jürgen Groß @ 2021-01-19 7:43 UTC (permalink / raw)
To: Roger Pau Monne, linux-kernel
Cc: Arthur Borsboom, Boris Ostrovsky, Stefano Stabellini,
Konrad Rzeszutek Wilk, Jens Axboe, xen-devel, linux-block
[-- Attachment #1.1.1: Type: text/plain, Size: 2288 bytes --]
On 18.01.21 16:15, Roger Pau Monne wrote:
> Don't require the discard-alignment xenstore node to be present in
> order to correctly setup the feature. This can happen with versions of
> QEMU that only write the discard-granularity but not the
> discard-alignment node.
>
> Assume discard-alignment is 0 if not present. While there also fix the
> logic to not enable the discard feature if discard-granularity is not
> present.
>
> Reported-by: Arthur Borsboom <arthurborsboom@gmail.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: xen-devel@lists.xenproject.org
> Cc: linux-block@vger.kernel.org
> Cc: Arthur Borsboom <arthurborsboom@gmail.com>
> ---
> drivers/block/xen-blkfront.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 5265975b3fba..5a93f7cc2939 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -2179,22 +2179,23 @@ static void blkfront_closing(struct blkfront_info *info)
>
> static void blkfront_setup_discard(struct blkfront_info *info)
> {
> - int err;
> - unsigned int discard_granularity;
> - unsigned int discard_alignment;
> + unsigned int discard_granularity = 0;
> + unsigned int discard_alignment = 0;
> + unsigned int discard_secure = 0;
>
> - info->feature_discard = 1;
> - err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> + xenbus_gather(XBT_NIL, info->xbdev->otherend,
> "discard-granularity", "%u", &discard_granularity,
> "discard-alignment", "%u", &discard_alignment,
> + "discard-secure", "%u", &discard_secure,
> NULL);
This would mean that "discard-secure" will be evaluated only if the
other two items are set in Xenstore. From blkif.h I can't see this is
required, and your patch is modifying today's behavior in this regard.
You might want to have three xenbus_read_unsigned() calls instead.
Juergen
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen-blkfront: don't make discard-alignment mandatory
2021-01-19 7:43 ` Jürgen Groß
@ 2021-01-19 10:06 ` Roger Pau Monné
2021-01-19 10:11 ` Jürgen Groß
0 siblings, 1 reply; 5+ messages in thread
From: Roger Pau Monné @ 2021-01-19 10:06 UTC (permalink / raw)
To: Jürgen Groß
Cc: linux-kernel, Arthur Borsboom, Boris Ostrovsky,
Stefano Stabellini, Konrad Rzeszutek Wilk, Jens Axboe, xen-devel,
linux-block
On Tue, Jan 19, 2021 at 08:43:01AM +0100, Jürgen Groß wrote:
> On 18.01.21 16:15, Roger Pau Monne wrote:
> > Don't require the discard-alignment xenstore node to be present in
> > order to correctly setup the feature. This can happen with versions of
> > QEMU that only write the discard-granularity but not the
> > discard-alignment node.
> >
> > Assume discard-alignment is 0 if not present. While there also fix the
> > logic to not enable the discard feature if discard-granularity is not
> > present.
> >
> > Reported-by: Arthur Borsboom <arthurborsboom@gmail.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> > Cc: Jens Axboe <axboe@kernel.dk>
> > Cc: xen-devel@lists.xenproject.org
> > Cc: linux-block@vger.kernel.org
> > Cc: Arthur Borsboom <arthurborsboom@gmail.com>
> > ---
> > drivers/block/xen-blkfront.c | 25 +++++++++++++------------
> > 1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > index 5265975b3fba..5a93f7cc2939 100644
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -2179,22 +2179,23 @@ static void blkfront_closing(struct blkfront_info *info)
> > static void blkfront_setup_discard(struct blkfront_info *info)
> > {
> > - int err;
> > - unsigned int discard_granularity;
> > - unsigned int discard_alignment;
> > + unsigned int discard_granularity = 0;
> > + unsigned int discard_alignment = 0;
> > + unsigned int discard_secure = 0;
> > - info->feature_discard = 1;
> > - err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> > + xenbus_gather(XBT_NIL, info->xbdev->otherend,
> > "discard-granularity", "%u", &discard_granularity,
> > "discard-alignment", "%u", &discard_alignment,
> > + "discard-secure", "%u", &discard_secure,
> > NULL);
>
> This would mean that "discard-secure" will be evaluated only if the
> other two items are set in Xenstore. From blkif.h I can't see this is
> required, and your patch is modifying today's behavior in this regard.
>
> You might want to have three xenbus_read_unsigned() calls instead.
You are right, discard-secure should be fetched regardless of whether
discard-alignment exists.
I can fetch discard-granularity and discard-alignment using
xenbus_gather and keep discard-secure using xenbus_read_unsigned. Let
me send a new version.
Thanks, Roger.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen-blkfront: don't make discard-alignment mandatory
2021-01-19 10:06 ` Roger Pau Monné
@ 2021-01-19 10:11 ` Jürgen Groß
2021-01-19 10:21 ` Roger Pau Monné
0 siblings, 1 reply; 5+ messages in thread
From: Jürgen Groß @ 2021-01-19 10:11 UTC (permalink / raw)
To: Roger Pau Monné
Cc: linux-kernel, Arthur Borsboom, Boris Ostrovsky,
Stefano Stabellini, Konrad Rzeszutek Wilk, Jens Axboe, xen-devel,
linux-block
[-- Attachment #1.1.1: Type: text/plain, Size: 2923 bytes --]
On 19.01.21 11:06, Roger Pau Monné wrote:
> On Tue, Jan 19, 2021 at 08:43:01AM +0100, Jürgen Groß wrote:
>> On 18.01.21 16:15, Roger Pau Monne wrote:
>>> Don't require the discard-alignment xenstore node to be present in
>>> order to correctly setup the feature. This can happen with versions of
>>> QEMU that only write the discard-granularity but not the
>>> discard-alignment node.
>>>
>>> Assume discard-alignment is 0 if not present. While there also fix the
>>> logic to not enable the discard feature if discard-granularity is not
>>> present.
>>>
>>> Reported-by: Arthur Borsboom <arthurborsboom@gmail.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Cc: Juergen Gross <jgross@suse.com>
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
>>> Cc: Jens Axboe <axboe@kernel.dk>
>>> Cc: xen-devel@lists.xenproject.org
>>> Cc: linux-block@vger.kernel.org
>>> Cc: Arthur Borsboom <arthurborsboom@gmail.com>
>>> ---
>>> drivers/block/xen-blkfront.c | 25 +++++++++++++------------
>>> 1 file changed, 13 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>> index 5265975b3fba..5a93f7cc2939 100644
>>> --- a/drivers/block/xen-blkfront.c
>>> +++ b/drivers/block/xen-blkfront.c
>>> @@ -2179,22 +2179,23 @@ static void blkfront_closing(struct blkfront_info *info)
>>> static void blkfront_setup_discard(struct blkfront_info *info)
>>> {
>>> - int err;
>>> - unsigned int discard_granularity;
>>> - unsigned int discard_alignment;
>>> + unsigned int discard_granularity = 0;
>>> + unsigned int discard_alignment = 0;
>>> + unsigned int discard_secure = 0;
>>> - info->feature_discard = 1;
>>> - err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
>>> + xenbus_gather(XBT_NIL, info->xbdev->otherend,
>>> "discard-granularity", "%u", &discard_granularity,
>>> "discard-alignment", "%u", &discard_alignment,
>>> + "discard-secure", "%u", &discard_secure,
>>> NULL);
>>
>> This would mean that "discard-secure" will be evaluated only if the
>> other two items are set in Xenstore. From blkif.h I can't see this is
>> required, and your patch is modifying today's behavior in this regard.
>>
>> You might want to have three xenbus_read_unsigned() calls instead.
>
> You are right, discard-secure should be fetched regardless of whether
> discard-alignment exists.
>
> I can fetch discard-granularity and discard-alignment using
> xenbus_gather and keep discard-secure using xenbus_read_unsigned. Let
> me send a new version.
I'm still not convinced this is correct. blkif.h doesn't mention that
discard-alignment will be valid only with discard-granularity being
present.
Juergen
[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen-blkfront: don't make discard-alignment mandatory
2021-01-19 10:11 ` Jürgen Groß
@ 2021-01-19 10:21 ` Roger Pau Monné
0 siblings, 0 replies; 5+ messages in thread
From: Roger Pau Monné @ 2021-01-19 10:21 UTC (permalink / raw)
To: Jürgen Groß
Cc: linux-kernel, Arthur Borsboom, Boris Ostrovsky,
Stefano Stabellini, Konrad Rzeszutek Wilk, Jens Axboe, xen-devel,
linux-block
On Tue, Jan 19, 2021 at 11:11:26AM +0100, Jürgen Groß wrote:
> On 19.01.21 11:06, Roger Pau Monné wrote:
> > On Tue, Jan 19, 2021 at 08:43:01AM +0100, Jürgen Groß wrote:
> > > On 18.01.21 16:15, Roger Pau Monne wrote:
> > > > Don't require the discard-alignment xenstore node to be present in
> > > > order to correctly setup the feature. This can happen with versions of
> > > > QEMU that only write the discard-granularity but not the
> > > > discard-alignment node.
> > > >
> > > > Assume discard-alignment is 0 if not present. While there also fix the
> > > > logic to not enable the discard feature if discard-granularity is not
> > > > present.
> > > >
> > > > Reported-by: Arthur Borsboom <arthurborsboom@gmail.com>
> > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > ---
> > > > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > > Cc: Juergen Gross <jgross@suse.com>
> > > > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > > Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> > > > Cc: Jens Axboe <axboe@kernel.dk>
> > > > Cc: xen-devel@lists.xenproject.org
> > > > Cc: linux-block@vger.kernel.org
> > > > Cc: Arthur Borsboom <arthurborsboom@gmail.com>
> > > > ---
> > > > drivers/block/xen-blkfront.c | 25 +++++++++++++------------
> > > > 1 file changed, 13 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > > > index 5265975b3fba..5a93f7cc2939 100644
> > > > --- a/drivers/block/xen-blkfront.c
> > > > +++ b/drivers/block/xen-blkfront.c
> > > > @@ -2179,22 +2179,23 @@ static void blkfront_closing(struct blkfront_info *info)
> > > > static void blkfront_setup_discard(struct blkfront_info *info)
> > > > {
> > > > - int err;
> > > > - unsigned int discard_granularity;
> > > > - unsigned int discard_alignment;
> > > > + unsigned int discard_granularity = 0;
> > > > + unsigned int discard_alignment = 0;
> > > > + unsigned int discard_secure = 0;
> > > > - info->feature_discard = 1;
> > > > - err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> > > > + xenbus_gather(XBT_NIL, info->xbdev->otherend,
> > > > "discard-granularity", "%u", &discard_granularity,
> > > > "discard-alignment", "%u", &discard_alignment,
> > > > + "discard-secure", "%u", &discard_secure,
> > > > NULL);
> > >
> > > This would mean that "discard-secure" will be evaluated only if the
> > > other two items are set in Xenstore. From blkif.h I can't see this is
> > > required, and your patch is modifying today's behavior in this regard.
> > >
> > > You might want to have three xenbus_read_unsigned() calls instead.
> >
> > You are right, discard-secure should be fetched regardless of whether
> > discard-alignment exists.
> >
> > I can fetch discard-granularity and discard-alignment using
> > xenbus_gather and keep discard-secure using xenbus_read_unsigned. Let
> > me send a new version.
>
> I'm still not convinced this is correct. blkif.h doesn't mention that
> discard-alignment will be valid only with discard-granularity being
> present.
No, in fact I think I need to rework this a little further. Just
having feature-discard = 1 should enable the discard functionality, by
setting discard-granularity = physical block size and
discard-alignment = 0.
Roger.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-19 10:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-18 15:15 [PATCH] xen-blkfront: don't make discard-alignment mandatory Roger Pau Monne
2021-01-19 7:43 ` Jürgen Groß
2021-01-19 10:06 ` Roger Pau Monné
2021-01-19 10:11 ` Jürgen Groß
2021-01-19 10:21 ` Roger Pau Monné
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).