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