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

[Maximilian Heyne <mheyne@xxxxxxxxx>]

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@xxxxxxxxxxxxxxx> # 5.10.x
> > Suggested-by: Juergen Gross <jgross@xxxxxxxx>
> > Signed-off-by: SeongJae Park <sj@xxxxxxxxxx>
> > ---
> >  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