Re: [PATCH] xen-blkfront: don't make discard-alignment mandatory
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
> > > > Signed-off-by: Roger Pau Monné
> > > > ---
> > > > Cc: Boris Ostrovsky
> > > > Cc: Juergen Gross
> > > > Cc: Stefano Stabellini
> > > > Cc: Konrad Rzeszutek Wilk
> > > > Cc: "Roger Pau Monné"
> > > > Cc: Jens Axboe
> > > > Cc: xen-de...@lists.xenproject.org
> > > > Cc: linux-bl...@vger.kernel.org
> > > > Cc: Arthur Borsboom
> > > > ---
> > > >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.
Re: [PATCH] xen-blkfront: don't make discard-alignment mandatory
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
Signed-off-by: Roger Pau Monné
---
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Stefano Stabellini
Cc: Konrad Rzeszutek Wilk
Cc: "Roger Pau Monné"
Cc: Jens Axboe
Cc: xen-de...@lists.xenproject.org
Cc: linux-bl...@vger.kernel.org
Cc: Arthur Borsboom
---
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
OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH] xen-blkfront: don't make discard-alignment mandatory
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
> > Signed-off-by: Roger Pau Monné
> > ---
> > Cc: Boris Ostrovsky
> > Cc: Juergen Gross
> > Cc: Stefano Stabellini
> > Cc: Konrad Rzeszutek Wilk
> > Cc: "Roger Pau Monné"
> > Cc: Jens Axboe
> > Cc: xen-de...@lists.xenproject.org
> > Cc: linux-bl...@vger.kernel.org
> > Cc: Arthur Borsboom
> > ---
> > 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.
Re: [PATCH] xen-blkfront: don't make discard-alignment mandatory
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
Signed-off-by: Roger Pau Monné
---
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Stefano Stabellini
Cc: Konrad Rzeszutek Wilk
Cc: "Roger Pau Monné"
Cc: Jens Axboe
Cc: xen-de...@lists.xenproject.org
Cc: linux-bl...@vger.kernel.org
Cc: Arthur Borsboom
---
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
OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys
OpenPGP_signature
Description: OpenPGP digital signature
[PATCH] xen-blkfront: don't make discard-alignment mandatory
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
Signed-off-by: Roger Pau Monné
---
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Stefano Stabellini
Cc: Konrad Rzeszutek Wilk
Cc: "Roger Pau Monné"
Cc: Jens Axboe
Cc: xen-de...@lists.xenproject.org
Cc: linux-bl...@vger.kernel.org
Cc: Arthur Borsboom
---
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
