[PATCH v2 0/3] xen-blk(back|front): Let users disable persistent grants

[PATCH v2 0/3] xen-blk(back|front): Let users disable persistent grants

[SeongJae Park]

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_v2

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

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

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            | 28 ++++++++++++++-----
 drivers/block/xen-blkfront.c                  | 28 +++++++++++++------
 4 files changed, 60 insertions(+), 16 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] xen-blkback: add a parameter for disabling of persistent grants

[SeongJae Park]

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            | 28 ++++++++++++++-----
 2 files changed, 30 insertions(+), 7 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..8a95ddd08b13 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -879,6 +879,12 @@ static void reclaim_memory(struct xenbus_device *dev)
 
 /* ** Connection ** */
 
+/* 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");
+
 /*
  * Write the physical details regarding the block device to the store, and
  * switch to Connected state.
@@ -906,11 +912,15 @@ 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);
-	if (err) {
-		xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
-				 dev->nodename);
-		goto abort;
+	if (feature_persistent) {
+		err = xenbus_printf(xbt, dev->nodename, "feature-persistent",
+				"%u", feature_persistent);
+		if (err) {
+			xenbus_dev_fatal(dev, err,
+					"writing %s/feature-persistent",
+					dev->nodename);
+			goto abort;
+		}
 	}
 
 	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",
@@ -1093,8 +1103,12 @@ 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);
+	if (feature_persistent)
+		pers_grants = xenbus_read_unsigned(dev->otherend,
+				"feature-persistent", 0);
+	else
+		pers_grants = 0;
+
 	blkif->vbd.feature_gnt_persistent = pers_grants;
 	blkif->vbd.overflow_max_grants = 0;
 
-- 
2.17.1

Re: [PATCH v2 1/3] xen-blkback: add a parameter for disabling of persistent grants

[Roger Pau Monné]

On Tue, Sep 22, 2020 at 12:52:07PM +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 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            | 28 ++++++++++++++-----
>  2 files changed, 30 insertions(+), 7 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..8a95ddd08b13 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -879,6 +879,12 @@ static void reclaim_memory(struct xenbus_device *dev)
>  
>  /* ** Connection ** */
>  
> +/* 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");
> +
>  /*
>   * Write the physical details regarding the block device to the store, and
>   * switch to Connected state.
> @@ -906,11 +912,15 @@ 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);
> -	if (err) {
> -		xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
> -				 dev->nodename);
> -		goto abort;
> +	if (feature_persistent) {
> +		err = xenbus_printf(xbt, dev->nodename, "feature-persistent",
> +				"%u", feature_persistent);
> +		if (err) {
> +			xenbus_dev_fatal(dev, err,
> +					"writing %s/feature-persistent",
> +					dev->nodename);
> +			goto abort;
> +		}
>  	}
>  
>  	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",
> @@ -1093,8 +1103,12 @@ 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);
> +	if (feature_persistent)
> +		pers_grants = xenbus_read_unsigned(dev->otherend,
> +				"feature-persistent", 0);
> +	else
> +		pers_grants = 0;
> +

Sorry for not realizing earlier, but looking at it again I think you
need to cache the value of feature_persistent when it's first used in
the blkback state data, so that it's consistent.

What would happen for example with the following flow (assume a
persistent grants enabled frontend):

feature_persistent = false

connect(...)
feature-persistent is not written to xenstore

User changes feature_persistent = true

connect_ring(...)
pers_grants = true, because feature-persistent is set unconditionally
by the frontend and feature_persistent variable is now true.

Then blkback will try to use persistent grants and the whole
connection will malfunction because the frontend won't.

The other option is to prevent changing the variable when there are
blkback instances already running.

Thanks, Roger.

Re: [PATCH v2 1/3] xen-blkback: add a parameter for disabling of persistent grants

[SeongJae Park]

On Tue, 22 Sep 2020 13:12:59 +0200 "Roger Pau Monné" <roger.pau@citrix.com> wrote:

> On Tue, Sep 22, 2020 at 12:52:07PM +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 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            | 28 ++++++++++++++-----
> >  2 files changed, 30 insertions(+), 7 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..8a95ddd08b13 100644
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -879,6 +879,12 @@ static void reclaim_memory(struct xenbus_device *dev)
> >  
> >  /* ** Connection ** */
> >  
> > +/* 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");
> > +
> >  /*
> >   * Write the physical details regarding the block device to the store, and
> >   * switch to Connected state.
> > @@ -906,11 +912,15 @@ 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);
> > -	if (err) {
> > -		xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
> > -				 dev->nodename);
> > -		goto abort;
> > +	if (feature_persistent) {
> > +		err = xenbus_printf(xbt, dev->nodename, "feature-persistent",
> > +				"%u", feature_persistent);
> > +		if (err) {
> > +			xenbus_dev_fatal(dev, err,
> > +					"writing %s/feature-persistent",
> > +					dev->nodename);
> > +			goto abort;
> > +		}
> >  	}
> >  
> >  	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",
> > @@ -1093,8 +1103,12 @@ 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);
> > +	if (feature_persistent)
> > +		pers_grants = xenbus_read_unsigned(dev->otherend,
> > +				"feature-persistent", 0);
> > +	else
> > +		pers_grants = 0;
> > +
> 
> Sorry for not realizing earlier, but looking at it again I think you
> need to cache the value of feature_persistent when it's first used in
> the blkback state data, so that it's consistent.
> 
> What would happen for example with the following flow (assume a
> persistent grants enabled frontend):
> 
> feature_persistent = false
> 
> connect(...)
> feature-persistent is not written to xenstore
> 
> User changes feature_persistent = true
> 
> connect_ring(...)
> pers_grants = true, because feature-persistent is set unconditionally
> by the frontend and feature_persistent variable is now true.
> 
> Then blkback will try to use persistent grants and the whole
> connection will malfunction because the frontend won't.

Ah, you're right.  I should also catch this before but didn't, sorry.

> 
> The other option is to prevent changing the variable when there are
> blkback instances already running.

I think storing the option value in xenstore would be simpler.  That said, if
you prefer this way, please let me know.


Thanks,
SeongJae Park

Re: [PATCH v2 1/3] xen-blkback: add a parameter for disabling of persistent grants

[Roger Pau Monné]

On Tue, Sep 22, 2020 at 01:26:38PM +0200, SeongJae Park wrote:
> On Tue, 22 Sep 2020 13:12:59 +0200 "Roger Pau Monné" <roger.pau@citrix.com> wrote:
> 
> > On Tue, Sep 22, 2020 at 12:52:07PM +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 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            | 28 ++++++++++++++-----
> > >  2 files changed, 30 insertions(+), 7 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..8a95ddd08b13 100644
> > > --- a/drivers/block/xen-blkback/xenbus.c
> > > +++ b/drivers/block/xen-blkback/xenbus.c
> > > @@ -879,6 +879,12 @@ static void reclaim_memory(struct xenbus_device *dev)
> > >  
> > >  /* ** Connection ** */
> > >  
> > > +/* 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");
> > > +
> > >  /*
> > >   * Write the physical details regarding the block device to the store, and
> > >   * switch to Connected state.
> > > @@ -906,11 +912,15 @@ 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);
> > > -	if (err) {
> > > -		xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
> > > -				 dev->nodename);
> > > -		goto abort;
> > > +	if (feature_persistent) {
> > > +		err = xenbus_printf(xbt, dev->nodename, "feature-persistent",
> > > +				"%u", feature_persistent);
> > > +		if (err) {
> > > +			xenbus_dev_fatal(dev, err,
> > > +					"writing %s/feature-persistent",
> > > +					dev->nodename);
> > > +			goto abort;
> > > +		}
> > >  	}
> > >  
> > >  	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",
> > > @@ -1093,8 +1103,12 @@ 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);
> > > +	if (feature_persistent)
> > > +		pers_grants = xenbus_read_unsigned(dev->otherend,
> > > +				"feature-persistent", 0);
> > > +	else
> > > +		pers_grants = 0;
> > > +
> > 
> > Sorry for not realizing earlier, but looking at it again I think you
> > need to cache the value of feature_persistent when it's first used in
> > the blkback state data, so that it's consistent.
> > 
> > What would happen for example with the following flow (assume a
> > persistent grants enabled frontend):
> > 
> > feature_persistent = false
> > 
> > connect(...)
> > feature-persistent is not written to xenstore
> > 
> > User changes feature_persistent = true
> > 
> > connect_ring(...)
> > pers_grants = true, because feature-persistent is set unconditionally
> > by the frontend and feature_persistent variable is now true.
> > 
> > Then blkback will try to use persistent grants and the whole
> > connection will malfunction because the frontend won't.
> 
> Ah, you're right.  I should also catch this before but didn't, sorry.
> 
> > 
> > The other option is to prevent changing the variable when there are
> > blkback instances already running.
> 
> I think storing the option value in xenstore would be simpler.  That said, if
> you prefer this way, please let me know.

If possible I prefer to avoid reading from xenstore because it's slow,
very slow compared to storing this somewhere in the blkback
structure.

Could you use the feature_gnt_persistent field in xen_vbd maybe as a
place to cache the value before it's set with the final negotiated
value between the frontend and the backend?

Thanks, Roger.

Re: [PATCH v2 1/3] xen-blkback: add a parameter for disabling of persistent grants

[SeongJae Park]

On Tue, 22 Sep 2020 13:35:11 +0200 "Roger Pau Monné" <roger.pau@citrix.com> wrote:

> On Tue, Sep 22, 2020 at 01:26:38PM +0200, SeongJae Park wrote:
> > On Tue, 22 Sep 2020 13:12:59 +0200 "Roger Pau Monné" <roger.pau@citrix.com> wrote:
> > 
> > > On Tue, Sep 22, 2020 at 12:52:07PM +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 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            | 28 ++++++++++++++-----
> > > >  2 files changed, 30 insertions(+), 7 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..8a95ddd08b13 100644
> > > > --- a/drivers/block/xen-blkback/xenbus.c
> > > > +++ b/drivers/block/xen-blkback/xenbus.c
> > > > @@ -879,6 +879,12 @@ static void reclaim_memory(struct xenbus_device *dev)
> > > >  
> > > >  /* ** Connection ** */
> > > >  
> > > > +/* 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");
> > > > +
> > > >  /*
> > > >   * Write the physical details regarding the block device to the store, and
> > > >   * switch to Connected state.
> > > > @@ -906,11 +912,15 @@ 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);
> > > > -	if (err) {
> > > > -		xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
> > > > -				 dev->nodename);
> > > > -		goto abort;
> > > > +	if (feature_persistent) {
> > > > +		err = xenbus_printf(xbt, dev->nodename, "feature-persistent",
> > > > +				"%u", feature_persistent);
> > > > +		if (err) {
> > > > +			xenbus_dev_fatal(dev, err,
> > > > +					"writing %s/feature-persistent",
> > > > +					dev->nodename);
> > > > +			goto abort;
> > > > +		}
> > > >  	}
> > > >  
> > > >  	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",
> > > > @@ -1093,8 +1103,12 @@ 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);
> > > > +	if (feature_persistent)
> > > > +		pers_grants = xenbus_read_unsigned(dev->otherend,
> > > > +				"feature-persistent", 0);
> > > > +	else
> > > > +		pers_grants = 0;
> > > > +
> > > 
> > > Sorry for not realizing earlier, but looking at it again I think you
> > > need to cache the value of feature_persistent when it's first used in
> > > the blkback state data, so that it's consistent.
> > > 
> > > What would happen for example with the following flow (assume a
> > > persistent grants enabled frontend):
> > > 
> > > feature_persistent = false
> > > 
> > > connect(...)
> > > feature-persistent is not written to xenstore
> > > 
> > > User changes feature_persistent = true
> > > 
> > > connect_ring(...)
> > > pers_grants = true, because feature-persistent is set unconditionally
> > > by the frontend and feature_persistent variable is now true.
> > > 
> > > Then blkback will try to use persistent grants and the whole
> > > connection will malfunction because the frontend won't.
> > 
> > Ah, you're right.  I should also catch this before but didn't, sorry.
> > 
> > > 
> > > The other option is to prevent changing the variable when there are
> > > blkback instances already running.
> > 
> > I think storing the option value in xenstore would be simpler.  That said, if
> > you prefer this way, please let me know.
> 
> If possible I prefer to avoid reading from xenstore because it's slow,
> very slow compared to storing this somewhere in the blkback
> structure.

Agreed.

> 
> Could you use the feature_gnt_persistent field in xen_vbd maybe as a
> place to cache the value before it's set with the final negotiated
> value between the frontend and the backend?

Ok, I will do so in the next version.


Thanks,
SeongJae Park

Re: [PATCH v2 1/3] xen-blkback: add a parameter for disabling of persistent grants

[Jürgen Groß]

On 22.09.20 13:26, SeongJae Park wrote:
> On Tue, 22 Sep 2020 13:12:59 +0200 "Roger Pau Monné" <roger.pau@citrix.com> wrote:
> 
>> On Tue, Sep 22, 2020 at 12:52:07PM +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 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            | 28 ++++++++++++++-----
>>>   2 files changed, 30 insertions(+), 7 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..8a95ddd08b13 100644
>>> --- a/drivers/block/xen-blkback/xenbus.c
>>> +++ b/drivers/block/xen-blkback/xenbus.c
>>> @@ -879,6 +879,12 @@ static void reclaim_memory(struct xenbus_device *dev)
>>>   
>>>   /* ** Connection ** */
>>>   
>>> +/* 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");
>>> +
>>>   /*
>>>    * Write the physical details regarding the block device to the store, and
>>>    * switch to Connected state.
>>> @@ -906,11 +912,15 @@ 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);
>>> -	if (err) {
>>> -		xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
>>> -				 dev->nodename);
>>> -		goto abort;
>>> +	if (feature_persistent) {
>>> +		err = xenbus_printf(xbt, dev->nodename, "feature-persistent",
>>> +				"%u", feature_persistent);
>>> +		if (err) {
>>> +			xenbus_dev_fatal(dev, err,
>>> +					"writing %s/feature-persistent",
>>> +					dev->nodename);
>>> +			goto abort;
>>> +		}
>>>   	}
>>>   
>>>   	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",
>>> @@ -1093,8 +1103,12 @@ 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);
>>> +	if (feature_persistent)
>>> +		pers_grants = xenbus_read_unsigned(dev->otherend,
>>> +				"feature-persistent", 0);
>>> +	else
>>> +		pers_grants = 0;
>>> +
>>
>> Sorry for not realizing earlier, but looking at it again I think you
>> need to cache the value of feature_persistent when it's first used in
>> the blkback state data, so that it's consistent.
>>
>> What would happen for example with the following flow (assume a
>> persistent grants enabled frontend):
>>
>> feature_persistent = false
>>
>> connect(...)
>> feature-persistent is not written to xenstore
>>
>> User changes feature_persistent = true
>>
>> connect_ring(...)
>> pers_grants = true, because feature-persistent is set unconditionally
>> by the frontend and feature_persistent variable is now true.
>>
>> Then blkback will try to use persistent grants and the whole
>> connection will malfunction because the frontend won't.
> 
> Ah, you're right.  I should also catch this before but didn't, sorry.
> 
>>
>> The other option is to prevent changing the variable when there are
>> blkback instances already running.
> 
> I think storing the option value in xenstore would be simpler.  That said, if
> you prefer this way, please let me know.

No, Xenstore isn't the right place for that. This is a local
implementation detail of blkback and shouldn't be exported to Xenstore.


Juergen

Re: [PATCH v2 1/3] xen-blkback: add a parameter for disabling of persistent grants

[SeongJae Park]

On Tue, 22 Sep 2020 13:35:30 +0200 "Jürgen Groß" <jgross@suse.com> wrote:

> On 22.09.20 13:26, SeongJae Park wrote:
> > On Tue, 22 Sep 2020 13:12:59 +0200 "Roger Pau Monné" <roger.pau@citrix.com> wrote:
> > 
> >> On Tue, Sep 22, 2020 at 12:52:07PM +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 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            | 28 ++++++++++++++-----
> >>>   2 files changed, 30 insertions(+), 7 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..8a95ddd08b13 100644
> >>> --- a/drivers/block/xen-blkback/xenbus.c
> >>> +++ b/drivers/block/xen-blkback/xenbus.c
> >>> @@ -879,6 +879,12 @@ static void reclaim_memory(struct xenbus_device *dev)
> >>>   
> >>>   /* ** Connection ** */
> >>>   
> >>> +/* 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");
> >>> +
> >>>   /*
> >>>    * Write the physical details regarding the block device to the store, and
> >>>    * switch to Connected state.
> >>> @@ -906,11 +912,15 @@ 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);
> >>> -	if (err) {
> >>> -		xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
> >>> -				 dev->nodename);
> >>> -		goto abort;
> >>> +	if (feature_persistent) {
> >>> +		err = xenbus_printf(xbt, dev->nodename, "feature-persistent",
> >>> +				"%u", feature_persistent);
> >>> +		if (err) {
> >>> +			xenbus_dev_fatal(dev, err,
> >>> +					"writing %s/feature-persistent",
> >>> +					dev->nodename);
> >>> +			goto abort;
> >>> +		}
> >>>   	}
> >>>   
> >>>   	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",
> >>> @@ -1093,8 +1103,12 @@ 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);
> >>> +	if (feature_persistent)
> >>> +		pers_grants = xenbus_read_unsigned(dev->otherend,
> >>> +				"feature-persistent", 0);
> >>> +	else
> >>> +		pers_grants = 0;
> >>> +
> >>
> >> Sorry for not realizing earlier, but looking at it again I think you
> >> need to cache the value of feature_persistent when it's first used in
> >> the blkback state data, so that it's consistent.
> >>
> >> What would happen for example with the following flow (assume a
> >> persistent grants enabled frontend):
> >>
> >> feature_persistent = false
> >>
> >> connect(...)
> >> feature-persistent is not written to xenstore
> >>
> >> User changes feature_persistent = true
> >>
> >> connect_ring(...)
> >> pers_grants = true, because feature-persistent is set unconditionally
> >> by the frontend and feature_persistent variable is now true.
> >>
> >> Then blkback will try to use persistent grants and the whole
> >> connection will malfunction because the frontend won't.
> > 
> > Ah, you're right.  I should also catch this before but didn't, sorry.
> > 
> >>
> >> The other option is to prevent changing the variable when there are
> >> blkback instances already running.
> > 
> > I think storing the option value in xenstore would be simpler.  That said, if
> > you prefer this way, please let me know.
> 
> No, Xenstore isn't the right place for that. This is a local
> implementation detail of blkback and shouldn't be exported to Xenstore.

Agreed.  I will try using the 'feature_gnt_persistent' as Roger recommended.


Thanks,
SeongJae Park

[PATCH v2 2/3] xen-blkfront: add a parameter for disabling of persistent grants

[SeongJae Park]

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                  | 28 +++++++++++++------
 2 files changed, 29 insertions(+), 8 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..49c324f377de 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -149,6 +149,13 @@ static unsigned int xen_blkif_max_ring_order;
 module_param_named(max_ring_page_order, xen_blkif_max_ring_order, int, 0444);
 MODULE_PARM_DESC(max_ring_page_order, "Maximum order of pages to be used for the shared ring");
 
+/* 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");
+
+
 #define BLK_RING_SIZE(info)	\
 	__CONST_RING_SIZE(blkif, XEN_PAGE_SIZE * (info)->nr_ring_pages)
 
@@ -1866,11 +1873,13 @@ 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);
-	if (err)
-		dev_warn(&dev->dev,
-			 "writing persistent grants feature to xenbus");
+	if (feature_persistent) {
+		err = xenbus_printf(xbt, dev->nodename,
+				    "feature-persistent", "%u", 1);
+		if (err)
+			dev_warn(&dev->dev,
+				 "writing persistent grants feature to xenbus");
+	}
 
 	err = xenbus_transaction_end(xbt, 0);
 	if (err) {
@@ -2316,9 +2325,12 @@ 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 (feature_persistent)
+		info->feature_persistent =
+			!!xenbus_read_unsigned(info->xbdev->otherend,
+					       "feature-persistent", 0);
+	else
+		info->feature_persistent = 0;
 
 	indirect_segments = xenbus_read_unsigned(info->xbdev->otherend,
 					"feature-max-indirect-segments", 0);
-- 
2.17.1

Re: [PATCH v2 2/3] xen-blkfront: add a parameter for disabling of persistent grants

[Jürgen Groß]

On 22.09.20 12:52, 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                  | 28 +++++++++++++------
>   2 files changed, 29 insertions(+), 8 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..49c324f377de 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -149,6 +149,13 @@ static unsigned int xen_blkif_max_ring_order;
>   module_param_named(max_ring_page_order, xen_blkif_max_ring_order, int, 0444);
>   MODULE_PARM_DESC(max_ring_page_order, "Maximum order of pages to be used for the shared ring");
>   
> +/* 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");
> +
> +
>   #define BLK_RING_SIZE(info)	\
>   	__CONST_RING_SIZE(blkif, XEN_PAGE_SIZE * (info)->nr_ring_pages)
>   
> @@ -1866,11 +1873,13 @@ 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);
> -	if (err)
> -		dev_warn(&dev->dev,
> -			 "writing persistent grants feature to xenbus");
> +	if (feature_persistent) {
> +		err = xenbus_printf(xbt, dev->nodename,
> +				    "feature-persistent", "%u", 1);
> +		if (err)
> +			dev_warn(&dev->dev,
> +				 "writing persistent grants feature to xenbus");
> +	}
>   
>   	err = xenbus_transaction_end(xbt, 0);
>   	if (err) {
> @@ -2316,9 +2325,12 @@ 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 (feature_persistent)
> +		info->feature_persistent =
> +			!!xenbus_read_unsigned(info->xbdev->otherend,
> +					       "feature-persistent", 0);
> +	else
> +		info->feature_persistent = 0;
>   
>   	indirect_segments = xenbus_read_unsigned(info->xbdev->otherend,
>   					"feature-max-indirect-segments", 0);
> 

Here you have the same problem as in blkback: feature_persistent could
change its value between the two tests.


Juergen

Re: [PATCH v2 2/3] xen-blkfront: add a parameter for disabling of persistent grants

[SeongJae Park]

On Tue, 22 Sep 2020 14:11:32 +0200 "Jürgen Groß" <jgross@suse.com> wrote:

> On 22.09.20 12:52, 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                  | 28 +++++++++++++------
> >   2 files changed, 29 insertions(+), 8 deletions(-)
> > 
[...]
> > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > index 91de2e0755ae..49c324f377de 100644
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -149,6 +149,13 @@ static unsigned int xen_blkif_max_ring_order;
> >   module_param_named(max_ring_page_order, xen_blkif_max_ring_order, int, 0444);
> >   MODULE_PARM_DESC(max_ring_page_order, "Maximum order of pages to be used for the shared ring");
> >   
> > +/* 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");
> > +
> > +
> >   #define BLK_RING_SIZE(info)	\
> >   	__CONST_RING_SIZE(blkif, XEN_PAGE_SIZE * (info)->nr_ring_pages)
> >   
> > @@ -1866,11 +1873,13 @@ 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);
> > -	if (err)
> > -		dev_warn(&dev->dev,
> > -			 "writing persistent grants feature to xenbus");
> > +	if (feature_persistent) {
> > +		err = xenbus_printf(xbt, dev->nodename,
> > +				    "feature-persistent", "%u", 1);
> > +		if (err)
> > +			dev_warn(&dev->dev,
> > +				 "writing persistent grants feature to xenbus");
> > +	}
> >   
> >   	err = xenbus_transaction_end(xbt, 0);
> >   	if (err) {
> > @@ -2316,9 +2325,12 @@ 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 (feature_persistent)
> > +		info->feature_persistent =
> > +			!!xenbus_read_unsigned(info->xbdev->otherend,
> > +					       "feature-persistent", 0);
> > +	else
> > +		info->feature_persistent = 0;
> >   
> >   	indirect_segments = xenbus_read_unsigned(info->xbdev->otherend,
> >   					"feature-max-indirect-segments", 0);
> > 
> 
> Here you have the same problem as in blkback: feature_persistent could
> change its value between the two tests.

Yes, indeed.  I will fix this in the next version.


Thanks,
SeongJae Park

[PATCH v2 3/3] xen-blkfront: Apply changed parameter name to the document

[SeongJae Park]

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