Re: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx> # 5.10.x
Suggested-by: Juergen Gross <jgross@xxxxxxxx>
Signed-off-by: SeongJae Park <sj@xxxxxxxxxx>
---
  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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux