Introduction of 'feature_persistent' made two bugs. First one is wrong
overwrite of 'vbd->feature_gnt_persistent' in 'blkback' due to wrong
parameter value caching position, and the second one is unintended
behavioral change that could break previous dynamic frontend/backend
persistent feature support changes. This patchset fixes the issues.
Changes from v3
(https://lore.kernel.org/xen-devel/[email protected]/)
- Split 'blkback' patch for each of the two issues
- Add 'Reported-by: Andrii Chepurnyi <[email protected]>'
Changes from v2
(https://lore.kernel.org/xen-devel/[email protected]/)
- Keep the behavioral change of v1
- Update blkfront's counterpart to follow the changed behavior
- Update documents for the changed behavior
Changes from v1
(https://lore.kernel.org/xen-devel/[email protected]/)
- Avoid the behavioral change
(https://lore.kernel.org/xen-devel/[email protected]/)
- Rebase on latest xen/tip/linux-next
- Re-work by SeongJae Park <[email protected]>
- Cc stable@
Maximilian Heyne (1):
xen-blkback: Apply 'feature_persistent' parameter when connect
SeongJae Park (2):
xen-blkback: fix persistent grants negotiation
xen-blkfront: Apply 'feature_persistent' parameter when connect
.../ABI/testing/sysfs-driver-xen-blkback | 2 +-
.../ABI/testing/sysfs-driver-xen-blkfront | 2 +-
drivers/block/xen-blkback/xenbus.c | 20 ++++++++-----------
drivers/block/xen-blkfront.c | 4 +---
4 files changed, 11 insertions(+), 17 deletions(-)
--
2.25.1
Persistent grants feature can be used only when both backend and the
frontend supports the feature. The feature was always supported by
'blkback', but commit aac8a70db24b ("xen-blkback: add a parameter for
disabling of persistent grants") has introduced a parameter for
disabling it runtime.
To avoid the parameter be updated while being used by 'blkback', the
commit caches the parameter into 'vbd->feature_gnt_persistent' in
'xen_vbd_create()', and then check if the guest also supports the
feature and finally updates the field in 'connect_ring()'.
However, 'connect_ring()' could be called before 'xen_vbd_create()', so
later execution of 'xen_vbd_create()' can wrongly overwrite 'true' to
'vbd->feature_gnt_persistent'. As a result, 'blkback' could try to use
'persistent grants' feature even if the guest doesn't support the
feature.
This commit fixes the issue by moving the parameter value caching to
'xen_blkif_alloc()', which allocates the 'blkif'. Because the struct
embeds 'vbd' object, which will be used by 'connect_ring()' later, this
should be called before 'connect_ring()' and therefore this should be
the right and safe place to do the caching.
Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent grants")
Cc: <[email protected]> # 5.10.x
Signed-off-by: Maximilian Heyne <[email protected]>
Signed-off-by: SeongJae Park <[email protected]>
---
drivers/block/xen-blkback/xenbus.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 97de13b14175..16c6785d260c 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -157,6 +157,11 @@ static int xen_blkif_alloc_rings(struct xen_blkif *blkif)
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");
+
static struct xen_blkif *xen_blkif_alloc(domid_t domid)
{
struct xen_blkif *blkif;
@@ -181,6 +186,8 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid)
__module_get(THIS_MODULE);
INIT_WORK(&blkif->free_work, xen_blkif_deferred_free);
+ blkif->vbd.feature_gnt_persistent = feature_persistent;
+
return blkif;
}
@@ -472,12 +479,6 @@ static void xen_vbd_free(struct xen_vbd *vbd)
vbd->bdev = NULL;
}
-/* 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");
-
static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
unsigned major, unsigned minor, int readonly,
int cdrom)
@@ -520,8 +521,6 @@ static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
if (bdev_max_secure_erase_sectors(bdev))
vbd->discard_secure = true;
- vbd->feature_gnt_persistent = feature_persistent;
-
pr_debug("Successful creation of handle=%04x (dom=%u)\n",
handle, blkif->domid);
return 0;
--
2.25.1
In some use cases[1], the backend is created while the frontend doesn't
support the persistent grants feature, but later the frontend can be
changed to support the feature and reconnect. In the past, 'blkback'
enabled the persistent grants feature since it unconditionally checked
if frontend supports the persistent grants feature for every connect
('connect_ring()') and decided whether it should use persistent grans or
not.
However, commit aac8a70db24b ("xen-blkback: add a parameter for
disabling of persistent grants") has mistakenly changed the behavior.
It made the frontend feature support check to not be repeated once it
shown the 'feature_persistent' as 'false', or the frontend doesn't
support persistent grants.
Similar behavioral change has made on 'blkfront' by commit 74a852479c68
("xen-blkfront: add a parameter for disabling of persistent grants").
This commit changes the behavior of the parameter to make effect for
every connect, so that the previous behavior of 'blkfront' can be
restored.
[1] https://lore.kernel.org/xen-devel/CAJwUmVB6H3iTs-C+U=v-pwJB7-_ZRHPxHzKRJZ22xEPW7z8a=g@mail.gmail.com/
Fixes: 74a852479c68 ("xen-blkfront: add a parameter for disabling of persistent grants")
Cc: <[email protected]> # 5.10.x
Signed-off-by: SeongJae Park <[email protected]>
---
Documentation/ABI/testing/sysfs-driver-xen-blkfront | 2 +-
drivers/block/xen-blkfront.c | 4 +---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkfront b/Documentation/ABI/testing/sysfs-driver-xen-blkfront
index 7f646c58832e..4d36c5a10546 100644
--- a/Documentation/ABI/testing/sysfs-driver-xen-blkfront
+++ b/Documentation/ABI/testing/sysfs-driver-xen-blkfront
@@ -15,5 +15,5 @@ KernelVersion: 5.10
Contact: Maximilian Heyne <[email protected]>
Description:
Whether to enable the persistent grants feature or not. Note
- that this option only takes effect on newly created frontends.
+ that this option only takes effect on newly connected frontends.
The default is Y (enable).
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 3646c0cae672..4e763701b372 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1988,8 +1988,6 @@ static int blkfront_probe(struct xenbus_device *dev,
info->vdevice = vdevice;
info->connected = BLKIF_STATE_DISCONNECTED;
- info->feature_persistent = feature_persistent;
-
/* Front end dir is a number, which is used as the id. */
info->handle = simple_strtoul(strrchr(dev->nodename, '/')+1, NULL, 0);
dev_set_drvdata(&dev->dev, info);
@@ -2283,7 +2281,7 @@ static void blkfront_gather_backend_features(struct blkfront_info *info)
if (xenbus_read_unsigned(info->xbdev->otherend, "feature-discard", 0))
blkfront_setup_discard(info);
- if (info->feature_persistent)
+ if (feature_persistent)
info->feature_persistent =
!!xenbus_read_unsigned(info->xbdev->otherend,
"feature-persistent", 0);
--
2.25.1
From: Maximilian Heyne <[email protected]>
In some use cases[1], the backend is created while the frontend doesn't
support the persistent grants feature, but later the frontend can be
changed to support the feature and reconnect. In the past, 'blkback'
enabled the persistent grants feature since it unconditionally checked
if frontend supports the persistent grants feature for every connect
('connect_ring()') and decided whether it should use persistent grans or
not.
However, commit aac8a70db24b ("xen-blkback: add a parameter for
disabling of persistent grants") has mistakenly changed the behavior.
It made the frontend feature support check to not be repeated once it
shown the 'feature_persistent' as 'false', or the frontend doesn't
support persistent grants.
This commit changes the behavior of the parameter to make effect for
every connect, so that the previous workflow can work again as expected.
[1] https://lore.kernel.org/xen-devel/CAJwUmVB6H3iTs-C+U=v-pwJB7-_ZRHPxHzKRJZ22xEPW7z8a=g@mail.gmail.com/
Reported-by: Andrii Chepurnyi <[email protected]>
Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent grants")
Cc: <[email protected]> # 5.10.x
Signed-off-by: Maximilian Heyne <[email protected]>
Signed-off-by: SeongJae Park <[email protected]>
---
Documentation/ABI/testing/sysfs-driver-xen-blkback | 2 +-
drivers/block/xen-blkback/xenbus.c | 9 +++------
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkback b/Documentation/ABI/testing/sysfs-driver-xen-blkback
index 7faf719af165..fac0f429a869 100644
--- a/Documentation/ABI/testing/sysfs-driver-xen-blkback
+++ b/Documentation/ABI/testing/sysfs-driver-xen-blkback
@@ -42,5 +42,5 @@ KernelVersion: 5.10
Contact: Maximilian Heyne <[email protected]>
Description:
Whether to enable the persistent grants feature or not. Note
- that this option only takes effect on newly created backends.
+ that this option only takes effect on newly connected backends.
The default is Y (enable).
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 16c6785d260c..ee7ad2fb432d 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -186,8 +186,6 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid)
__module_get(THIS_MODULE);
INIT_WORK(&blkif->free_work, xen_blkif_deferred_free);
- blkif->vbd.feature_gnt_persistent = feature_persistent;
-
return blkif;
}
@@ -1086,10 +1084,9 @@ static int connect_ring(struct backend_info *be)
xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
return -ENOSYS;
}
- if (blkif->vbd.feature_gnt_persistent)
- blkif->vbd.feature_gnt_persistent =
- xenbus_read_unsigned(dev->otherend,
- "feature-persistent", 0);
+
+ blkif->vbd.feature_gnt_persistent = feature_persistent &&
+ xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
blkif->vbd.overflow_max_grants = 0;
--
2.25.1
Hello SeongJae,
Thanks for the efforts.
I've tested backend patches(1,2) on my custom 5.10 kernel (since I
can't test on vanilla) and it works for me.
Best regards,
Andrii
On Sat, Jul 16, 2022 at 1:51 AM SeongJae Park <[email protected]> wrote:
>
> Introduction of 'feature_persistent' made two bugs. First one is wrong
> overwrite of 'vbd->feature_gnt_persistent' in 'blkback' due to wrong
> parameter value caching position, and the second one is unintended
> behavioral change that could break previous dynamic frontend/backend
> persistent feature support changes. This patchset fixes the issues.
>
> Changes from v3
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Split 'blkback' patch for each of the two issues
> - Add 'Reported-by: Andrii Chepurnyi <[email protected]>'
>
> Changes from v2
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Keep the behavioral change of v1
> - Update blkfront's counterpart to follow the changed behavior
> - Update documents for the changed behavior
>
> Changes from v1
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Avoid the behavioral change
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Rebase on latest xen/tip/linux-next
> - Re-work by SeongJae Park <[email protected]>
> - Cc stable@
>
> Maximilian Heyne (1):
> xen-blkback: Apply 'feature_persistent' parameter when connect
>
> SeongJae Park (2):
> xen-blkback: fix persistent grants negotiation
> xen-blkfront: Apply 'feature_persistent' parameter when connect
>
> .../ABI/testing/sysfs-driver-xen-blkback | 2 +-
> .../ABI/testing/sysfs-driver-xen-blkfront | 2 +-
> drivers/block/xen-blkback/xenbus.c | 20 ++++++++-----------
> drivers/block/xen-blkfront.c | 4 +---
> 4 files changed, 11 insertions(+), 17 deletions(-)
>
> --
> 2.25.1
>
On Fri, Jul 15, 2022 at 10:51:05PM +0000, SeongJae Park wrote:
>
> Introduction of 'feature_persistent' made two bugs. First one is wrong
> overwrite of 'vbd->feature_gnt_persistent' in 'blkback' due to wrong
> parameter value caching position, and the second one is unintended
> behavioral change that could break previous dynamic frontend/backend
> persistent feature support changes. This patchset fixes the issues.
>
> Changes from v3
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Split 'blkback' patch for each of the two issues
> - Add 'Reported-by: Andrii Chepurnyi <[email protected]>'
>
> Changes from v2
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Keep the behavioral change of v1
> - Update blkfront's counterpart to follow the changed behavior
> - Update documents for the changed behavior
>
> Changes from v1
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Avoid the behavioral change
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Rebase on latest xen/tip/linux-next
> - Re-work by SeongJae Park <[email protected]>
> - Cc stable@
>
> Maximilian Heyne (1):
> xen-blkback: Apply 'feature_persistent' parameter when connect
>
> SeongJae Park (2):
> xen-blkback: fix persistent grants negotiation
> xen-blkfront: Apply 'feature_persistent' parameter when connect
>
> .../ABI/testing/sysfs-driver-xen-blkback | 2 +-
> .../ABI/testing/sysfs-driver-xen-blkfront | 2 +-
> drivers/block/xen-blkback/xenbus.c | 20 ++++++++-----------
> drivers/block/xen-blkfront.c | 4 +---
> 4 files changed, 11 insertions(+), 17 deletions(-)
>
> --
> 2.25.1
>
Changes look good to me. Thank you for reworking my patch and also fixing the
blkfront driver.
Reviewed-by: Maximilian Heyne <[email protected]>
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
On 16.07.22 00:51, SeongJae Park wrote:
> Introduction of 'feature_persistent' made two bugs. First one is wrong
> overwrite of 'vbd->feature_gnt_persistent' in 'blkback' due to wrong
> parameter value caching position, and the second one is unintended
> behavioral change that could break previous dynamic frontend/backend
> persistent feature support changes. This patchset fixes the issues.
>
> Changes from v3
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Split 'blkback' patch for each of the two issues
> - Add 'Reported-by: Andrii Chepurnyi <[email protected]>'
>
> Changes from v2
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Keep the behavioral change of v1
> - Update blkfront's counterpart to follow the changed behavior
> - Update documents for the changed behavior
>
> Changes from v1
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Avoid the behavioral change
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Rebase on latest xen/tip/linux-next
> - Re-work by SeongJae Park <[email protected]>
> - Cc stable@
>
> Maximilian Heyne (1):
> xen-blkback: Apply 'feature_persistent' parameter when connect
>
> SeongJae Park (2):
> xen-blkback: fix persistent grants negotiation
> xen-blkfront: Apply 'feature_persistent' parameter when connect
>
> .../ABI/testing/sysfs-driver-xen-blkback | 2 +-
> .../ABI/testing/sysfs-driver-xen-blkfront | 2 +-
> drivers/block/xen-blkback/xenbus.c | 20 ++++++++-----------
> drivers/block/xen-blkfront.c | 4 +---
> 4 files changed, 11 insertions(+), 17 deletions(-)
>
For the series:
Reviewed-by: Juergen Gross <[email protected]>
Juergen
On 16.07.22 00:51, SeongJae Park wrote:
> Introduction of 'feature_persistent' made two bugs. First one is wrong
> overwrite of 'vbd->feature_gnt_persistent' in 'blkback' due to wrong
> parameter value caching position, and the second one is unintended
> behavioral change that could break previous dynamic frontend/backend
> persistent feature support changes. This patchset fixes the issues.
>
> Changes from v3
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Split 'blkback' patch for each of the two issues
> - Add 'Reported-by: Andrii Chepurnyi <[email protected]>'
>
> Changes from v2
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Keep the behavioral change of v1
> - Update blkfront's counterpart to follow the changed behavior
> - Update documents for the changed behavior
>
> Changes from v1
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Avoid the behavioral change
> (https://lore.kernel.org/xen-devel/[email protected]/)
> - Rebase on latest xen/tip/linux-next
> - Re-work by SeongJae Park <[email protected]>
> - Cc stable@
>
> Maximilian Heyne (1):
> xen-blkback: Apply 'feature_persistent' parameter when connect
>
> SeongJae Park (2):
> xen-blkback: fix persistent grants negotiation
> xen-blkfront: Apply 'feature_persistent' parameter when connect
>
> .../ABI/testing/sysfs-driver-xen-blkback | 2 +-
> .../ABI/testing/sysfs-driver-xen-blkfront | 2 +-
> drivers/block/xen-blkback/xenbus.c | 20 ++++++++-----------
> drivers/block/xen-blkfront.c | 4 +---
> 4 files changed, 11 insertions(+), 17 deletions(-)
>
Series pushed to xen/tip.git for-linus-6.0
Juergen