Messages in this thread | | | Date | Fri, 2 Sep 2022 13:08:37 +0200 | Subject | Re: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested | From | Juergen Gross <> |
| |
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 [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |