[PATCH v2 0/3] xen-blk{front,back}: Fix the broken semantic and flow of feature-persistent

SeongJae Park posted 3 patches 1 year, 8 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
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(-)
[PATCH v2 0/3] xen-blk{front,back}: Fix the broken semantic and flow of feature-persistent
Posted by SeongJae Park 1 year, 8 months ago
Changes from v1
(https://lore.kernel.org/xen-devel/20220825161511.94922-1-sj@kernel.org/)
- 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(-)

-- 
2.25.1
Re: [PATCH v2 0/3] xen-blk{front, back}: Fix the broken semantic and flow of feature-persistent
Posted by Maximilian Heyne 1 year, 8 months ago
On Wed, Aug 31, 2022 at 04:58:21PM +0000, SeongJae Park wrote:
> Changes from v1
> (https://lore.kernel.org/xen-devel/20220825161511.94922-1-sj@kernel.org/)
> - 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(-)
> 
> --
> 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

Tested-by: Maximilian Heyne <mheyne@amazon.de>

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
Re: [PATCH v2 0/3] xen-blk{front,back}: Fix the broken semantic and flow of feature-persistent
Posted by Juergen Gross 1 year, 8 months ago
On 31.08.22 18:58, SeongJae Park wrote:
> Changes from v1
> (https://lore.kernel.org/xen-devel/20220825161511.94922-1-sj@kernel.org/)
> - 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(-)
> 

For the whole series:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen
Re: [PATCH v2 0/3] xen-blk{front,back}: Fix the broken semantic and flow of feature-persistent
Posted by SeongJae Park 1 year, 8 months ago
On Wed, 31 Aug 2022 16:58:21 +0000 SeongJae Park <sj@kernel.org> wrote:

> Changes from v1
> (https://lore.kernel.org/xen-devel/20220825161511.94922-1-sj@kernel.org/)
> - 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.

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

> 
> 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(-)
> 
> -- 
> 2.25.1
>
Re: [PATCH v2 0/3] xen-blk{front,back}: Fix the broken semantic and flow of feature-persistent
Posted by Marek Marczykowski-Górecki 1 year, 8 months ago
On Wed, Aug 31, 2022 at 05:08:17PM +0000, SeongJae Park wrote:
> On Wed, 31 Aug 2022 16:58:21 +0000 SeongJae Park <sj@kernel.org> wrote:
> 
> > Changes from v1
> > (https://lore.kernel.org/xen-devel/20220825161511.94922-1-sj@kernel.org/)
> > - 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.
> 
> 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!

Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> 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
> 
> > 
> > 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(-)
> > 
> > -- 
> > 2.25.1
> > 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab