xen-blkback: fix persistent grants negotiation

PATCH Superseded SeongJae Park 3 participants 4 messages Jul 14, 2022 – Jul 15, 2022

Subsystems (3)

Stable branch Block layer Xen block

lore

Share & tools

lore

SeongJae Park

author

2022-07-14 22:44 UTC #0 lore raw

PATCH v2 1/1 xen-blkback: fix persistent grants negotiation

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]>

---

Changes from v1[1]
- Avoid the behavioral change[2]
- Rebase on latest xen/tip/linux-next
- Re-work by SeongJae Park <[email protected]>
- Cc stable@

[1] https://lore.kernel.org/xen-devel/[email protected]/
[2] https://lore.kernel.org/xen-devel/[email protected]/

 drivers/block/xen-blkback/xenbus.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

drivers/block/xen-blkback/xenbus.c +7 -8

index 97de13b14175..16c6785d260c 100644
@@ -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;

Signature · 1 line

2.25.1

Oleksandr

2022-07-15 10:15 UTC #1 lore raw

On 15.07.22 01:44, SeongJae Park wrote:

Hello all.

Adding Andrii Chepurnyi to CC who have played with the use-case which required reconnect recently and faced some issues with feature_persistent handling.

Quoted 88 lines

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]>
---
Changes from v1[1]
- Avoid the behavioral change[2]
- Rebase on latest xen/tip/linux-next
- Re-work by SeongJae Park <[email protected]>
- Cc stable@
[1] https://lore.kernel.org/xen-devel/[email protected]/
[2] https://lore.kernel.org/xen-devel/[email protected]/
  drivers/block/xen-blkback/xenbus.c | 15 +++++++--------
  1 file changed, 7 insertions(+), 8 deletions(-)
drivers/block/xen-blkback/xenbus.c +7 -8
index 97de13b14175..16c6785d260c 100644
@@ -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;

Signature · 3 lines

Regards,

Oleksandr Tyshchenko

Andrii Chepurnyi

2022-07-15 12:05 UTC #2 lore raw

Hello All,

I faced the mentioned issue recently and just to bring more context here is our setup:
We use pvblock backend for Android guest. It starts using u-boot with pvblock support(which frontend doesn't support the persistent grants feature), later it loads and starts the Linux kernel(which frontend supports the persistent grants feature). So in total, we have sequent two different frontends reconnection, the first of which doesn't support persistent grants.
So the original patch [1] perfectly solves the original issue and provides the ability to use persistent grants after the reconnection when Linux frontend which supports persistent grants comes into play.
At the same time [2] will disable the persistent grants feature for the first and second frontend.
Is it possible to keep [1] as is?

[1]
https://lore.kernel.org/xen-devel/[email protected]/
[2] https://lore.kernel.org/xen-devel/[email protected]/

Best regards,
Andrii

Quoted 116 lines · Oleksandr

On 15.07.22 01:44, SeongJae Park wrote:
Hello all.
Adding Andrii Chepurnyi to CC who have played with the use-case which required reconnect recently and faced some issues with feature_persistent handling.
Quoted 17 lines
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.
Quoted 7 lines
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")
Quoted 5 lines
Cc <[email protected]> # 5.10.x
Signed-off-by Maximilian Heyne <[email protected]>
Signed-off-by SeongJae Park <[email protected]>
---
Quoted 7 lines
Changes from v1[1]
- Avoid the behavioral change[2]
- Rebase on latest xen/tip/linux-next
- Re-work by SeongJae Park <[email protected]>
- Cc stable@
[1]
https://lore.kernel.org/xen-devel/[email protected]/
Quoted 1 line
[2]
https://lore.kernel.org/xen-devel/[email protected]/
Review
Quoted 5 lines
  drivers/block/xen-blkback/xenbus.c | 15 +++++++--------
  1 file changed, 7 insertions(+), 8 deletions(-)
(quoted diff) +0 -0
b/drivers/block/xen-blkback/xenbus.c
drivers/block/xen-blkback/xenbus.c static int xen_blkif_alloc_rings(struct xen_blkif
Quoted 4 lines
index 97de13b14175..16c6785d260c 100644
drivers/block/xen-blkback/xenbus.c +0 -0
@@ -157,6 +157,11 @@ static int xen_blkif_alloc_rings(struct xen_blkif
*blkif)
drivers/block/xen-blkback/xenbus.c static int xen_blkif_alloc_rings(struct xen_blkif *blkif)
Quoted 7 lines
drivers/block/xen-blkback/xenbus.c +4 -0
      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");
drivers/block/xen-blkback/xenbus.c static struct xen_blkif *xen_blkif_alloc(domid_t
Quoted 5 lines
drivers/block/xen-blkback/xenbus.c +1 -0
+
  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)
drivers/block/xen-blkback/xenbus.c static int xen_vbd_create(struct xen_blkif *blkif,
Quoted 22 lines
drivers/block/xen-blkback/xenbus.c +2 -6
      __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,
drivers/block/xen-blkback/xenbus.c static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle, · line 526
Quoted 8 lines
drivers/block/xen-blkback/xenbus.c +0 -2
      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;
--
Regards,
Oleksandr Tyshchenko

SeongJae Park

author

2022-07-15 15:46 UTC #3 lore raw

Hello,

Oleksandr, thank you for Cc-ing Andrii. Andrii, thank you for the comment!

Quoted 4 lines · On Fri, 15 Jul 2022 15:00:10 +0300 Andrii Chepurnyi…

[-- Attachment #1: Type: text/plain, Size: 5237 bytes --]
Hello All,

Quoted 13 lines

I faced the mentioned issue recently and just to bring more context here is our setup:
We use pvblock backend for Android guest. It starts using u-boot with pvblock support(which frontend doesn't support the persistent grants feature), later it loads and starts the Linux kernel(which frontend supports the persistent grants feature). So in total, we have sequent two different frontends reconnection, the first of which doesn't support persistent grants.
So the original patch [1] perfectly solves the original issue and provides the ability to use persistent grants after the reconnection when Linux frontend which supports persistent grants comes into play.
At the same time [2] will disable the persistent grants feature for the first and second frontend.

Thank you for this great explanation of your situation.

Quoted 1 line

Is it possible to keep [1] as is?

Yes, my concerns about Max's original patch[1] are conflicting behavior description in the document[1] and different behavior on blkfront-side 'feature_persistent' parameter. I will post Max's patch again with patches for blkfront behavior change and Documents updates.

[1] https://lore.kernel.org/xen-devel/[email protected]/

Thanks,
SJ

Quoted 19 lines

[1]
https://lore.kernel.org/xen-devel/[email protected]/
[2] https://lore.kernel.org/xen-devel/[email protected]/
Best regards,
Andrii
Quoted 9 lines · Oleksandr
On 15.07.22 01:44, SeongJae Park wrote:
Hello all.
Adding Andrii Chepurnyi to CC who have played with the use-case which required reconnect recently and faced some issues with feature_persistent handling.

[...]