xen-blk{front,back}: Fix the broken semantic and flow of feature-persistent
Subsystems (4)
v2 cover xen-blk{front,back}: Fix the broken semantic and flow of feature-persistent
Changes from v1
(https://lore.kernel.org/xen-devel/[email protected]/)
- Fix the wrong feature_persistent caching position of blkfront
- Set blkfront's feature_persistent field setting with simple '&&' instead of 'if' (Pratyush Yadav)
This patchset fixes misuse of the 'feature-persistent' advertisement semantic (patches 1 and 2), and the wrong timing of the 'feature_persistent' value caching, which made persistent grants feature always disabled.
SeongJae Park (3):
xen-blkback: Advertise feature-persistent as user requested
xen-blkfront: Advertise feature-persistent as user requested
xen-blkfront: Cache feature_persistent value before advertisement
drivers/block/xen-blkback/common.h | 3 +++ drivers/block/xen-blkback/xenbus.c | 6 ++++-- drivers/block/xen-blkfront.c | 20 ++++++++++++-------- 3 files changed, 19 insertions(+), 10 deletions(-)
Signature · 1 line
2.25.1
PATCH v2 1/3 xen-blkback: Advertise feature-persistent as user requested
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.
--- drivers/block/xen-blkback/common.h | 3 +++ drivers/block/xen-blkback/xenbus.c | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-)
drivers/block/xen-blkback/common.h +3 -0
@@ -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; };
drivers/block/xen-blkback/xenbus.c +4 -2
@@ -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 ;
Signature · 1 line
2.25.1
Re: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested
Quoted 27 lines · SeongJae Park
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 <[email protected]> # 5.10.xSuggested-by Juergen Gross <[email protected]>Signed-off-by SeongJae Park <[email protected]>--- drivers/block/xen-blkback/common.h | 3 +++ drivers/block/xen-blkback/xenbus.c | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-)drivers/block/xen-blkback/common.h +0
-0
drivers/block/xen-blkback/common.h struct xen_vbd { · line 230
Quoted 5 lines
drivers/block/xen-blkback/common.h +2
-0@@ -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:
Quoted 11 lines
Quoted 5 lines
If
feature_gnt_persistent_parmis always going to be equal tofeature_persistent, then why introduce it at all? Why not just usefeature_persistentdirectly? 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.
Quoted 4 lines
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.
Quoted 31 lines
drivers/block/xen-blkback/common.h +1
-0+/* Persistent grants feature negotiation result */ unsigned int feature_gnt_persistent:1; unsigned int overflow_max_grants:1; };drivers/block/xen-blkback/xenbus.c +4
-3@@ -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
Signature · 7 lines
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
Juergen Gross
maintainerRe: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested
Quoted 55 lines · Pratyush Yadav
Quoted 27 lines · SeongJae Park
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 <[email protected]> # 5.10.xSuggested-by Juergen Gross <[email protected]>Signed-off-by SeongJae Park <[email protected]>--- drivers/block/xen-blkback/common.h | 3 +++ drivers/block/xen-blkback/xenbus.c | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-)drivers/block/xen-blkback/common.h +0
-0drivers/block/xen-blkback/common.h struct xen_vbd { · line 230
Quoted 5 lines
drivers/block/xen-blkback/common.h +2
-0@@ -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:
Quoted 11 lines
Quoted 5 lines
If
feature_gnt_persistent_parmis always going to be equal tofeature_persistent, then why introduce it at all? Why not just usefeature_persistentdirectly? 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.
Quoted 4 lines
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:
Quoted 6 lines
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
Pratyush Yadav
Reviewed-byRe: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested
Quoted 60 lines · Juergen Gross
Quoted 59 lines · Pratyush Yadav
Quoted 27 lines · SeongJae Park
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 <[email protected]> # 5.10.xSuggested-by Juergen Gross <[email protected]>Signed-off-by SeongJae Park <[email protected]>--- drivers/block/xen-blkback/common.h | 3 +++ drivers/block/xen-blkback/xenbus.c | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-)drivers/block/xen-blkback/common.h +0
-0drivers/block/xen-blkback/common.h struct xen_vbd { · line 230
Quoted 5 lines
drivers/block/xen-blkback/common.h +2
-0@@ -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:
Quoted 11 lines
Quoted 5 lines
If
feature_gnt_persistent_parmis always going to be equal tofeature_persistent, then why introduce it at all? Why not just usefeature_persistentdirectly? 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.
Quoted 4 lines
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);
Quoted 5 lines
Quoted 2 lines
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,
Signature · 7 lines
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
Re: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested
In reply to Pratyush YadavQuoted 55 lines · Pratyush Yadav
Quoted 27 lines · SeongJae Park
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 <[email protected]> # 5.10.xSuggested-by Juergen Gross <[email protected]>Signed-off-by SeongJae Park <[email protected]>--- drivers/block/xen-blkback/common.h | 3 +++ drivers/block/xen-blkback/xenbus.c | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-)drivers/block/xen-blkback/common.h +0
-0drivers/block/xen-blkback/common.h struct xen_vbd { · line 230
Quoted 5 lines
drivers/block/xen-blkback/common.h +2
-0@@ -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:
Quoted 11 lines
Quoted 5 lines
If
feature_gnt_persistent_parmis always going to be equal tofeature_persistent, then why introduce it at all? Why not just usefeature_persistentdirectly? 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.
Quoted 4 lines
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:
Quoted 6 lines
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().
Quoted 28 lines
Quoted 28 lines
drivers/block/xen-blkback/common.h +1
-0+/* Persistent grants feature negotiation result */ unsigned int feature_gnt_persistent:1; unsigned int overflow_max_grants:1; };drivers/block/xen-blkback/xenbus.c +4
-2@@ -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 );
Quoted 13 lines
Quoted 4 lines
drivers/block/xen-blkback/xenbus.c +0
-1blkif->vbd.overflow_max_grants =0 ; --2.25.1
Signature · 7 lines
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
PATCH v2 2/3 xen-blkfront: Advertise feature-persistent as user requested
In reply to SeongJae ParkThe 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 74a852479c68 ("xen-blkfront: add a parameter for disabling of persistent grants") made a field of blkfront, 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 blkfront saves the parameter value in a separate place and advertises the support based on only the saved value.
--- drivers/block/xen-blkfront.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/block/xen-blkfront.c +6 -2
@@ -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);if (err)dev_warn (&dev->dev,"writing persistent grants feature to xenbus" );
@@ -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; +if (info->feature_persistent_parm) info->feature_persistent = !!xenbus_read_unsigned (info->xbdev->otherend,"feature-persistent" ,0 );
Signature · 1 line
2.25.1
PATCH v2 3/3 xen-blkfront: Cache feature_persistent value before advertisement
In reply to SeongJae ParkXen blkfront advertises its support of the persistent grants feature when it first setting up and when resuming in 'talk_to_blkback()'. Then, blkback reads the advertised value when it connects with blkfront and decides if it will use the persistent grants feature or not, and advertises its decision to blkfront. Blkfront reads the blkback's decision and it also makes the decision for the use of the feature.
Commit 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter when connect"), however, made the blkfront's read of the parameter for disabling the advertisement, namely 'feature_persistent', to be done when it negotiate, not when advertise. Therefore blkfront advertises without reading the parameter. As the field for caching the parameter value is zero-initialized, it always advertises as the feature is disabled, so that the persistent grants feature becomes always disabled.
This commit fixes the issue by making the blkfront does parmeter caching just before the advertisement.
--- drivers/block/xen-blkfront.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/block/xen-blkfront.c +7 -7
@@ -1759,6 +1759,12 @@ static int write_per_ring_nodes(struct xenbus_transaction xbt,return err; } +/* Enable the persistent grants feature. */ +static bool feature_persistent = true; +module_param (feature_persistent, bool,0644 ); +MODULE_PARM_DESC (feature_persistent, +"Enables the persistent grants feature" ); +/* Common code used when first setting up, and when resuming. */ static int talk_to_blkback (struct xenbus_device *dev,struct blkfront_info *info)
@@ -1850,6 +1856,7 @@ static int talk_to_blkback(struct xenbus_device *dev, message ="writing protocol" ; goto abort_transaction; } + info->feature_persistent_parm = feature_persistent; err =xenbus_printf (xbt, dev->nodename,"feature-persistent" ,"%u" , info->feature_persistent_parm);if (err)
@@ -1919,12 +1926,6 @@ static int negotiate_mq(struct blkfront_info *info)return 0 ; } -/* Enable the persistent grants feature. */ -static bool feature_persistent = true; -module_param (feature_persistent, bool,0644 ); -MODULE_PARM_DESC (feature_persistent, -"Enables the persistent grants feature" ); -/* * Entry point to this code when a new device is created. Allocate the basic * structures and the ring buffer for communication with the backend, and
@@ -2284,7 +2285,6 @@ static void blkfront_gather_backend_features(struct blkfront_info *info)if (xenbus_read_unsigned (info->xbdev->otherend,"feature-discard" ,0 ))blkfront_setup_discard (info); - info->feature_persistent_parm = feature_persistent;if (info->feature_persistent_parm) info->feature_persistent = !!xenbus_read_unsigned(info->xbdev->otherend,
Signature · 1 line
2.25.1
Quoted 10 lines · On Wed, 31 Aug 2022 16:58:21 +0000 SeongJae Park <[email protected]> wrote:
Changes from v1
(https://lore.kernel.org/xen-devel/[email protected]/)
- Fix the wrongfeature_persistentcaching position of blkfront
- Set blkfront'sfeature_persistentfield setting with simple '&&' instead of 'if' (Pratyush Yadav)This patchset fixes misuse of the 'feature-persistent' advertisement semantic (patches 1 and 2), and the wrong timing of the '
feature_persistent' value caching, which made persistent grants feature always disabled.
Please note that I have some problem in my test setup and therefore was unable to fully test this patchset. I am posting this though, as the impact of the bug is not trivial (always disabling persistent grants), and to make testing of my proposed fix from others easier. Hope to get someone's test results or code review of this patchset even before I fix my test setup problem.
Juergen, I didn't add your 'Reviewed-by:'s to the first two patches of this series because I changed some of the description for making it clear which bug and commit it is really fixing. Specifically, I wordsmithed the working and changed 'Fixed:' tag. Code change is almost same, though.
Thanks,
SJ
Quoted 14 lines
SeongJae Park (3):
xen-blkback: Advertise feature-persistent as user requested
xen-blkfront: Advertise feature-persistent as user requested
xen-blkfront: Cachefeature_persistentvalue before advertisementdrivers/block/xen-blkback/common.h | 3 +++ drivers/block/xen-blkback/xenbus.c | 6 ++++-- drivers/block/xen-blkfront.c | 20 ++++++++++++-------- 3 files changed, 19 insertions(+), 10 deletions(-)Signature · 1 line
2.25.1
Marek Marczykowski-Górecki
Tested-byQuoted 13 lines · SeongJae Park
Quoted 10 lines · On Wed, 31 Aug 2022 16:58:21 +0000 SeongJae Park <[email protected]> wrote:
Changes from v1
(https://lore.kernel.org/xen-devel/[email protected]/)
- Fix the wrongfeature_persistentcaching position of blkfront
- Set blkfront'sfeature_persistentfield setting with simple '&&' instead of 'if' (Pratyush Yadav)This patchset fixes misuse of the 'feature-persistent' advertisement semantic (patches 1 and 2), and the wrong timing of the '
feature_persistent' value caching, which made persistent grants feature always disabled.
Quoted 5 lines
Please note that I have some problem in my test setup and therefore was unable to fully test this patchset. I am posting this though, as the impact of the bug is not trivial (always disabling persistent grants), and to make testing of my proposed fix from others easier. Hope to get someone's test results or code review of this patchset even before I fix my test setup problem.
I can confirm it fixes the issue, thanks!
Quoted 23 lines
Juergen, I didn't add your 'Reviewed-by:'s to the first two patches of this series because I changed some of the description for making it clear which bug and commit it is really fixing. Specifically, I wordsmithed the working and changed 'Fixed:' tag. Code change is almost same, though.
Thanks,
SJQuoted 14 lines
SeongJae Park (3):
xen-blkback: Advertise feature-persistent as user requested
xen-blkfront: Advertise feature-persistent as user requested
xen-blkfront: Cachefeature_persistentvalue before advertisementdrivers/block/xen-blkback/common.h | 3 +++ drivers/block/xen-blkback/xenbus.c | 6 ++++-- drivers/block/xen-blkfront.c | 20 ++++++++++++-------- 3 files changed, 19 insertions(+), 10 deletions(-)Signature · 1 line
2.25.1
Signature · 3 lines
Best Regards, Marek Marczykowski-Górecki Invisible Things Lab
Quoted 16 lines · SeongJae Park
Changes from v1
(https://lore.kernel.org/xen-devel/[email protected]/)
- Fix the wrongfeature_persistentcaching position of blkfront
- Set blkfront'sfeature_persistentfield setting with simple '&&' instead of 'if' (Pratyush Yadav)This patchset fixes misuse of the 'feature-persistent' advertisement semantic (patches 1 and 2), and the wrong timing of the '
feature_persistent' value caching, which made persistent grants feature always disabled.SeongJae Park (3):
xen-blkback: Advertise feature-persistent as user requested
xen-blkfront: Advertise feature-persistent as user requested
xen-blkfront: Cachefeature_persistentvalue before advertisement
Quoted 5 lines
drivers/block/xen-blkback/common.h | 3 +++ drivers/block/xen-blkback/xenbus.c | 6 ++++-- drivers/block/xen-blkfront.c | 20 ++++++++++++-------- 3 files changed, 19 insertions(+), 10 deletions(-)
For the whole series:
Juergen
Re: [PATCH v2 0/3] xen-blk{front, back}: Fix the broken semantic and flow of feature-persistent
In reply to SeongJae ParkQuoted 16 lines · SeongJae Park
Changes from v1
(https://lore.kernel.org/xen-devel/[email protected]/)
- Fix the wrongfeature_persistentcaching position of blkfront
- Set blkfront'sfeature_persistentfield setting with simple '&&' instead of 'if' (Pratyush Yadav)This patchset fixes misuse of the 'feature-persistent' advertisement semantic (patches 1 and 2), and the wrong timing of the '
feature_persistent' value caching, which made persistent grants feature always disabled.SeongJae Park (3):
xen-blkback: Advertise feature-persistent as user requested
xen-blkfront: Advertise feature-persistent as user requested
xen-blkfront: Cachefeature_persistentvalue before advertisement
Quoted 8 lines
drivers/block/xen-blkback/common.h | 3 +++ drivers/block/xen-blkback/xenbus.c | 6 ++++-- drivers/block/xen-blkfront.c | 20 ++++++++++++-------- 3 files changed, 19 insertions(+), 10 deletions(-)--
2.25.1
I've tested this patch series in the following ways:
* Only applied the blkback patch but not the blkfront patches
* Only applied the blkfront patches but not the blkback patch
* Applied both
All scenarios worked, so
Actually I also wanted to test changing feature_persistent and try reconnecting but I don't know how this is done. If anyone has a pointer here, I could test that as well.
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