[PATCH v2 0/3] xen-blk{front,back}: Fix the broken semantic and flow of feature-persistent

[PATCH v2 0/3] xen-blk{front,back}: Fix the broken semantic and flow of feature-persistent

[SeongJae Park]

Changes from v1
(https://lore.kernel.org/xen-devel/20220825161511.94922-1-sj@kernel.org/)
- Fix the wrong feature_persistent caching position of blkfront
- Set blkfront's feature_persistent field setting with simple '&&'
  instead of 'if' (Pratyush Yadav)

This patchset fixes misuse of the 'feature-persistent' advertisement
semantic (patches 1 and 2), and the wrong timing of the
'feature_persistent' value caching, which made persistent grants feature
always disabled.

SeongJae Park (3):
  xen-blkback: Advertise feature-persistent as user requested
  xen-blkfront: Advertise feature-persistent as user requested
  xen-blkfront: Cache feature_persistent value before advertisement

 drivers/block/xen-blkback/common.h |  3 +++
 drivers/block/xen-blkback/xenbus.c |  6 ++++--
 drivers/block/xen-blkfront.c       | 20 ++++++++++++--------
 3 files changed, 19 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested

[SeongJae Park]

The advertisement of the persistent grants feature (writing
'feature-persistent' to xenbus) should mean not the decision for using
the feature but only the availability of the feature.  However, commit
aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent
grants") made a field of blkback, which was a place for saving only the
negotiation result, to be used for yet another purpose: caching of the
'feature_persistent' parameter value.  As a result, the advertisement,
which should follow only the parameter value, becomes inconsistent.

This commit fixes the misuse of the semantic by making blkback saves the
parameter value in a separate place and advertises the support based on
only the saved value.

Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent grants")
Cc: <stable@vger.kernel.org> # 5.10.x
Suggested-by: Juergen Gross <jgross@suse.com>
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 drivers/block/xen-blkback/common.h | 3 +++
 drivers/block/xen-blkback/xenbus.c | 6 ++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
index bda5c815e441..a28473470e66 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -226,6 +226,9 @@ struct xen_vbd {
 	sector_t		size;
 	unsigned int		flush_support:1;
 	unsigned int		discard_secure:1;
+	/* Connect-time cached feature_persistent parameter value */
+	unsigned int		feature_gnt_persistent_parm:1;
+	/* Persistent grants feature negotiation result */
 	unsigned int		feature_gnt_persistent:1;
 	unsigned int		overflow_max_grants:1;
 };
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index ee7ad2fb432d..c0227dfa4688 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -907,7 +907,7 @@ 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",
-			be->blkif->vbd.feature_gnt_persistent);
+			be->blkif->vbd.feature_gnt_persistent_parm);
 	if (err) {
 		xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
 				 dev->nodename);
@@ -1085,7 +1085,9 @@ static int connect_ring(struct backend_info *be)
 		return -ENOSYS;
 	}
 
-	blkif->vbd.feature_gnt_persistent = feature_persistent &&
+	blkif->vbd.feature_gnt_persistent_parm = feature_persistent;
+	blkif->vbd.feature_gnt_persistent =
+		blkif->vbd.feature_gnt_persistent_parm &&
 		xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
 
 	blkif->vbd.overflow_max_grants = 0;
-- 
2.25.1

[PATCH v2 2/3] xen-blkfront: Advertise feature-persistent as user requested

[SeongJae Park]

The advertisement of the persistent grants feature (writing
'feature-persistent' to xenbus) should mean not the decision for using
the feature but only the availability of the feature.  However, commit
74a852479c68 ("xen-blkfront: add a parameter for disabling of persistent
grants") made a field of blkfront, which was a place for saving only the
negotiation result, to be used for yet another purpose: caching of the
'feature_persistent' parameter value.  As a result, the advertisement,
which should follow only the parameter value, becomes inconsistent.

This commit fixes the misuse of the semantic by making blkfront saves
the parameter value in a separate place and advertises the support based
on only the saved value.

Fixes: 74a852479c68 ("xen-blkfront: add a parameter for disabling of persistent grants")
Cc: <stable@vger.kernel.org> # 5.10.x
Suggested-by: Juergen Gross <jgross@suse.com>
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 drivers/block/xen-blkfront.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 8e56e69fb4c4..dfae08115450 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -213,6 +213,9 @@ struct blkfront_info
 	unsigned int feature_fua:1;
 	unsigned int feature_discard:1;
 	unsigned int feature_secdiscard:1;
+	/* Connect-time cached feature_persistent parameter */
+	unsigned int feature_persistent_parm:1;
+	/* Persistent grants feature negotiation result */
 	unsigned int feature_persistent:1;
 	unsigned int bounce:1;
 	unsigned int discard_granularity;
@@ -1848,7 +1851,7 @@ static int talk_to_blkback(struct xenbus_device *dev,
 		goto abort_transaction;
 	}
 	err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
-			info->feature_persistent);
+			info->feature_persistent_parm);
 	if (err)
 		dev_warn(&dev->dev,
 			 "writing persistent grants feature to xenbus");
@@ -2281,7 +2284,8 @@ static void blkfront_gather_backend_features(struct blkfront_info *info)
 	if (xenbus_read_unsigned(info->xbdev->otherend, "feature-discard", 0))
 		blkfront_setup_discard(info);
 
-	if (feature_persistent)
+	info->feature_persistent_parm = feature_persistent;
+	if (info->feature_persistent_parm)
 		info->feature_persistent =
 			!!xenbus_read_unsigned(info->xbdev->otherend,
 					       "feature-persistent", 0);
-- 
2.25.1

[PATCH v2 3/3] xen-blkfront: Cache feature_persistent value before advertisement

[SeongJae Park]

Xen blkfront advertises its support of the persistent grants feature
when it first setting up and when resuming in 'talk_to_blkback()'.
Then, blkback reads the advertised value when it connects with blkfront
and decides if it will use the persistent grants feature or not, and
advertises its decision to blkfront.  Blkfront reads the blkback's
decision and it also makes the decision for the use of the feature.

Commit 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter
when connect"), however, made the blkfront's read of the parameter for
disabling the advertisement, namely 'feature_persistent', to be done
when it negotiate, not when advertise.  Therefore blkfront advertises
without reading the parameter.  As the field for caching the parameter
value is zero-initialized, it always advertises as the feature is
disabled, so that the persistent grants feature becomes always disabled.

This commit fixes the issue by making the blkfront does parmeter caching
just before the advertisement.

Fixes: 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter when connect")
Cc: <stable@vger.kernel.org> # 5.10.x
Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 drivers/block/xen-blkfront.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index dfae08115450..35b9bcad9db9 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1759,6 +1759,12 @@ static int write_per_ring_nodes(struct xenbus_transaction xbt,
 	return err;
 }
 
+/* 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");
+
 /* Common code used when first setting up, and when resuming. */
 static int talk_to_blkback(struct xenbus_device *dev,
 			   struct blkfront_info *info)
@@ -1850,6 +1856,7 @@ static int talk_to_blkback(struct xenbus_device *dev,
 		message = "writing protocol";
 		goto abort_transaction;
 	}
+	info->feature_persistent_parm = feature_persistent;
 	err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
 			info->feature_persistent_parm);
 	if (err)
@@ -1919,12 +1926,6 @@ 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
@@ -2284,7 +2285,6 @@ 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_parm = feature_persistent;
 	if (info->feature_persistent_parm)
 		info->feature_persistent =
 			!!xenbus_read_unsigned(info->xbdev->otherend,
-- 
2.25.1

Re: [PATCH v2 0/3] xen-blk{front,back}: Fix the broken semantic and flow of feature-persistent

[SeongJae Park]

On Wed, 31 Aug 2022 16:58:21 +0000 SeongJae Park <sj@kernel.org> wrote:

> Changes from v1
> (https://lore.kernel.org/xen-devel/20220825161511.94922-1-sj@kernel.org/)
> - Fix the wrong feature_persistent caching position of blkfront
> - Set blkfront's feature_persistent field setting with simple '&&'
>   instead of 'if' (Pratyush Yadav)
> 
> This patchset fixes misuse of the 'feature-persistent' advertisement
> semantic (patches 1 and 2), and the wrong timing of the
> 'feature_persistent' value caching, which made persistent grants feature
> always disabled.

Please note that I have some problem in my test setup and therefore was unable
to fully test this patchset.  I am posting this though, as the impact of the
bug is not trivial (always disabling persistent grants), and to make testing of
my proposed fix from others easier.  Hope to get someone's test results or code
review of this patchset even before I fix my test setup problem.

Juergen, I didn't add your 'Reviewed-by:'s to the first two patches of this
series because I changed some of the description for making it clear which bug
and commit it is really fixing.  Specifically, I wordsmithed the working and
changed 'Fixed:' tag.  Code change is almost same, though.


Thanks,
SJ

> 
> SeongJae Park (3):
>   xen-blkback: Advertise feature-persistent as user requested
>   xen-blkfront: Advertise feature-persistent as user requested
>   xen-blkfront: Cache feature_persistent value before advertisement
> 
>  drivers/block/xen-blkback/common.h |  3 +++
>  drivers/block/xen-blkback/xenbus.c |  6 ++++--
>  drivers/block/xen-blkfront.c       | 20 ++++++++++++--------
>  3 files changed, 19 insertions(+), 10 deletions(-)
> 
> -- 
> 2.25.1
> 

Re: [PATCH v2 0/3] xen-blk{front,back}: Fix the broken semantic and flow of feature-persistent

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

On Wed, Aug 31, 2022 at 05:08:17PM +0000, SeongJae Park wrote:
> On Wed, 31 Aug 2022 16:58:21 +0000 SeongJae Park <sj@kernel.org> wrote:
> 
> > Changes from v1
> > (https://lore.kernel.org/xen-devel/20220825161511.94922-1-sj@kernel.org/)
> > - Fix the wrong feature_persistent caching position of blkfront
> > - Set blkfront's feature_persistent field setting with simple '&&'
> >   instead of 'if' (Pratyush Yadav)
> > 
> > This patchset fixes misuse of the 'feature-persistent' advertisement
> > semantic (patches 1 and 2), and the wrong timing of the
> > 'feature_persistent' value caching, which made persistent grants feature
> > always disabled.
> 
> Please note that I have some problem in my test setup and therefore was unable
> to fully test this patchset.  I am posting this though, as the impact of the
> bug is not trivial (always disabling persistent grants), and to make testing of
> my proposed fix from others easier.  Hope to get someone's test results or code
> review of this patchset even before I fix my test setup problem.

I can confirm it fixes the issue, thanks!

Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> Juergen, I didn't add your 'Reviewed-by:'s to the first two patches of this
> series because I changed some of the description for making it clear which bug
> and commit it is really fixing.  Specifically, I wordsmithed the working and
> changed 'Fixed:' tag.  Code change is almost same, though.
> 
> 
> Thanks,
> SJ
> 
> > 
> > SeongJae Park (3):
> >   xen-blkback: Advertise feature-persistent as user requested
> >   xen-blkfront: Advertise feature-persistent as user requested
> >   xen-blkfront: Cache feature_persistent value before advertisement
> > 
> >  drivers/block/xen-blkback/common.h |  3 +++
> >  drivers/block/xen-blkback/xenbus.c |  6 ++++--
> >  drivers/block/xen-blkfront.c       | 20 ++++++++++++--------
> >  3 files changed, 19 insertions(+), 10 deletions(-)
> > 
> > -- 
> > 2.25.1
> > 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/3] xen-blk{front,back}: Fix the broken semantic and flow of feature-persistent

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1092 bytes --]

On 31.08.22 18:58, SeongJae Park wrote:
> Changes from v1
> (https://lore.kernel.org/xen-devel/20220825161511.94922-1-sj@kernel.org/)
> - Fix the wrong feature_persistent caching position of blkfront
> - Set blkfront's feature_persistent field setting with simple '&&'
>    instead of 'if' (Pratyush Yadav)
> 
> This patchset fixes misuse of the 'feature-persistent' advertisement
> semantic (patches 1 and 2), and the wrong timing of the
> 'feature_persistent' value caching, which made persistent grants feature
> always disabled.
> 
> SeongJae Park (3):
>    xen-blkback: Advertise feature-persistent as user requested
>    xen-blkfront: Advertise feature-persistent as user requested
>    xen-blkfront: Cache feature_persistent value before advertisement
> 
>   drivers/block/xen-blkback/common.h |  3 +++
>   drivers/block/xen-blkback/xenbus.c |  6 ++++--
>   drivers/block/xen-blkfront.c       | 20 ++++++++++++--------
>   3 files changed, 19 insertions(+), 10 deletions(-)
> 

For the whole series:

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


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested

[Pratyush Yadav]

On 31/08/22 04:58PM, SeongJae Park wrote:
> The advertisement of the persistent grants feature (writing
> 'feature-persistent' to xenbus) should mean not the decision for using
> the feature but only the availability of the feature.  However, commit
> aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent
> grants") made a field of blkback, which was a place for saving only the
> negotiation result, to be used for yet another purpose: caching of the
> 'feature_persistent' parameter value.  As a result, the advertisement,
> which should follow only the parameter value, becomes inconsistent.
> 
> This commit fixes the misuse of the semantic by making blkback saves the
> parameter value in a separate place and advertises the support based on
> only the saved value.
> 
> Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent grants")
> Cc: <stable@vger.kernel.org> # 5.10.x
> Suggested-by: Juergen Gross <jgross@suse.com>
> Signed-off-by: SeongJae Park <sj@kernel.org>
> ---
>  drivers/block/xen-blkback/common.h | 3 +++
>  drivers/block/xen-blkback/xenbus.c | 6 ++++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
> index bda5c815e441..a28473470e66 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -226,6 +226,9 @@ struct xen_vbd {
>         sector_t                size;
>         unsigned int            flush_support:1;
>         unsigned int            discard_secure:1;
> +       /* Connect-time cached feature_persistent parameter value */
> +       unsigned int            feature_gnt_persistent_parm:1;

Continuing over from the previous version:

> > If feature_gnt_persistent_parm is always going to be equal to 
> > feature_persistent, then why introduce it at all? Why not just use 
> > feature_persistent directly? This way you avoid adding an extra flag 
> > whose purpose is not immediately clear, and you also avoid all the 
> > mess with setting this flag at the right time.
>
> Mainly because the parameter should read twice (once for 
> advertisement, and once later just before the negotitation, for 
> checking if we advertised or not), and the user might change the 
> parameter value between the two reads.
>
> For the detailed available sequence of the race, you could refer to the 
> prior conversation[1].
>
> [1] https://lore.kernel.org/linux-block/20200922111259.GJ19254@Air-de-Roger/

Okay, I see. Thanks for the pointer. But still, I think it would be 
better to not maintain two copies of the value. How about doing:

	blkif->vbd.feature_gnt_persistent =
		xenbus_read_unsigned(dev->nodename, "feature-persistent", 0) &&
		xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);

This makes it quite clear that we only enable persistent grants if 
_both_ ends support it. We can do the same for blkfront.

> +       /* Persistent grants feature negotiation result */
>         unsigned int            feature_gnt_persistent:1;
>         unsigned int            overflow_max_grants:1;
>  };
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index ee7ad2fb432d..c0227dfa4688 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -907,7 +907,7 @@ 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",
> -                       be->blkif->vbd.feature_gnt_persistent);
> +                       be->blkif->vbd.feature_gnt_persistent_parm);
>         if (err) {
>                 xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
>                                  dev->nodename);
> @@ -1085,7 +1085,9 @@ static int connect_ring(struct backend_info *be)
>                 return -ENOSYS;
>         }
> 
> -       blkif->vbd.feature_gnt_persistent = feature_persistent &&
> +       blkif->vbd.feature_gnt_persistent_parm = feature_persistent;
> +       blkif->vbd.feature_gnt_persistent =
> +               blkif->vbd.feature_gnt_persistent_parm &&
>                 xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
> 
>         blkif->vbd.overflow_max_grants = 0;
> --
> 2.25.1
> 

-- 
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v2 0/3] xen-blk{front, back}: Fix the broken semantic and flow of feature-persistent

[Maximilian Heyne]

On Wed, Aug 31, 2022 at 04:58:21PM +0000, SeongJae Park wrote:
> Changes from v1
> (https://lore.kernel.org/xen-devel/20220825161511.94922-1-sj@kernel.org/)
> - Fix the wrong feature_persistent caching position of blkfront
> - Set blkfront's feature_persistent field setting with simple '&&'
>   instead of 'if' (Pratyush Yadav)
> 
> This patchset fixes misuse of the 'feature-persistent' advertisement
> semantic (patches 1 and 2), and the wrong timing of the
> 'feature_persistent' value caching, which made persistent grants feature
> always disabled.
> 
> SeongJae Park (3):
>   xen-blkback: Advertise feature-persistent as user requested
>   xen-blkfront: Advertise feature-persistent as user requested
>   xen-blkfront: Cache feature_persistent value before advertisement
> 
>  drivers/block/xen-blkback/common.h |  3 +++
>  drivers/block/xen-blkback/xenbus.c |  6 ++++--
>  drivers/block/xen-blkfront.c       | 20 ++++++++++++--------
>  3 files changed, 19 insertions(+), 10 deletions(-)
> 
> --
> 2.25.1
> 

I've tested this patch series in the following ways:
* Only applied the blkback patch but not the blkfront patches
* Only applied the blkfront patches but not the blkback patch
* Applied both

All scenarios worked, so

Tested-by: Maximilian Heyne <mheyne@amazon.de>

Actually I also wanted to test changing feature_persistent and try reconnecting
but I don't know how this is done. If anyone has a pointer here, I could test
that as well.



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3241 bytes --]

On 02.09.22 11:53, Pratyush Yadav wrote:
> On 31/08/22 04:58PM, SeongJae Park wrote:
>> The advertisement of the persistent grants feature (writing
>> 'feature-persistent' to xenbus) should mean not the decision for using
>> the feature but only the availability of the feature.  However, commit
>> aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent
>> grants") made a field of blkback, which was a place for saving only the
>> negotiation result, to be used for yet another purpose: caching of the
>> 'feature_persistent' parameter value.  As a result, the advertisement,
>> which should follow only the parameter value, becomes inconsistent.
>>
>> This commit fixes the misuse of the semantic by making blkback saves the
>> parameter value in a separate place and advertises the support based on
>> only the saved value.
>>
>> Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent grants")
>> Cc: <stable@vger.kernel.org> # 5.10.x
>> Suggested-by: Juergen Gross <jgross@suse.com>
>> Signed-off-by: SeongJae Park <sj@kernel.org>
>> ---
>>   drivers/block/xen-blkback/common.h | 3 +++
>>   drivers/block/xen-blkback/xenbus.c | 6 ++++--
>>   2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
>> index bda5c815e441..a28473470e66 100644
>> --- a/drivers/block/xen-blkback/common.h
>> +++ b/drivers/block/xen-blkback/common.h
>> @@ -226,6 +226,9 @@ struct xen_vbd {
>>          sector_t                size;
>>          unsigned int            flush_support:1;
>>          unsigned int            discard_secure:1;
>> +       /* Connect-time cached feature_persistent parameter value */
>> +       unsigned int            feature_gnt_persistent_parm:1;
> 
> Continuing over from the previous version:
> 
>>> If feature_gnt_persistent_parm is always going to be equal to
>>> feature_persistent, then why introduce it at all? Why not just use
>>> feature_persistent directly? This way you avoid adding an extra flag
>>> whose purpose is not immediately clear, and you also avoid all the
>>> mess with setting this flag at the right time.
>>
>> Mainly because the parameter should read twice (once for
>> advertisement, and once later just before the negotitation, for
>> checking if we advertised or not), and the user might change the
>> parameter value between the two reads.
>>
>> For the detailed available sequence of the race, you could refer to the
>> prior conversation[1].
>>
>> [1] https://lore.kernel.org/linux-block/20200922111259.GJ19254@Air-de-Roger/
> 
> Okay, I see. Thanks for the pointer. But still, I think it would be
> better to not maintain two copies of the value. How about doing:
> 
> 	blkif->vbd.feature_gnt_persistent =
> 		xenbus_read_unsigned(dev->nodename, "feature-persistent", 0) &&
> 		xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
> 
> This makes it quite clear that we only enable persistent grants if
> _both_ ends support it. We can do the same for blkfront.

I prefer it as is, as it will not rely on nobody having modified the
Xenstore node (which would in theory be possible).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested

[Maximilian Heyne]

On Fri, Sep 02, 2022 at 09:53:22AM +0000, Pratyush Yadav wrote:
> On 31/08/22 04:58PM, SeongJae Park wrote:
> > The advertisement of the persistent grants feature (writing
> > 'feature-persistent' to xenbus) should mean not the decision for using
> > the feature but only the availability of the feature.  However, commit
> > aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent
> > grants") made a field of blkback, which was a place for saving only the
> > negotiation result, to be used for yet another purpose: caching of the
> > 'feature_persistent' parameter value.  As a result, the advertisement,
> > which should follow only the parameter value, becomes inconsistent.
> > 
> > This commit fixes the misuse of the semantic by making blkback saves the
> > parameter value in a separate place and advertises the support based on
> > only the saved value.
> > 
> > Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent grants")
> > Cc: <stable@vger.kernel.org> # 5.10.x
> > Suggested-by: Juergen Gross <jgross@suse.com>
> > Signed-off-by: SeongJae Park <sj@kernel.org>
> > ---
> >  drivers/block/xen-blkback/common.h | 3 +++
> >  drivers/block/xen-blkback/xenbus.c | 6 ++++--
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
> > index bda5c815e441..a28473470e66 100644
> > --- a/drivers/block/xen-blkback/common.h
> > +++ b/drivers/block/xen-blkback/common.h
> > @@ -226,6 +226,9 @@ struct xen_vbd {
> >         sector_t                size;
> >         unsigned int            flush_support:1;
> >         unsigned int            discard_secure:1;
> > +       /* Connect-time cached feature_persistent parameter value */
> > +       unsigned int            feature_gnt_persistent_parm:1;
> 
> Continuing over from the previous version:
> 
> > > If feature_gnt_persistent_parm is always going to be equal to 
> > > feature_persistent, then why introduce it at all? Why not just use 
> > > feature_persistent directly? This way you avoid adding an extra flag 
> > > whose purpose is not immediately clear, and you also avoid all the 
> > > mess with setting this flag at the right time.
> >
> > Mainly because the parameter should read twice (once for 
> > advertisement, and once later just before the negotitation, for 
> > checking if we advertised or not), and the user might change the 
> > parameter value between the two reads.
> >
> > For the detailed available sequence of the race, you could refer to the 
> > prior conversation[1].
> >
> > [1] https://lore.kernel.org/linux-block/20200922111259.GJ19254@Air-de-Roger/
> 
> Okay, I see. Thanks for the pointer. But still, I think it would be 
> better to not maintain two copies of the value. How about doing:
> 
> 	blkif->vbd.feature_gnt_persistent =
> 		xenbus_read_unsigned(dev->nodename, "feature-persistent", 0) &&
> 		xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
> 
> This makes it quite clear that we only enable persistent grants if 
> _both_ ends support it. We can do the same for blkfront.

Currently, the feature-persistent xenstore entry is written to from connect()
which is called after connect_ring(). So it's not available like this.  Perhaps
it's possible to delay the decision whether to use persistent grants until
connect().

> 
> > +       /* Persistent grants feature negotiation result */
> >         unsigned int            feature_gnt_persistent:1;
> >         unsigned int            overflow_max_grants:1;
> >  };
> > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> > index ee7ad2fb432d..c0227dfa4688 100644
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -907,7 +907,7 @@ 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",
> > -                       be->blkif->vbd.feature_gnt_persistent);
> > +                       be->blkif->vbd.feature_gnt_persistent_parm);
> >         if (err) {
> >                 xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
> >                                  dev->nodename);
> > @@ -1085,7 +1085,9 @@ static int connect_ring(struct backend_info *be)
> >                 return -ENOSYS;
> >         }
> > 
> > -       blkif->vbd.feature_gnt_persistent = feature_persistent &&
> > +       blkif->vbd.feature_gnt_persistent_parm = feature_persistent;
> > +       blkif->vbd.feature_gnt_persistent =
> > +               blkif->vbd.feature_gnt_persistent_parm &&
> >                 xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
> > 
> >         blkif->vbd.overflow_max_grants = 0;
> > --
> > 2.25.1
> > 
> 
> -- 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested

[Pratyush Yadav]

On 02/09/22 01:08PM, Juergen Gross wrote:
> On 02.09.22 11:53, Pratyush Yadav wrote:
> > On 31/08/22 04:58PM, SeongJae Park wrote:
> > > The advertisement of the persistent grants feature (writing
> > > 'feature-persistent' to xenbus) should mean not the decision for using
> > > the feature but only the availability of the feature.  However, commit
> > > aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent
> > > grants") made a field of blkback, which was a place for saving only the
> > > negotiation result, to be used for yet another purpose: caching of the
> > > 'feature_persistent' parameter value.  As a result, the advertisement,
> > > which should follow only the parameter value, becomes inconsistent.
> > > 
> > > This commit fixes the misuse of the semantic by making blkback saves the
> > > parameter value in a separate place and advertises the support based on
> > > only the saved value.
> > > 
> > > Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent grants")
> > > Cc: <stable@vger.kernel.org> # 5.10.x
> > > Suggested-by: Juergen Gross <jgross@suse.com>
> > > Signed-off-by: SeongJae Park <sj@kernel.org>
> > > ---
> > >   drivers/block/xen-blkback/common.h | 3 +++
> > >   drivers/block/xen-blkback/xenbus.c | 6 ++++--
> > >   2 files changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
> > > index bda5c815e441..a28473470e66 100644
> > > --- a/drivers/block/xen-blkback/common.h
> > > +++ b/drivers/block/xen-blkback/common.h
> > > @@ -226,6 +226,9 @@ struct xen_vbd {
> > >          sector_t                size;
> > >          unsigned int            flush_support:1;
> > >          unsigned int            discard_secure:1;
> > > +       /* Connect-time cached feature_persistent parameter value */
> > > +       unsigned int            feature_gnt_persistent_parm:1;
> > 
> > Continuing over from the previous version:
> > 
> > > > If feature_gnt_persistent_parm is always going to be equal to
> > > > feature_persistent, then why introduce it at all? Why not just use
> > > > feature_persistent directly? This way you avoid adding an extra flag
> > > > whose purpose is not immediately clear, and you also avoid all the
> > > > mess with setting this flag at the right time.
> > > 
> > > Mainly because the parameter should read twice (once for
> > > advertisement, and once later just before the negotitation, for
> > > checking if we advertised or not), and the user might change the
> > > parameter value between the two reads.
> > > 
> > > For the detailed available sequence of the race, you could refer to the
> > > prior conversation[1].
> > > 
> > > [1] https://lore.kernel.org/linux-block/20200922111259.GJ19254@Air-de-Roger/
> > 
> > Okay, I see. Thanks for the pointer. But still, I think it would be
> > better to not maintain two copies of the value. How about doing:
> > 
> > 	blkif->vbd.feature_gnt_persistent =
> > 		xenbus_read_unsigned(dev->nodename, "feature-persistent", 0) &&
> > 		xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
> > 
> > This makes it quite clear that we only enable persistent grants if
> > _both_ ends support it. We can do the same for blkfront.
> 
> I prefer it as is, as it will not rely on nobody having modified the
> Xenstore node (which would in theory be possible).

Okay. In that case,

Reviewed-by: Pratyush Yadav <ptyadav@amazon.de>

-- 
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879