Changes from v1
(https://lore.kernel.org/xen-devel/[email protected]/)
- Fix the wrong feature_persistent caching position of blkfront
- Set blkfront's feature_persistent field setting with simple '&&'
instead of 'if' (Pratyush Yadav)
This patchset fixes misuse of the 'feature-persistent' advertisement
semantic (patches 1 and 2), and the wrong timing of the
'feature_persistent' value caching, which made persistent grants feature
always disabled.
SeongJae Park (3):
xen-blkback: Advertise feature-persistent as user requested
xen-blkfront: Advertise feature-persistent as user requested
xen-blkfront: Cache feature_persistent value before advertisement
drivers/block/xen-blkback/common.h | 3 +++
drivers/block/xen-blkback/xenbus.c | 6 ++++--
drivers/block/xen-blkfront.c | 20 ++++++++++++--------
3 files changed, 19 insertions(+), 10 deletions(-)
--
2.25.1
Xen blkfront advertises its support of the persistent grants feature
when it first setting up and when resuming in 'talk_to_blkback()'.
Then, blkback reads the advertised value when it connects with blkfront
and decides if it will use the persistent grants feature or not, and
advertises its decision to blkfront. Blkfront reads the blkback's
decision and it also makes the decision for the use of the feature.
Commit 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter
when connect"), however, made the blkfront's read of the parameter for
disabling the advertisement, namely 'feature_persistent', to be done
when it negotiate, not when advertise. Therefore blkfront advertises
without reading the parameter. As the field for caching the parameter
value is zero-initialized, it always advertises as the feature is
disabled, so that the persistent grants feature becomes always disabled.
This commit fixes the issue by making the blkfront does parmeter caching
just before the advertisement.
Fixes: 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter when connect")
Cc: <[email protected]> # 5.10.x
Reported-by: Marek Marczykowski-Górecki <[email protected]>
Signed-off-by: SeongJae Park <[email protected]>
---
drivers/block/xen-blkfront.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index dfae08115450..35b9bcad9db9 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1759,6 +1759,12 @@ static int write_per_ring_nodes(struct xenbus_transaction xbt,
return err;
}
+/* Enable the persistent grants feature. */
+static bool feature_persistent = true;
+module_param(feature_persistent, bool, 0644);
+MODULE_PARM_DESC(feature_persistent,
+ "Enables the persistent grants feature");
+
/* Common code used when first setting up, and when resuming. */
static int talk_to_blkback(struct xenbus_device *dev,
struct blkfront_info *info)
@@ -1850,6 +1856,7 @@ static int talk_to_blkback(struct xenbus_device *dev,
message = "writing protocol";
goto abort_transaction;
}
+ info->feature_persistent_parm = feature_persistent;
err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
info->feature_persistent_parm);
if (err)
@@ -1919,12 +1926,6 @@ static int negotiate_mq(struct blkfront_info *info)
return 0;
}
-/* Enable the persistent grants feature. */
-static bool feature_persistent = true;
-module_param(feature_persistent, bool, 0644);
-MODULE_PARM_DESC(feature_persistent,
- "Enables the persistent grants feature");
-
/*
* Entry point to this code when a new device is created. Allocate the basic
* structures and the ring buffer for communication with the backend, and
@@ -2284,7 +2285,6 @@ static void blkfront_gather_backend_features(struct blkfront_info *info)
if (xenbus_read_unsigned(info->xbdev->otherend, "feature-discard", 0))
blkfront_setup_discard(info);
- info->feature_persistent_parm = feature_persistent;
if (info->feature_persistent_parm)
info->feature_persistent =
!!xenbus_read_unsigned(info->xbdev->otherend,
--
2.25.1
On Wed, 31 Aug 2022 16:58:21 +0000 SeongJae Park <[email protected]> wrote:
> Changes from v1
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Fix the wrong feature_persistent caching position of blkfront
> - Set blkfront's feature_persistent field setting with simple '&&'
> instead of 'if' (Pratyush Yadav)
>
> This patchset fixes misuse of the 'feature-persistent' advertisement
> semantic (patches 1 and 2), and the wrong timing of the
> 'feature_persistent' value caching, which made persistent grants feature
> always disabled.
Please note that I have some problem in my test setup and therefore was unable
to fully test this patchset. I am posting this though, as the impact of the
bug is not trivial (always disabling persistent grants), and to make testing of
my proposed fix from others easier. Hope to get someone's test results or code
review of this patchset even before I fix my test setup problem.
Juergen, I didn't add your 'Reviewed-by:'s to the first two patches of this
series because I changed some of the description for making it clear which bug
and commit it is really fixing. Specifically, I wordsmithed the working and
changed 'Fixed:' tag. Code change is almost same, though.
Thanks,
SJ
>
> SeongJae Park (3):
> xen-blkback: Advertise feature-persistent as user requested
> xen-blkfront: Advertise feature-persistent as user requested
> xen-blkfront: Cache feature_persistent value before advertisement
>
> drivers/block/xen-blkback/common.h | 3 +++
> drivers/block/xen-blkback/xenbus.c | 6 ++++--
> drivers/block/xen-blkfront.c | 20 ++++++++++++--------
> 3 files changed, 19 insertions(+), 10 deletions(-)
>
> --
> 2.25.1
>
On 31.08.22 18:58, SeongJae Park wrote:
> Changes from v1
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Fix the wrong feature_persistent caching position of blkfront
> - Set blkfront's feature_persistent field setting with simple '&&'
> instead of 'if' (Pratyush Yadav)
>
> This patchset fixes misuse of the 'feature-persistent' advertisement
> semantic (patches 1 and 2), and the wrong timing of the
> 'feature_persistent' value caching, which made persistent grants feature
> always disabled.
>
> SeongJae Park (3):
> xen-blkback: Advertise feature-persistent as user requested
> xen-blkfront: Advertise feature-persistent as user requested
> xen-blkfront: Cache feature_persistent value before advertisement
>
> drivers/block/xen-blkback/common.h | 3 +++
> drivers/block/xen-blkback/xenbus.c | 6 ++++--
> drivers/block/xen-blkfront.c | 20 ++++++++++++--------
> 3 files changed, 19 insertions(+), 10 deletions(-)
>
For the whole series:
Reviewed-by: Juergen Gross <[email protected]>
Juergen
On Wed, Aug 31, 2022 at 05:08:17PM +0000, SeongJae Park wrote:
> On Wed, 31 Aug 2022 16:58:21 +0000 SeongJae Park <[email protected]> wrote:
>
> > Changes from v1
> > (https://lore.kernel.org/xen-devel/[email protected]/)
> > - Fix the wrong feature_persistent caching position of blkfront
> > - Set blkfront's feature_persistent field setting with simple '&&'
> > instead of 'if' (Pratyush Yadav)
> >
> > This patchset fixes misuse of the 'feature-persistent' advertisement
> > semantic (patches 1 and 2), and the wrong timing of the
> > 'feature_persistent' value caching, which made persistent grants feature
> > always disabled.
>
> Please note that I have some problem in my test setup and therefore was unable
> to fully test this patchset. I am posting this though, as the impact of the
> bug is not trivial (always disabling persistent grants), and to make testing of
> my proposed fix from others easier. Hope to get someone's test results or code
> review of this patchset even before I fix my test setup problem.
I can confirm it fixes the issue, thanks!
Tested-by: Marek Marczykowski-Górecki <[email protected]>
> Juergen, I didn't add your 'Reviewed-by:'s to the first two patches of this
> series because I changed some of the description for making it clear which bug
> and commit it is really fixing. Specifically, I wordsmithed the working and
> changed 'Fixed:' tag. Code change is almost same, though.
>
>
> Thanks,
> SJ
>
> >
> > SeongJae Park (3):
> > xen-blkback: Advertise feature-persistent as user requested
> > xen-blkfront: Advertise feature-persistent as user requested
> > xen-blkfront: Cache feature_persistent value before advertisement
> >
> > drivers/block/xen-blkback/common.h | 3 +++
> > drivers/block/xen-blkback/xenbus.c | 6 ++++--
> > drivers/block/xen-blkfront.c | 20 ++++++++++++--------
> > 3 files changed, 19 insertions(+), 10 deletions(-)
> >
> > --
> > 2.25.1
> >
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
On Wed, Aug 31, 2022 at 04:58:21PM +0000, SeongJae Park wrote:
> Changes from v1
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Fix the wrong feature_persistent caching position of blkfront
> - Set blkfront's feature_persistent field setting with simple '&&'
> instead of 'if' (Pratyush Yadav)
>
> This patchset fixes misuse of the 'feature-persistent' advertisement
> semantic (patches 1 and 2), and the wrong timing of the
> 'feature_persistent' value caching, which made persistent grants feature
> always disabled.
>
> SeongJae Park (3):
> xen-blkback: Advertise feature-persistent as user requested
> xen-blkfront: Advertise feature-persistent as user requested
> xen-blkfront: Cache feature_persistent value before advertisement
>
> drivers/block/xen-blkback/common.h | 3 +++
> drivers/block/xen-blkback/xenbus.c | 6 ++++--
> drivers/block/xen-blkfront.c | 20 ++++++++++++--------
> 3 files changed, 19 insertions(+), 10 deletions(-)
>
> --
> 2.25.1
>
I've tested this patch series in the following ways:
* Only applied the blkback patch but not the blkfront patches
* Only applied the blkfront patches but not the blkback patch
* Applied both
All scenarios worked, so
Tested-by: Maximilian Heyne <[email protected]>
Actually I also wanted to test changing feature_persistent and try reconnecting
but I don't know how this is done. If anyone has a pointer here, I could test
that as well.
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879