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