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

[Pratyush Yadav <ptyadav@xxxxxxxxx>]

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

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