Hi Pratyush,
On Wed, 31 Aug 2022 15:50:45 +0000 Pratyush Yadav ptyadav@amazon.de wrote:
On 25/08/22 04:15PM, SeongJae Park wrote:
Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter when connect") made blkback to advertise its support of the persistent grants feature only if the user sets the 'feature_persistent' parameter of the driver and the frontend advertised its support of the feature. However, following commit 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter when connect") made the blkfront to work in the same way. That is, blkfront also advertises its support of the persistent grants feature only if the user sets the 'feature_persistent' parameter of the driver and the backend advertised its support of the feature.
Hence blkback and blkfront will never advertise their support of the feature but wait until the other advertises the support, even though users set the 'feature_persistent' parameters of the drivers. As a result, the persistent grants feature is disabled always regardless of the 'feature_persistent' values[1].
The problem comes from the misuse of the semantic of the advertisement of the feature. The advertisement of the feature should means only availability of the feature not the decision for using the feature. However, current behavior is working in the wrong way.
This commit fixes the issue by making the blkfront advertises its support of the feature as user requested via 'feature_persistent' parameter regardless of the otherend's support of the feature.
[1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse....
Fixes: 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter when connect") Cc: stable@vger.kernel.org # 5.10.x Reported-by: Marek Marczykowski-Górecki marmarek@invisiblethingslab.com Suggested-by: Juergen Gross jgross@suse.com Signed-off-by: SeongJae Park sj@kernel.org
drivers/block/xen-blkfront.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index 8e56e69fb4c4..dfae08115450 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -213,6 +213,9 @@ struct blkfront_info unsigned int feature_fua:1; unsigned int feature_discard:1; unsigned int feature_secdiscard:1;
- /* Connect-time cached feature_persistent parameter */
- unsigned int feature_persistent_parm:1;
- /* Persistent grants feature negotiation result */ unsigned int feature_persistent:1; unsigned int bounce:1; unsigned int discard_granularity;
@@ -1848,7 +1851,7 @@ static int talk_to_blkback(struct xenbus_device *dev, goto abort_transaction; } err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
info->feature_persistent);
info->feature_persistent_parm);@@ -2281,7 +2284,8 @@ static void blkfront_gather_backend_features(struct blkfront_info *info) if (xenbus_read_unsigned(info->xbdev->otherend, "feature-discard", 0)) blkfront_setup_discard(info);
- if (feature_persistent)
- info->feature_persistent_parm = feature_persistent;
Same question as before. Why not just use feature_persistent directly?
Same answer as before, due to the possible race[1].
[1] https://lore.kernel.org/linux-block/20200922111259.GJ19254@Air-de-Roger/
- if (info->feature_persistent_parm) info->feature_persistent = !!xenbus_read_unsigned(info->xbdev->otherend, "feature-persistent", 0);
Aside: IMO this would look nicer as below:
info->feature_persistent = feature_persistent && !!xenbus_read_unsigned();
Agreed, that would also make the code more consistent with the blkback side code.
I would make the change in the next version of this patchset.
Thanks, SJ