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@xxxxxxxxxx> wrote:

> Hi Max,
> 
> On Fri, 26 Aug 2022 14:26:58 +0000 Maximilian Heyne <mheyne@xxxxxxxxx> 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@xxxxxxxx/
> > > 
> > > Fixes: 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter when connect")
> > > Cc: <stable@xxxxxxxxxxxxxxx> # 5.10.x
> > > Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > > Suggested-by: Juergen Gross <jgross@xxxxxxxx>
> > > Signed-off-by: SeongJae Park <sj@xxxxxxxxxx>
> > > ---
> > >  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
> > > 
> >