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: <[email protected]> # 5.10.x
Suggested-by: Juergen Gross <[email protected]>
Signed-off-by: SeongJae Park <[email protected]>
---
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
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: <[email protected]> # 5.10.x
> Suggested-by: Juergen Gross <[email protected]>
> Signed-off-by: SeongJae Park <[email protected]>
> ---
> 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.
> + /* 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
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: <[email protected]> # 5.10.x
> > Suggested-by: Juergen Gross <[email protected]>
> > Signed-off-by: SeongJae Park <[email protected]>
> > ---
> > 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
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: <[email protected]> # 5.10.x
>> Suggested-by: Juergen Gross <[email protected]>
>> Signed-off-by: SeongJae Park <[email protected]>
>> ---
>> 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).
Juergen
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: <[email protected]> # 5.10.x
> > > Suggested-by: Juergen Gross <[email protected]>
> > > Signed-off-by: SeongJae Park <[email protected]>
> > > ---
> > > 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 <[email protected]>
--
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