All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/3] xen-blk(back|front): Let users disable persistent grants
@ 2020-09-22 14:15 SeongJae Park
  2020-09-22 14:15 ` [PATCH v3 1/3] xen-blkback: add a parameter for disabling of " SeongJae Park
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: SeongJae Park @ 2020-09-22 14:15 UTC (permalink / raw)
  To: konrad.wilk, roger.pau, jgross
  Cc: SeongJae Park, axboe, aliguori, amit, mheyne, pdurrant,
	linux-block, xen-devel, linux-kernel

From: SeongJae Park <sjpark@amazon.de>

Persistent grants feature provides high scalability.  On some small
systems, however, it could incur data copy overheads[1] and thus it is
required to be disabled.  But, there is no option to disable it.  For
the reason, this commit adds module parameters for disabling of the
feature.

[1] https://wiki.xen.org/wiki/Xen_4.3_Block_Protocol_Scalability

Baseline and Complete Git Trees
===============================

The patches are based on the v5.9-rc6.  You can also clone the complete
git tree:

    $ git clone git://github.com/sjp38/linux -b pgrants_disable_v3

The web is also available:
https://github.com/sjp38/linux/tree/pgrants_disable_v3

Patch History
=============

Changes from v2
(https://lore.kernel.org/linux-block/20200922105209.5284-1-sjpark@amazon.com/)
- Avoid race conditions (Roger Pau Monné)

Changes from v1
(https://lore.kernel.org/linux-block/20200922070125.27251-1-sjpark@amazon.com/)
- use 'bool' parameter type (Jürgen Groß)
- Let blkfront can also disable the feature from its side
  (Roger Pau Monné)
- Avoid unnecessary xenbus_printf (Roger Pau Monné)
- Update frontend parameter doc


SeongJae Park (3):
  xen-blkback: add a parameter for disabling of persistent grants
  xen-blkfront: add a parameter for disabling of persistent grants
  xen-blkfront: Apply changed parameter name to the document

 .../ABI/testing/sysfs-driver-xen-blkback      |  9 ++++++++
 .../ABI/testing/sysfs-driver-xen-blkfront     | 11 +++++++++-
 drivers/block/xen-blkback/xenbus.c            | 22 ++++++++++++++-----
 drivers/block/xen-blkfront.c                  | 20 ++++++++++++-----
 4 files changed, 50 insertions(+), 12 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 1/3] xen-blkback: add a parameter for disabling of persistent grants
  2020-09-22 14:15 [PATCH v3 0/3] xen-blk(back|front): Let users disable persistent grants SeongJae Park
@ 2020-09-22 14:15 ` SeongJae Park
  2020-09-22 14:25   ` Jürgen Groß
  2020-09-22 14:15 ` [PATCH v3 2/3] xen-blkfront: " SeongJae Park
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: SeongJae Park @ 2020-09-22 14:15 UTC (permalink / raw)
  To: konrad.wilk, roger.pau, jgross
  Cc: SeongJae Park, axboe, aliguori, amit, mheyne, pdurrant,
	linux-block, xen-devel, linux-kernel

From: SeongJae Park <sjpark@amazon.de>

Persistent grants feature provides high scalability.  On some small
systems, however, it could incur data copy overheads[1] and thus it is
required to be disabled.  But, there is no option to disable it.  For
the reason, this commit adds a module parameter for disabling of the
feature.

[1] https://wiki.xen.org/wiki/Xen_4.3_Block_Protocol_Scalability

Signed-off-by: Anthony Liguori <aliguori@amazon.com>
Signed-off-by: SeongJae Park <sjpark@amazon.de>
---
 .../ABI/testing/sysfs-driver-xen-blkback      |  9 ++++++++
 drivers/block/xen-blkback/xenbus.c            | 22 ++++++++++++++-----
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkback b/Documentation/ABI/testing/sysfs-driver-xen-blkback
index ecb7942ff146..ac2947b98950 100644
--- a/Documentation/ABI/testing/sysfs-driver-xen-blkback
+++ b/Documentation/ABI/testing/sysfs-driver-xen-blkback
@@ -35,3 +35,12 @@ Description:
                 controls the duration in milliseconds that blkback will not
                 cache any page not backed by a grant mapping.
                 The default is 10ms.
+
+What:           /sys/module/xen_blkback/parameters/feature_persistent
+Date:           September 2020
+KernelVersion:  5.10
+Contact:        SeongJae Park <sjpark@amazon.de>
+Description:
+                Whether to enable the persistent grants feature or not.  Note
+                that this option only takes effect on newly created backends.
+                The default is Y (enable).
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index b9aa5d1ac10b..f4c8827fa0ad 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -474,6 +474,12 @@ 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)
@@ -519,6 +525,8 @@ static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
 	if (q && blk_queue_secure_erase(q))
 		vbd->discard_secure = true;
 
+	vbd->feature_gnt_persistent = feature_persistent ? 1 : 0;
+
 	pr_debug("Successful creation of handle=%04x (dom=%u)\n",
 		handle, blkif->domid);
 	return 0;
@@ -906,7 +914,8 @@ static void connect(struct backend_info *be)
 
 	xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
 
-	err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u", 1);
+	err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
+			be->blkif->vbd.feature_gnt_persistent);
 	if (err) {
 		xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
 				 dev->nodename);
@@ -1067,7 +1076,6 @@ static int connect_ring(struct backend_info *be)
 {
 	struct xenbus_device *dev = be->dev;
 	struct xen_blkif *blkif = be->blkif;
-	unsigned int pers_grants;
 	char protocol[64] = "";
 	int err, i;
 	char *xspath;
@@ -1093,9 +1101,11 @@ static int connect_ring(struct backend_info *be)
 		xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
 		return -ENOSYS;
 	}
-	pers_grants = xenbus_read_unsigned(dev->otherend, "feature-persistent",
-					   0);
-	blkif->vbd.feature_gnt_persistent = pers_grants;
+	if (blkif->vbd.feature_gnt_persistent)
+		blkif->vbd.feature_gnt_persistent =
+			xenbus_read_unsigned(dev->otherend,
+					"feature-persistent", 0);
+
 	blkif->vbd.overflow_max_grants = 0;
 
 	/*
@@ -1118,7 +1128,7 @@ static int connect_ring(struct backend_info *be)
 
 	pr_info("%s: using %d queues, protocol %d (%s) %s\n", dev->nodename,
 		 blkif->nr_rings, blkif->blk_protocol, protocol,
-		 pers_grants ? "persistent grants" : "");
+		 blkif->vbd.feature_gnt_persistent ? "persistent grants" : "");
 
 	ring_page_order = xenbus_read_unsigned(dev->otherend,
 					       "ring-page-order", 0);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 2/3] xen-blkfront: add a parameter for disabling of persistent grants
  2020-09-22 14:15 [PATCH v3 0/3] xen-blk(back|front): Let users disable persistent grants SeongJae Park
  2020-09-22 14:15 ` [PATCH v3 1/3] xen-blkback: add a parameter for disabling of " SeongJae Park
@ 2020-09-22 14:15 ` SeongJae Park
  2020-09-22 14:27   ` Jürgen Groß
  2020-09-22 14:15 ` [PATCH v3 3/3] xen-blkfront: Apply changed parameter name to the document SeongJae Park
  2020-09-22 14:45 ` [PATCH v3 0/3] xen-blk(back|front): Let users disable persistent grants Roger Pau Monné
  3 siblings, 1 reply; 13+ messages in thread
From: SeongJae Park @ 2020-09-22 14:15 UTC (permalink / raw)
  To: konrad.wilk, roger.pau, jgross
  Cc: SeongJae Park, axboe, aliguori, amit, mheyne, pdurrant,
	linux-block, xen-devel, linux-kernel

From: SeongJae Park <sjpark@amazon.de>

Persistent grants feature provides high scalability.  On some small
systems, however, it could incur data copy overheads[1] and thus it is
required to be disabled.  It can be disabled from blkback side using a
module parameter, 'feature_persistent'.  But, it is impossible from
blkfront side.  For the reason, this commit adds a blkfront module
parameter for disabling of the feature.

[1] https://wiki.xen.org/wiki/Xen_4.3_Block_Protocol_Scalability

Signed-off-by: SeongJae Park <sjpark@amazon.de>
---
 .../ABI/testing/sysfs-driver-xen-blkfront     |  9 +++++++++
 drivers/block/xen-blkfront.c                  | 20 ++++++++++++++-----
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkfront b/Documentation/ABI/testing/sysfs-driver-xen-blkfront
index c0a6cb7eb314..9c31334cb2e6 100644
--- a/Documentation/ABI/testing/sysfs-driver-xen-blkfront
+++ b/Documentation/ABI/testing/sysfs-driver-xen-blkfront
@@ -8,3 +8,12 @@ Description:
                 is 32 - higher value means more potential throughput but more
                 memory usage. The backend picks the minimum of the frontend
                 and its default backend value.
+
+What:           /sys/module/xen_blkfront/parameters/feature_persistent
+Date:           September 2020
+KernelVersion:  5.10
+Contact:        SeongJae Park <sjpark@amazon.de>
+Description:
+                Whether to enable the persistent grants feature or not.  Note
+                that this option only takes effect on newly created frontends.
+                The default is Y (enable).
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 91de2e0755ae..b2d3b1262404 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1866,8 +1866,8 @@ static int talk_to_blkback(struct xenbus_device *dev,
 		message = "writing protocol";
 		goto abort_transaction;
 	}
-	err = xenbus_printf(xbt, dev->nodename,
-			    "feature-persistent", "%u", 1);
+	err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
+			info->feature_persistent);
 	if (err)
 		dev_warn(&dev->dev,
 			 "writing persistent grants feature to xenbus");
@@ -1941,6 +1941,13 @@ 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
@@ -2007,6 +2014,8 @@ static int blkfront_probe(struct xenbus_device *dev,
 	info->vdevice = vdevice;
 	info->connected = BLKIF_STATE_DISCONNECTED;
 
+	info->feature_persistent = feature_persistent ? 1 : 0;
+
 	/* 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);
@@ -2316,9 +2325,10 @@ 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 =
-		!!xenbus_read_unsigned(info->xbdev->otherend,
-				       "feature-persistent", 0);
+	if (info->feature_persistent)
+		info->feature_persistent =
+			!!xenbus_read_unsigned(info->xbdev->otherend,
+					       "feature-persistent", 0);
 
 	indirect_segments = xenbus_read_unsigned(info->xbdev->otherend,
 					"feature-max-indirect-segments", 0);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 3/3] xen-blkfront: Apply changed parameter name to the document
  2020-09-22 14:15 [PATCH v3 0/3] xen-blk(back|front): Let users disable persistent grants SeongJae Park
  2020-09-22 14:15 ` [PATCH v3 1/3] xen-blkback: add a parameter for disabling of " SeongJae Park
  2020-09-22 14:15 ` [PATCH v3 2/3] xen-blkfront: " SeongJae Park
@ 2020-09-22 14:15 ` SeongJae Park
  2020-09-22 14:27   ` Jürgen Groß
  2020-09-22 14:45 ` [PATCH v3 0/3] xen-blk(back|front): Let users disable persistent grants Roger Pau Monné
  3 siblings, 1 reply; 13+ messages in thread
From: SeongJae Park @ 2020-09-22 14:15 UTC (permalink / raw)
  To: konrad.wilk, roger.pau, jgross
  Cc: SeongJae Park, axboe, aliguori, amit, mheyne, pdurrant,
	linux-block, xen-devel, linux-kernel

From: SeongJae Park <sjpark@amazon.de>

Commit 14e710fe7897 ("xen-blkfront: rename indirect descriptor
parameter") changed the name of the module parameter for the maximum
amount of segments in indirect requests but missed updating the
document.  This commit updates the document.

Signed-off-by: SeongJae Park <sjpark@amazon.de>
---
 Documentation/ABI/testing/sysfs-driver-xen-blkfront | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkfront b/Documentation/ABI/testing/sysfs-driver-xen-blkfront
index 9c31334cb2e6..28008905615f 100644
--- a/Documentation/ABI/testing/sysfs-driver-xen-blkfront
+++ b/Documentation/ABI/testing/sysfs-driver-xen-blkfront
@@ -1,4 +1,4 @@
-What:           /sys/module/xen_blkfront/parameters/max
+What:           /sys/module/xen_blkfront/parameters/max_indirect_segments
 Date:           June 2013
 KernelVersion:  3.11
 Contact:        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/3] xen-blkback: add a parameter for disabling of persistent grants
  2020-09-22 14:15 ` [PATCH v3 1/3] xen-blkback: add a parameter for disabling of " SeongJae Park
@ 2020-09-22 14:25   ` Jürgen Groß
  2020-09-22 14:30     ` SeongJae Park
  0 siblings, 1 reply; 13+ messages in thread
From: Jürgen Groß @ 2020-09-22 14:25 UTC (permalink / raw)
  To: SeongJae Park, konrad.wilk, roger.pau
  Cc: SeongJae Park, axboe, aliguori, amit, mheyne, pdurrant,
	linux-block, xen-devel, linux-kernel

On 22.09.20 16:15, SeongJae Park wrote:
> From: SeongJae Park <sjpark@amazon.de>
> 
> Persistent grants feature provides high scalability.  On some small
> systems, however, it could incur data copy overheads[1] and thus it is
> required to be disabled.  But, there is no option to disable it.  For
> the reason, this commit adds a module parameter for disabling of the
> feature.
> 
> [1] https://wiki.xen.org/wiki/Xen_4.3_Block_Protocol_Scalability
> 
> Signed-off-by: Anthony Liguori <aliguori@amazon.com>
> Signed-off-by: SeongJae Park <sjpark@amazon.de>
> ---
>   .../ABI/testing/sysfs-driver-xen-blkback      |  9 ++++++++
>   drivers/block/xen-blkback/xenbus.c            | 22 ++++++++++++++-----
>   2 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkback b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> index ecb7942ff146..ac2947b98950 100644
> --- a/Documentation/ABI/testing/sysfs-driver-xen-blkback
> +++ b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> @@ -35,3 +35,12 @@ Description:
>                   controls the duration in milliseconds that blkback will not
>                   cache any page not backed by a grant mapping.
>                   The default is 10ms.
> +
> +What:           /sys/module/xen_blkback/parameters/feature_persistent
> +Date:           September 2020
> +KernelVersion:  5.10
> +Contact:        SeongJae Park <sjpark@amazon.de>
> +Description:
> +                Whether to enable the persistent grants feature or not.  Note
> +                that this option only takes effect on newly created backends.
> +                The default is Y (enable).
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index b9aa5d1ac10b..f4c8827fa0ad 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -474,6 +474,12 @@ 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)
> @@ -519,6 +525,8 @@ static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
>   	if (q && blk_queue_secure_erase(q))
>   		vbd->discard_secure = true;
>   
> +	vbd->feature_gnt_persistent = feature_persistent ? 1 : 0;

Just assign the value instead of using the ternary operator?

With that changed you can add my

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/3] xen-blkfront: add a parameter for disabling of persistent grants
  2020-09-22 14:15 ` [PATCH v3 2/3] xen-blkfront: " SeongJae Park
@ 2020-09-22 14:27   ` Jürgen Groß
  2020-09-22 14:31     ` SeongJae Park
  0 siblings, 1 reply; 13+ messages in thread
From: Jürgen Groß @ 2020-09-22 14:27 UTC (permalink / raw)
  To: SeongJae Park, konrad.wilk, roger.pau
  Cc: SeongJae Park, axboe, aliguori, amit, mheyne, pdurrant,
	linux-block, xen-devel, linux-kernel

On 22.09.20 16:15, SeongJae Park wrote:
> From: SeongJae Park <sjpark@amazon.de>
> 
> Persistent grants feature provides high scalability.  On some small
> systems, however, it could incur data copy overheads[1] and thus it is
> required to be disabled.  It can be disabled from blkback side using a
> module parameter, 'feature_persistent'.  But, it is impossible from
> blkfront side.  For the reason, this commit adds a blkfront module
> parameter for disabling of the feature.
> 
> [1] https://wiki.xen.org/wiki/Xen_4.3_Block_Protocol_Scalability
> 
> Signed-off-by: SeongJae Park <sjpark@amazon.de>
> ---
>   .../ABI/testing/sysfs-driver-xen-blkfront     |  9 +++++++++
>   drivers/block/xen-blkfront.c                  | 20 ++++++++++++++-----
>   2 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkfront b/Documentation/ABI/testing/sysfs-driver-xen-blkfront
> index c0a6cb7eb314..9c31334cb2e6 100644
> --- a/Documentation/ABI/testing/sysfs-driver-xen-blkfront
> +++ b/Documentation/ABI/testing/sysfs-driver-xen-blkfront
> @@ -8,3 +8,12 @@ Description:
>                   is 32 - higher value means more potential throughput but more
>                   memory usage. The backend picks the minimum of the frontend
>                   and its default backend value.
> +
> +What:           /sys/module/xen_blkfront/parameters/feature_persistent
> +Date:           September 2020
> +KernelVersion:  5.10
> +Contact:        SeongJae Park <sjpark@amazon.de>
> +Description:
> +                Whether to enable the persistent grants feature or not.  Note
> +                that this option only takes effect on newly created frontends.
> +                The default is Y (enable).
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 91de2e0755ae..b2d3b1262404 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1866,8 +1866,8 @@ static int talk_to_blkback(struct xenbus_device *dev,
>   		message = "writing protocol";
>   		goto abort_transaction;
>   	}
> -	err = xenbus_printf(xbt, dev->nodename,
> -			    "feature-persistent", "%u", 1);
> +	err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> +			info->feature_persistent);
>   	if (err)
>   		dev_warn(&dev->dev,
>   			 "writing persistent grants feature to xenbus");
> @@ -1941,6 +1941,13 @@ 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
> @@ -2007,6 +2014,8 @@ static int blkfront_probe(struct xenbus_device *dev,
>   	info->vdevice = vdevice;
>   	info->connected = BLKIF_STATE_DISCONNECTED;
>   
> +	info->feature_persistent = feature_persistent ? 1 : 0;

Just use the value.

With that changed:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/3] xen-blkfront: Apply changed parameter name to the document
  2020-09-22 14:15 ` [PATCH v3 3/3] xen-blkfront: Apply changed parameter name to the document SeongJae Park
@ 2020-09-22 14:27   ` Jürgen Groß
  2020-09-22 14:44     ` Roger Pau Monné
  0 siblings, 1 reply; 13+ messages in thread
From: Jürgen Groß @ 2020-09-22 14:27 UTC (permalink / raw)
  To: SeongJae Park, konrad.wilk, roger.pau
  Cc: SeongJae Park, axboe, aliguori, amit, mheyne, pdurrant,
	linux-block, xen-devel, linux-kernel

On 22.09.20 16:15, SeongJae Park wrote:
> From: SeongJae Park <sjpark@amazon.de>
> 
> Commit 14e710fe7897 ("xen-blkfront: rename indirect descriptor
> parameter") changed the name of the module parameter for the maximum
> amount of segments in indirect requests but missed updating the
> document.  This commit updates the document.
> 
> Signed-off-by: SeongJae Park <sjpark@amazon.de>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/3] xen-blkback: add a parameter for disabling of persistent grants
  2020-09-22 14:25   ` Jürgen Groß
@ 2020-09-22 14:30     ` SeongJae Park
  0 siblings, 0 replies; 13+ messages in thread
From: SeongJae Park @ 2020-09-22 14:30 UTC (permalink / raw)
  To: Jürgen Groß
  Cc: SeongJae Park, konrad.wilk, roger.pau, SeongJae Park, axboe,
	aliguori, amit, mheyne, pdurrant, linux-block, xen-devel,
	linux-kernel

On Tue, 22 Sep 2020 16:25:57 +0200 "Jürgen Groß" <jgross@suse.com> wrote:

> On 22.09.20 16:15, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@amazon.de>
> > 
> > Persistent grants feature provides high scalability.  On some small
> > systems, however, it could incur data copy overheads[1] and thus it is
> > required to be disabled.  But, there is no option to disable it.  For
> > the reason, this commit adds a module parameter for disabling of the
> > feature.
> > 
> > [1] https://wiki.xen.org/wiki/Xen_4.3_Block_Protocol_Scalability
> > 
> > Signed-off-by: Anthony Liguori <aliguori@amazon.com>
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> > ---
> >   .../ABI/testing/sysfs-driver-xen-blkback      |  9 ++++++++
> >   drivers/block/xen-blkback/xenbus.c            | 22 ++++++++++++++-----
> >   2 files changed, 25 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkback b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> > index ecb7942ff146..ac2947b98950 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-xen-blkback
> > +++ b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> > @@ -35,3 +35,12 @@ Description:
> >                   controls the duration in milliseconds that blkback will not
> >                   cache any page not backed by a grant mapping.
> >                   The default is 10ms.
> > +
> > +What:           /sys/module/xen_blkback/parameters/feature_persistent
> > +Date:           September 2020
> > +KernelVersion:  5.10
> > +Contact:        SeongJae Park <sjpark@amazon.de>
> > +Description:
> > +                Whether to enable the persistent grants feature or not.  Note
> > +                that this option only takes effect on newly created backends.
> > +                The default is Y (enable).
> > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> > index b9aa5d1ac10b..f4c8827fa0ad 100644
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -474,6 +474,12 @@ 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)
> > @@ -519,6 +525,8 @@ static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
> >   	if (q && blk_queue_secure_erase(q))
> >   		vbd->discard_secure = true;
> >   
> > +	vbd->feature_gnt_persistent = feature_persistent ? 1 : 0;
> 
> Just assign the value instead of using the ternary operator?

I will do so in the next version.

> 
> With that changed you can add my
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Thank you for your kind, quick, and detailed review!


Thanks,
SeongJae Park

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/3] xen-blkfront: add a parameter for disabling of persistent grants
  2020-09-22 14:27   ` Jürgen Groß
@ 2020-09-22 14:31     ` SeongJae Park
  0 siblings, 0 replies; 13+ messages in thread
From: SeongJae Park @ 2020-09-22 14:31 UTC (permalink / raw)
  To: Jürgen Groß
  Cc: SeongJae Park, konrad.wilk, roger.pau, SeongJae Park, axboe,
	aliguori, amit, mheyne, pdurrant, linux-block, xen-devel,
	linux-kernel

On Tue, 22 Sep 2020 16:27:03 +0200 "Jürgen Groß" <jgross@suse.com> wrote:

> On 22.09.20 16:15, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@amazon.de>
> > 
> > Persistent grants feature provides high scalability.  On some small
> > systems, however, it could incur data copy overheads[1] and thus it is
> > required to be disabled.  It can be disabled from blkback side using a
> > module parameter, 'feature_persistent'.  But, it is impossible from
> > blkfront side.  For the reason, this commit adds a blkfront module
> > parameter for disabling of the feature.
> > 
> > [1] https://wiki.xen.org/wiki/Xen_4.3_Block_Protocol_Scalability
> > 
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> > ---
> >   .../ABI/testing/sysfs-driver-xen-blkfront     |  9 +++++++++
> >   drivers/block/xen-blkfront.c                  | 20 ++++++++++++++-----
> >   2 files changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkfront b/Documentation/ABI/testing/sysfs-driver-xen-blkfront
> > index c0a6cb7eb314..9c31334cb2e6 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-xen-blkfront
> > +++ b/Documentation/ABI/testing/sysfs-driver-xen-blkfront
> > @@ -8,3 +8,12 @@ Description:
> >                   is 32 - higher value means more potential throughput but more
> >                   memory usage. The backend picks the minimum of the frontend
> >                   and its default backend value.
> > +
> > +What:           /sys/module/xen_blkfront/parameters/feature_persistent
> > +Date:           September 2020
> > +KernelVersion:  5.10
> > +Contact:        SeongJae Park <sjpark@amazon.de>
> > +Description:
> > +                Whether to enable the persistent grants feature or not.  Note
> > +                that this option only takes effect on newly created frontends.
> > +                The default is Y (enable).
> > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > index 91de2e0755ae..b2d3b1262404 100644
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -1866,8 +1866,8 @@ static int talk_to_blkback(struct xenbus_device *dev,
> >   		message = "writing protocol";
> >   		goto abort_transaction;
> >   	}
> > -	err = xenbus_printf(xbt, dev->nodename,
> > -			    "feature-persistent", "%u", 1);
> > +	err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> > +			info->feature_persistent);
> >   	if (err)
> >   		dev_warn(&dev->dev,
> >   			 "writing persistent grants feature to xenbus");
> > @@ -1941,6 +1941,13 @@ 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
> > @@ -2007,6 +2014,8 @@ static int blkfront_probe(struct xenbus_device *dev,
> >   	info->vdevice = vdevice;
> >   	info->connected = BLKIF_STATE_DISCONNECTED;
> >   
> > +	info->feature_persistent = feature_persistent ? 1 : 0;
> 
> Just use the value.

Yes, I will.

> 
> With that changed:
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Appreciate :)


Thanks,
SeongJae Park

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/3] xen-blkfront: Apply changed parameter name to the document
  2020-09-22 14:27   ` Jürgen Groß
@ 2020-09-22 14:44     ` Roger Pau Monné
  2020-09-22 15:02       ` SeongJae Park
  0 siblings, 1 reply; 13+ messages in thread
From: Roger Pau Monné @ 2020-09-22 14:44 UTC (permalink / raw)
  To: Jürgen Groß
  Cc: SeongJae Park, konrad.wilk, SeongJae Park, axboe, aliguori, amit,
	mheyne, pdurrant, linux-block, xen-devel, linux-kernel

On Tue, Sep 22, 2020 at 04:27:39PM +0200, Jürgen Groß wrote:
> On 22.09.20 16:15, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@amazon.de>
> > 
> > Commit 14e710fe7897 ("xen-blkfront: rename indirect descriptor
> > parameter") changed the name of the module parameter for the maximum
> > amount of segments in indirect requests but missed updating the
> > document.  This commit updates the document.
> > 
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Does this need to be backported to stable branches?

Thanks, Roger.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/3] xen-blk(back|front): Let users disable persistent grants
  2020-09-22 14:15 [PATCH v3 0/3] xen-blk(back|front): Let users disable persistent grants SeongJae Park
                   ` (2 preceding siblings ...)
  2020-09-22 14:15 ` [PATCH v3 3/3] xen-blkfront: Apply changed parameter name to the document SeongJae Park
@ 2020-09-22 14:45 ` Roger Pau Monné
  2020-09-22 14:52   ` SeongJae Park
  3 siblings, 1 reply; 13+ messages in thread
From: Roger Pau Monné @ 2020-09-22 14:45 UTC (permalink / raw)
  To: SeongJae Park
  Cc: konrad.wilk, jgross, SeongJae Park, axboe, aliguori, amit,
	mheyne, pdurrant, linux-block, xen-devel, linux-kernel

On Tue, Sep 22, 2020 at 04:15:46PM +0200, SeongJae Park wrote:
> From: SeongJae Park <sjpark@amazon.de>
> 
> Persistent grants feature provides high scalability.  On some small
> systems, however, it could incur data copy overheads[1] and thus it is
> required to be disabled.  But, there is no option to disable it.  For
> the reason, this commit adds module parameters for disabling of the
> feature.

With the changes requested by Jürgen you can add my:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/3] xen-blk(back|front): Let users disable persistent grants
  2020-09-22 14:45 ` [PATCH v3 0/3] xen-blk(back|front): Let users disable persistent grants Roger Pau Monné
@ 2020-09-22 14:52   ` SeongJae Park
  0 siblings, 0 replies; 13+ messages in thread
From: SeongJae Park @ 2020-09-22 14:52 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: SeongJae Park, konrad.wilk, jgross, SeongJae Park, axboe,
	aliguori, amit, mheyne, pdurrant, linux-block, xen-devel,
	linux-kernel

On Tue, 22 Sep 2020 16:45:56 +0200 "Roger Pau Monné" <roger.pau@citrix.com> wrote:

> On Tue, Sep 22, 2020 at 04:15:46PM +0200, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@amazon.de>
> > 
> > Persistent grants feature provides high scalability.  On some small
> > systems, however, it could incur data copy overheads[1] and thus it is
> > required to be disabled.  But, there is no option to disable it.  For
> > the reason, this commit adds module parameters for disabling of the
> > feature.
> 
> With the changes requested by Jürgen you can add my:
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thank you for your kind and helpful reviews!


Thanks,
SeongJae Park

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/3] xen-blkfront: Apply changed parameter name to the document
  2020-09-22 14:44     ` Roger Pau Monné
@ 2020-09-22 15:02       ` SeongJae Park
  0 siblings, 0 replies; 13+ messages in thread
From: SeongJae Park @ 2020-09-22 15:02 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Jürgen Groß,
	SeongJae Park, konrad.wilk, SeongJae Park, axboe, aliguori, amit,
	mheyne, pdurrant, linux-block, xen-devel, linux-kernel

On Tue, 22 Sep 2020 16:44:25 +0200 "Roger Pau Monné" <roger.pau@citrix.com> wrote:

> On Tue, Sep 22, 2020 at 04:27:39PM +0200, Jürgen Groß wrote:
> > On 22.09.20 16:15, SeongJae Park wrote:
> > > From: SeongJae Park <sjpark@amazon.de>
> > > 
> > > Commit 14e710fe7897 ("xen-blkfront: rename indirect descriptor
> > > parameter") changed the name of the module parameter for the maximum
> > > amount of segments in indirect requests but missed updating the
> > > document.  This commit updates the document.
> > > 
> > > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> > 
> > Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> Does this need to be backported to stable branches?

I don't think so, as this is not a bug affecting users?


Thanks,
SeongJae Park

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-09-22 15:03 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-22 14:15 [PATCH v3 0/3] xen-blk(back|front): Let users disable persistent grants SeongJae Park
2020-09-22 14:15 ` [PATCH v3 1/3] xen-blkback: add a parameter for disabling of " SeongJae Park
2020-09-22 14:25   ` Jürgen Groß
2020-09-22 14:30     ` SeongJae Park
2020-09-22 14:15 ` [PATCH v3 2/3] xen-blkfront: " SeongJae Park
2020-09-22 14:27   ` Jürgen Groß
2020-09-22 14:31     ` SeongJae Park
2020-09-22 14:15 ` [PATCH v3 3/3] xen-blkfront: Apply changed parameter name to the document SeongJae Park
2020-09-22 14:27   ` Jürgen Groß
2020-09-22 14:44     ` Roger Pau Monné
2020-09-22 15:02       ` SeongJae Park
2020-09-22 14:45 ` [PATCH v3 0/3] xen-blk(back|front): Let users disable persistent grants Roger Pau Monné
2020-09-22 14:52   ` SeongJae Park

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.