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@vger.kernel.org # 5.10.x Suggested-by: Juergen Gross jgross@suse.com Signed-off-by: SeongJae Park sj@kernel.org --- 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;
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@vger.kernel.org # 5.10.x Suggested-by: Juergen Gross jgross@suse.com Signed-off-by: SeongJae Park sj@kernel.org
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
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@vger.kernel.org # 5.10.x Suggested-by: Juergen Gross jgross@suse.com Signed-off-by: SeongJae Park sj@kernel.org
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: stable@vger.kernel.org # 5.10.x Suggested-by: Juergen Gross jgross@suse.com Signed-off-by: SeongJae Park sj@kernel.org
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@amazon.de
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@vger.kernel.org # 5.10.x Suggested-by: Juergen Gross jgross@suse.com Signed-off-by: SeongJae Park sj@kernel.org
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
linux-stable-mirror@lists.linaro.org
-
Juergen Gross -
Maximilian Heyne -
Pratyush Yadav -
SeongJae Park