[PATCH 0/2] xen-blk{front,back}: Advertise feature-persistent as user requested

[PATCH 0/2] xen-blk{front,back}: Advertise feature-persistent as user requested

[SeongJae Park]

Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
when connect") made blkback to advertise its support of the persistent
grants feature only if the user sets the 'feature_persistent' parameter
of the driver and the frontend advertised its support of the feature.
However, following commit 402c43ea6b34 ("xen-blkfront: Apply
'feature_persistent' parameter when connect") made the blkfront to work
in the same way.  That is, blkfront also advertises its support of the
persistent grants feature only if the user sets the 'feature_persistent'
parameter of the driver and the backend advertised its support of the
feature.

Hence blkback and blkfront will never advertise their support of the
feature but wait until the other advertises the support, even though
users set the 'feature_persistent' parameters of the drivers.  As a
result, the persistent grants feature is disabled always regardless of
the 'feature_persistent' values[1].

The problem comes from the misuse of the semantic of the advertisement
of the feature.  The advertisement of the feature should means only
availability of the feature not the decision for using the feature.
However, current behavior is working in the wrong way.

This patchset fixes the issue by making both blkback and blkfront
advertise their support of the feature as user requested via
'feature_persistent' parameter regardless of the otherend's support of
the feature.

[1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/

SeongJae Park (2):
  xen-blkback: Advertise feature-persistent as user requested
  xen-blkfront: Advertise feature-persistent as user requested

 drivers/block/xen-blkback/common.h | 3 +++
 drivers/block/xen-blkback/xenbus.c | 6 ++++--
 drivers/block/xen-blkfront.c       | 8 ++++++--
 3 files changed, 13 insertions(+), 4 deletions(-)

-- 
2.25.1

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

[SeongJae Park]

Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
when connect") made blkback to advertise its support of the persistent
grants feature only if the user sets the 'feature_persistent' parameter
of the driver and the frontend advertised its support of the feature.
However, following commit 402c43ea6b34 ("xen-blkfront: Apply
'feature_persistent' parameter when connect") made the blkfront to work
in the same way.  That is, blkfront also advertises its support of the
persistent grants feature only if the user sets the 'feature_persistent'
parameter of the driver and the backend advertised its support of the
feature.

Hence blkback and blkfront will never advertise their support of the
feature but wait until the other advertises the support, even though
users set the 'feature_persistent' parameters of the drivers.  As a
result, the persistent grants feature is disabled always regardless of
the 'feature_persistent' values[1].

The problem comes from the misuse of the semantic of the advertisement
of the feature.  The advertisement of the feature should means only
availability of the feature not the decision for using the feature.
However, current behavior is working in the wrong way.

This commit fixes the issue by making the blkback advertises its support
of the feature as user requested via 'feature_persistent' parameter
regardless of the otherend's support of the feature.

[1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/

Fixes: e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter when connect")
Cc: <stable@vger.kernel.org> # 5.10.x
Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
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 2/2] xen-blkfront: Advertise feature-persistent as user requested

[SeongJae Park]

Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
when connect") made blkback to advertise its support of the persistent
grants feature only if the user sets the 'feature_persistent' parameter
of the driver and the frontend advertised its support of the feature.
However, following commit 402c43ea6b34 ("xen-blkfront: Apply
'feature_persistent' parameter when connect") made the blkfront to work
in the same way.  That is, blkfront also advertises its support of the
persistent grants feature only if the user sets the 'feature_persistent'
parameter of the driver and the backend advertised its support of the
feature.

Hence blkback and blkfront will never advertise their support of the
feature but wait until the other advertises the support, even though
users set the 'feature_persistent' parameters of the drivers.  As a
result, the persistent grants feature is disabled always regardless of
the 'feature_persistent' values[1].

The problem comes from the misuse of the semantic of the advertisement
of the feature.  The advertisement of the feature should means only
availability of the feature not the decision for using the feature.
However, current behavior is working in the wrong way.

This commit fixes the issue by making the blkfront advertises its
support of the feature as user requested via 'feature_persistent'
parameter regardless of the otherend's support of the feature.

[1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/

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

Re: [PATCH 0/2] xen-blk{front,back}: Advertise feature-persistent as user requested

[Juergen Gross]

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

On 25.08.22 18:15, SeongJae Park wrote:
> Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> when connect") made blkback to advertise its support of the persistent
> grants feature only if the user sets the 'feature_persistent' parameter
> of the driver and the frontend advertised its support of the feature.
> However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> 'feature_persistent' parameter when connect") made the blkfront to work
> in the same way.  That is, blkfront also advertises its support of the
> persistent grants feature only if the user sets the 'feature_persistent'
> parameter of the driver and the backend advertised its support of the
> feature.
> 
> Hence blkback and blkfront will never advertise their support of the
> feature but wait until the other advertises the support, even though
> users set the 'feature_persistent' parameters of the drivers.  As a
> result, the persistent grants feature is disabled always regardless of
> the 'feature_persistent' values[1].
> 
> The problem comes from the misuse of the semantic of the advertisement
> of the feature.  The advertisement of the feature should means only
> availability of the feature not the decision for using the feature.
> However, current behavior is working in the wrong way.
> 
> This patchset fixes the issue by making both blkback and blkfront
> advertise their support of the feature as user requested via
> 'feature_persistent' parameter regardless of the otherend's support of
> the feature.
> 
> [1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/
> 
> SeongJae Park (2):
>    xen-blkback: Advertise feature-persistent as user requested
>    xen-blkfront: Advertise feature-persistent as user requested
> 
>   drivers/block/xen-blkback/common.h | 3 +++
>   drivers/block/xen-blkback/xenbus.c | 6 ++++--
>   drivers/block/xen-blkfront.c       | 8 ++++++--
>   3 files changed, 13 insertions(+), 4 deletions(-)
> 

For the 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 2/2] xen-blkfront: Advertise feature-persistent as user requested

[Maximilian Heyne]

On Thu, Aug 25, 2022 at 04:15:11PM +0000, SeongJae Park wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> when connect") made blkback to advertise its support of the persistent
> grants feature only if the user sets the 'feature_persistent' parameter
> of the driver and the frontend advertised its support of the feature.
> However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> 'feature_persistent' parameter when connect") made the blkfront to work
> in the same way.  That is, blkfront also advertises its support of the
> persistent grants feature only if the user sets the 'feature_persistent'
> parameter of the driver and the backend advertised its support of the
> feature.
> 
> Hence blkback and blkfront will never advertise their support of the
> feature but wait until the other advertises the support, even though
> users set the 'feature_persistent' parameters of the drivers.  As a
> result, the persistent grants feature is disabled always regardless of
> the 'feature_persistent' values[1].
> 
> The problem comes from the misuse of the semantic of the advertisement
> of the feature.  The advertisement of the feature should means only
> availability of the feature not the decision for using the feature.
> However, current behavior is working in the wrong way.
> 
> This commit fixes the issue by making the blkfront advertises its
> support of the feature as user requested via 'feature_persistent'
> parameter regardless of the otherend's support of the feature.
> 
> [1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/
> 
> 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>
> 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;

I think setting this here is too late because "feature-persistent" was already
written to xenstore via talk_to_blkback but with default 0. So during the
connect blkback will not see that the guest supports the feature and falls back
to no persistent grants.

Tested only this patch with some hacky dom0 kernel that doesn't have the patch
from your series yet. Will do more testing next week.

> +       if (info->feature_persistent_parm)
>                 info->feature_persistent =
>                         !!xenbus_read_unsigned(info->xbdev->otherend,
>                                                "feature-persistent", 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 2/2] xen-blkfront: Advertise feature-persistent as user requested

[SeongJae Park]

Hi Max,

On Fri, 26 Aug 2022 14:26:58 +0000 Maximilian Heyne <mheyne@amazon.de> wrote:

> On Thu, Aug 25, 2022 at 04:15:11PM +0000, SeongJae Park wrote:
> > 
> > Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> > when connect") made blkback to advertise its support of the persistent
> > grants feature only if the user sets the 'feature_persistent' parameter
> > of the driver and the frontend advertised its support of the feature.
> > However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> > 'feature_persistent' parameter when connect") made the blkfront to work
> > in the same way.  That is, blkfront also advertises its support of the
> > persistent grants feature only if the user sets the 'feature_persistent'
> > parameter of the driver and the backend advertised its support of the
> > feature.
> > 
> > Hence blkback and blkfront will never advertise their support of the
> > feature but wait until the other advertises the support, even though
> > users set the 'feature_persistent' parameters of the drivers.  As a
> > result, the persistent grants feature is disabled always regardless of
> > the 'feature_persistent' values[1].
> > 
> > The problem comes from the misuse of the semantic of the advertisement
> > of the feature.  The advertisement of the feature should means only
> > availability of the feature not the decision for using the feature.
> > However, current behavior is working in the wrong way.
> > 
> > This commit fixes the issue by making the blkfront advertises its
> > support of the feature as user requested via 'feature_persistent'
> > parameter regardless of the otherend's support of the feature.
> > 
> > [1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/
> > 
> > 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>
> > 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;
> 
> I think setting this here is too late because "feature-persistent" was already
> written to xenstore via talk_to_blkback but with default 0. So during the
> connect blkback will not see that the guest supports the feature and falls back
> to no persistent grants.
> 
> Tested only this patch with some hacky dom0 kernel that doesn't have the patch
> from your series yet. Will do more testing next week.

Appreciate for your test!  And you're right, this patch is not fixing the issue
completely.  That is, commit 402c43ea6b34 ("xen-blkfront: Apply
'feature_persistent' parameter when connect") introduced two bugs.  One is the
misuse of the semantic of the advertisement.  It's fixed by this patch.  The
second bug, which you found here, is caching the parameter in a wrong place.

In detail, blkfront does the advertisement before connect (for init and resume)
and then negotiation after connected.  And the blkback does the negotiation
first, and then the advertisement during the establishing the connection.
Hence, blkback should cache the parameter just before the negotiation logic
while blkfront should do that just before the advertisement logic.

The blkback behavior change commit (e94c6101e151) did the work in the right
place, but the blkfront behavior change commit didn't.

So, I guess below change would fix the issue entirely when applied together
with this patch.  Any opinion, please?


diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index dfae08115450..7d3bde271e69 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1850,6 +1850,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)
@@ -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,


Thanks,
SJ


> 
> > +       if (info->feature_persistent_parm)
> >                 info->feature_persistent =
> >                         !!xenbus_read_unsigned(info->xbdev->otherend,
> >                                                "feature-persistent", 0);
> > --
> > 2.25.1
> > 
> 

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

[SeongJae Park]

On Fri, 26 Aug 2022 21:20:39 +0000 SeongJae Park <sj@kernel.org> wrote:

> Hi Max,
> 
> On Fri, 26 Aug 2022 14:26:58 +0000 Maximilian Heyne <mheyne@amazon.de> wrote:
> 
> > On Thu, Aug 25, 2022 at 04:15:11PM +0000, SeongJae Park wrote:
> > > 
> > > Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> > > when connect") made blkback to advertise its support of the persistent
> > > grants feature only if the user sets the 'feature_persistent' parameter
> > > of the driver and the frontend advertised its support of the feature.
> > > However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> > > 'feature_persistent' parameter when connect") made the blkfront to work
> > > in the same way.  That is, blkfront also advertises its support of the
> > > persistent grants feature only if the user sets the 'feature_persistent'
> > > parameter of the driver and the backend advertised its support of the
> > > feature.
> > > 
> > > Hence blkback and blkfront will never advertise their support of the
> > > feature but wait until the other advertises the support, even though
> > > users set the 'feature_persistent' parameters of the drivers.  As a
> > > result, the persistent grants feature is disabled always regardless of
> > > the 'feature_persistent' values[1].
> > > 
> > > The problem comes from the misuse of the semantic of the advertisement
> > > of the feature.  The advertisement of the feature should means only
> > > availability of the feature not the decision for using the feature.
> > > However, current behavior is working in the wrong way.
> > > 
> > > This commit fixes the issue by making the blkfront advertises its
> > > support of the feature as user requested via 'feature_persistent'
> > > parameter regardless of the otherend's support of the feature.
> > > 
> > > [1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/
> > > 
> > > 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>
> > > 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;
> > 
> > I think setting this here is too late because "feature-persistent" was already
> > written to xenstore via talk_to_blkback but with default 0. So during the
> > connect blkback will not see that the guest supports the feature and falls back
> > to no persistent grants.
> > 
> > Tested only this patch with some hacky dom0 kernel that doesn't have the patch
> > from your series yet. Will do more testing next week.
> 
> Appreciate for your test!  And you're right, this patch is not fixing the issue
> completely.  That is, commit 402c43ea6b34 ("xen-blkfront: Apply
> 'feature_persistent' parameter when connect") introduced two bugs.  One is the
> misuse of the semantic of the advertisement.  It's fixed by this patch.  The
> second bug, which you found here, is caching the parameter in a wrong place.

To be fair, I think we should say the misuse of the semantic issue came from
the initial commit[1] that uses only one field for the two different
information (availability of the feature and decision to use the feature), and
the mis-placed caching has introduced by the behavior change commit[2].

[1] 74a852479c68 ("xen-blkfront: add a parameter for disabling of persistent grants")
[2] 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter when connect")


Thanks,
SJ

> 
> In detail, blkfront does the advertisement before connect (for init and resume)
> and then negotiation after connected.  And the blkback does the negotiation
> first, and then the advertisement during the establishing the connection.
> Hence, blkback should cache the parameter just before the negotiation logic
> while blkfront should do that just before the advertisement logic.
> 
> The blkback behavior change commit (e94c6101e151) did the work in the right
> place, but the blkfront behavior change commit didn't.
> 
> So, I guess below change would fix the issue entirely when applied together
> with this patch.  Any opinion, please?
> 
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index dfae08115450..7d3bde271e69 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1850,6 +1850,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)
> @@ -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,
> 
> 
> Thanks,
> SJ
> 
> 
> > 
> > > +       if (info->feature_persistent_parm)
> > >                 info->feature_persistent =
> > >                         !!xenbus_read_unsigned(info->xbdev->otherend,
> > >                                                "feature-persistent", 0);
> > > --
> > > 2.25.1
> > > 
> > 

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

[SeongJae Park]

On Fri, 26 Aug 2022 21:20:39 +0000 SeongJae Park <sj@kernel.org> wrote:

> Hi Max,
> 
> On Fri, 26 Aug 2022 14:26:58 +0000 Maximilian Heyne <mheyne@amazon.de> wrote:
> 
> > On Thu, Aug 25, 2022 at 04:15:11PM +0000, SeongJae Park wrote:
> > > 
> > > Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> > > when connect") made blkback to advertise its support of the persistent
> > > grants feature only if the user sets the 'feature_persistent' parameter
> > > of the driver and the frontend advertised its support of the feature.
> > > However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> > > 'feature_persistent' parameter when connect") made the blkfront to work
> > > in the same way.  That is, blkfront also advertises its support of the
> > > persistent grants feature only if the user sets the 'feature_persistent'
> > > parameter of the driver and the backend advertised its support of the
> > > feature.
> > > 
> > > Hence blkback and blkfront will never advertise their support of the
> > > feature but wait until the other advertises the support, even though
> > > users set the 'feature_persistent' parameters of the drivers.  As a
> > > result, the persistent grants feature is disabled always regardless of
> > > the 'feature_persistent' values[1].
> > > 
> > > The problem comes from the misuse of the semantic of the advertisement
> > > of the feature.  The advertisement of the feature should means only
> > > availability of the feature not the decision for using the feature.
> > > However, current behavior is working in the wrong way.
> > > 
> > > This commit fixes the issue by making the blkfront advertises its
> > > support of the feature as user requested via 'feature_persistent'
> > > parameter regardless of the otherend's support of the feature.
> > > 
> > > [1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/
> > > 
> > > 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>
> > > 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;
> > 
> > I think setting this here is too late because "feature-persistent" was already
> > written to xenstore via talk_to_blkback but with default 0. So during the
> > connect blkback will not see that the guest supports the feature and falls back
> > to no persistent grants.
> > 
> > Tested only this patch with some hacky dom0 kernel that doesn't have the patch
> > from your series yet. Will do more testing next week.
> 
> Appreciate for your test!  And you're right, this patch is not fixing the issue
> completely.  That is, commit 402c43ea6b34 ("xen-blkfront: Apply
> 'feature_persistent' parameter when connect") introduced two bugs.  One is the
> misuse of the semantic of the advertisement.  It's fixed by this patch.  The
> second bug, which you found here, is caching the parameter in a wrong place.
> 
> In detail, blkfront does the advertisement before connect (for init and resume)
> and then negotiation after connected.  And the blkback does the negotiation
> first, and then the advertisement during the establishing the connection.
> Hence, blkback should cache the parameter just before the negotiation logic
> while blkfront should do that just before the advertisement logic.
> 
> The blkback behavior change commit (e94c6101e151) did the work in the right
> place, but the blkfront behavior change commit didn't.
> 
> So, I guess below change would fix the issue entirely when applied together
> with this patch.  Any opinion, please?
> 
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index dfae08115450..7d3bde271e69 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1850,6 +1850,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)
> @@ -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,

FWIW, 'feature_persistent' variable definition should be moved to above of
'talk_to_blkback()'.  Otherwise, build would fail.


Thanks,
SJ

> 
> 
> Thanks,
> SJ
> 
> 
> > 
> > > +       if (info->feature_persistent_parm)
> > >                 info->feature_persistent =
> > >                         !!xenbus_read_unsigned(info->xbdev->otherend,
> > >                                                "feature-persistent", 0);
> > > --
> > > 2.25.1
> > > 
> > 

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

[Pratyush Yadav]

Hi,

On 25/08/22 04:15PM, SeongJae Park wrote:
> Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> when connect") made blkback to advertise its support of the persistent
> grants feature only if the user sets the 'feature_persistent' parameter
> of the driver and the frontend advertised its support of the feature.
> However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> 'feature_persistent' parameter when connect") made the blkfront to work
> in the same way.  That is, blkfront also advertises its support of the
> persistent grants feature only if the user sets the 'feature_persistent'
> parameter of the driver and the backend advertised its support of the
> feature.
> 
> Hence blkback and blkfront will never advertise their support of the
> feature but wait until the other advertises the support, even though
> users set the 'feature_persistent' parameters of the drivers.  As a
> result, the persistent grants feature is disabled always regardless of
> the 'feature_persistent' values[1].
> 
> The problem comes from the misuse of the semantic of the advertisement
> of the feature.  The advertisement of the feature should means only
> availability of the feature not the decision for using the feature.
> However, current behavior is working in the wrong way.
> 
> This commit fixes the issue by making the blkback advertises its support
> of the feature as user requested via 'feature_persistent' parameter
> regardless of the otherend's support of the feature.
> 
> [1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/
> 
> Fixes: e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter when connect")
> Cc: <stable@vger.kernel.org> # 5.10.x
> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> 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;

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.

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

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

[Pratyush Yadav]

On 25/08/22 04:15PM, SeongJae Park wrote:
> Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> when connect") made blkback to advertise its support of the persistent
> grants feature only if the user sets the 'feature_persistent' parameter
> of the driver and the frontend advertised its support of the feature.
> However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> 'feature_persistent' parameter when connect") made the blkfront to work
> in the same way.  That is, blkfront also advertises its support of the
> persistent grants feature only if the user sets the 'feature_persistent'
> parameter of the driver and the backend advertised its support of the
> feature.
> 
> Hence blkback and blkfront will never advertise their support of the
> feature but wait until the other advertises the support, even though
> users set the 'feature_persistent' parameters of the drivers.  As a
> result, the persistent grants feature is disabled always regardless of
> the 'feature_persistent' values[1].
> 
> The problem comes from the misuse of the semantic of the advertisement
> of the feature.  The advertisement of the feature should means only
> availability of the feature not the decision for using the feature.
> However, current behavior is working in the wrong way.
> 
> This commit fixes the issue by making the blkfront advertises its
> support of the feature as user requested via 'feature_persistent'
> parameter regardless of the otherend's support of the feature.
> 
> [1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/
> 
> 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>
> 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;

Same question as before. Why not just use feature_persistent directly?

> +	if (info->feature_persistent_parm)
>  		info->feature_persistent =
>  			!!xenbus_read_unsigned(info->xbdev->otherend,
>  					       "feature-persistent", 0);

Aside: IMO this would look nicer as below:

	info->feature_persistent = feature_persistent && !!xenbus_read_unsigned();

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

[SeongJae Park]

Hi Pratyush,

On Wed, 31 Aug 2022 15:47:50 +0000 Pratyush Yadav <ptyadav@amazon.de> wrote:

> Hi,
> 
> On 25/08/22 04:15PM, SeongJae Park wrote:
> > Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> > when connect") made blkback to advertise its support of the persistent
> > grants feature only if the user sets the 'feature_persistent' parameter
> > of the driver and the frontend advertised its support of the feature.
> > However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> > 'feature_persistent' parameter when connect") made the blkfront to work
> > in the same way.  That is, blkfront also advertises its support of the
> > persistent grants feature only if the user sets the 'feature_persistent'
> > parameter of the driver and the backend advertised its support of the
> > feature.
> > 
> > Hence blkback and blkfront will never advertise their support of the
> > feature but wait until the other advertises the support, even though
> > users set the 'feature_persistent' parameters of the drivers.  As a
> > result, the persistent grants feature is disabled always regardless of
> > the 'feature_persistent' values[1].
> > 
> > The problem comes from the misuse of the semantic of the advertisement
> > of the feature.  The advertisement of the feature should means only
> > availability of the feature not the decision for using the feature.
> > However, current behavior is working in the wrong way.
> > 
> > This commit fixes the issue by making the blkback advertises its support
> > of the feature as user requested via 'feature_persistent' parameter
> > regardless of the otherend's support of the feature.
> > 
> > [1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/
> > 
> > Fixes: e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter when connect")
> > Cc: <stable@vger.kernel.org> # 5.10.x
> > Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > 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;
> 
> 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/


Thanks,
SJ

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

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

[SeongJae Park]

Hi Pratyush,

On Wed, 31 Aug 2022 15:50:45 +0000 Pratyush Yadav <ptyadav@amazon.de> wrote:

> On 25/08/22 04:15PM, SeongJae Park wrote:
> > Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> > when connect") made blkback to advertise its support of the persistent
> > grants feature only if the user sets the 'feature_persistent' parameter
> > of the driver and the frontend advertised its support of the feature.
> > However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> > 'feature_persistent' parameter when connect") made the blkfront to work
> > in the same way.  That is, blkfront also advertises its support of the
> > persistent grants feature only if the user sets the 'feature_persistent'
> > parameter of the driver and the backend advertised its support of the
> > feature.
> > 
> > Hence blkback and blkfront will never advertise their support of the
> > feature but wait until the other advertises the support, even though
> > users set the 'feature_persistent' parameters of the drivers.  As a
> > result, the persistent grants feature is disabled always regardless of
> > the 'feature_persistent' values[1].
> > 
> > The problem comes from the misuse of the semantic of the advertisement
> > of the feature.  The advertisement of the feature should means only
> > availability of the feature not the decision for using the feature.
> > However, current behavior is working in the wrong way.
> > 
> > This commit fixes the issue by making the blkfront advertises its
> > support of the feature as user requested via 'feature_persistent'
> > parameter regardless of the otherend's support of the feature.
> > 
> > [1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/
> > 
> > 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>
> > 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;
> 
> Same question as before. Why not just use feature_persistent directly?

Same answer as before, due to the possible race[1].

[1] https://lore.kernel.org/linux-block/20200922111259.GJ19254@Air-de-Roger/

> 
> > +	if (info->feature_persistent_parm)
> >  		info->feature_persistent =
> >  			!!xenbus_read_unsigned(info->xbdev->otherend,
> >  					       "feature-persistent", 0);
> 
> Aside: IMO this would look nicer as below:
> 
> 	info->feature_persistent = feature_persistent && !!xenbus_read_unsigned();

Agreed, that would also make the code more consistent with the blkback side
code.

I would make the change in the next version of this patchset.


Thanks,
SJ