Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
From: "Roger Pau Monné" <roger.pau@citrix.com>
To: SeongJae Park <sjpark@amazon.com>
Cc: <konrad.wilk@oracle.com>, <jgross@suse.com>,
	SeongJae Park <sjpark@amazon.de>, <axboe@kernel.dk>,
	<aliguori@amazon.com>, <amit@kernel.org>, <mheyne@amazon.de>,
	<pdurrant@amazon.co.uk>, <linux-block@vger.kernel.org>,
	<xen-devel@lists.xenproject.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/3] xen-blkback: add a parameter for disabling of persistent grants
Date: Tue, 22 Sep 2020 13:12:59 +0200
Message-ID: <20200922111259.GJ19254@Air-de-Roger> (raw)
In-Reply-To: <20200922105209.5284-2-sjpark@amazon.com>

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.

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-22 10:52 [PATCH v2 0/3] xen-blk(back|front): Let users disable " SeongJae Park
2020-09-22 10:52 ` [PATCH v2 1/3] xen-blkback: add a parameter for disabling of " SeongJae Park
2020-09-22 11:12   ` Roger Pau Monné [this message]
2020-09-22 11:26     ` SeongJae Park
2020-09-22 11:35       ` Roger Pau Monné
2020-09-22 12:07         ` SeongJae Park
2020-09-22 11:35       ` Jürgen Groß
2020-09-22 12:08         ` SeongJae Park
2020-09-22 10:52 ` [PATCH v2 2/3] xen-blkfront: " SeongJae Park
2020-09-22 12:11   ` Jürgen Groß
2020-09-22 12:44     ` SeongJae Park
2020-09-22 10:52 ` [PATCH v2 3/3] xen-blkfront: Apply changed parameter name to the document SeongJae Park

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200922111259.GJ19254@Air-de-Roger \
    --to=roger.pau@citrix.com \
    --cc=aliguori@amazon.com \
    --cc=amit@kernel.org \
    --cc=axboe@kernel.dk \
    --cc=jgross@suse.com \
    --cc=konrad.wilk@oracle.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mheyne@amazon.de \
    --cc=pdurrant@amazon.co.uk \
    --cc=sjpark@amazon.com \
    --cc=sjpark@amazon.de \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git