mbox series

[v3,0/4] fix two edge cases related to stream block jobs

Message ID 20240322095009.346989-1-f.ebner@proxmox.com
Headers show
Series fix two edge cases related to stream block jobs | expand

Message

Fiona Ebner March 22, 2024, 9:50 a.m. UTC
Changes in v3:
    * Also deal with edge case in bdrv_next_cleanup(). Haven't run
      into an actual issue there, but at least the caller in
      migration/block.c uses bdrv_nb_sectors() which, while not a
      coroutine wrapper itself (it's written manually), may call
      bdrv_refresh_total_sectors(), which is a generated coroutine
      wrapper, so AFAIU, the block graph can change during that call.
      And even without that, it's just better to be more consistent
      with bdrv_next().

Changes in v2:
    * Ran into another issue while writing the IO test Stefan wanted
      to have (good call :)), so include a fix for that and add the
      test. I didn't notice during manual testing, because I hadn't
      used a scripted QMP 'quit', so there was no race.

Fiona Ebner (3):
  block-backend: fix edge case in bdrv_next() where BDS associated to BB
    changes
  block-backend: fix edge case in bdrv_next_cleanup() where BDS
    associated to BB changes
  iotests: add test for stream job with an unaligned prefetch read

Stefan Reiter (1):
  block/io: accept NULL qiov in bdrv_pad_request

 block/block-backend.c                         | 18 ++--
 block/io.c                                    | 31 ++++---
 .../tests/stream-unaligned-prefetch           | 86 +++++++++++++++++++
 .../tests/stream-unaligned-prefetch.out       |  5 ++
 4 files changed, 117 insertions(+), 23 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/stream-unaligned-prefetch
 create mode 100644 tests/qemu-iotests/tests/stream-unaligned-prefetch.out

Comments

Stefan Hajnoczi March 25, 2024, 8:11 p.m. UTC | #1
On Fri, Mar 22, 2024 at 10:50:05AM +0100, Fiona Ebner wrote:
> Changes in v3:
>     * Also deal with edge case in bdrv_next_cleanup(). Haven't run
>       into an actual issue there, but at least the caller in
>       migration/block.c uses bdrv_nb_sectors() which, while not a
>       coroutine wrapper itself (it's written manually), may call
>       bdrv_refresh_total_sectors(), which is a generated coroutine
>       wrapper, so AFAIU, the block graph can change during that call.
>       And even without that, it's just better to be more consistent
>       with bdrv_next().
> 
> Changes in v2:
>     * Ran into another issue while writing the IO test Stefan wanted
>       to have (good call :)), so include a fix for that and add the
>       test. I didn't notice during manual testing, because I hadn't
>       used a scripted QMP 'quit', so there was no race.
> 
> Fiona Ebner (3):
>   block-backend: fix edge case in bdrv_next() where BDS associated to BB
>     changes
>   block-backend: fix edge case in bdrv_next_cleanup() where BDS
>     associated to BB changes
>   iotests: add test for stream job with an unaligned prefetch read
> 
> Stefan Reiter (1):
>   block/io: accept NULL qiov in bdrv_pad_request
> 
>  block/block-backend.c                         | 18 ++--
>  block/io.c                                    | 31 ++++---
>  .../tests/stream-unaligned-prefetch           | 86 +++++++++++++++++++
>  .../tests/stream-unaligned-prefetch.out       |  5 ++
>  4 files changed, 117 insertions(+), 23 deletions(-)
>  create mode 100755 tests/qemu-iotests/tests/stream-unaligned-prefetch
>  create mode 100644 tests/qemu-iotests/tests/stream-unaligned-prefetch.out

Looks good to me. I will wait until Thursday before merging in case
Hanna, Vladimir, or Kevin have comments. Thanks!

Stefan
Kevin Wolf March 26, 2024, 12:53 p.m. UTC | #2
Am 25.03.2024 um 21:11 hat Stefan Hajnoczi geschrieben:
> On Fri, Mar 22, 2024 at 10:50:05AM +0100, Fiona Ebner wrote:
> > Changes in v3:
> >     * Also deal with edge case in bdrv_next_cleanup(). Haven't run
> >       into an actual issue there, but at least the caller in
> >       migration/block.c uses bdrv_nb_sectors() which, while not a
> >       coroutine wrapper itself (it's written manually), may call
> >       bdrv_refresh_total_sectors(), which is a generated coroutine
> >       wrapper, so AFAIU, the block graph can change during that call.
> >       And even without that, it's just better to be more consistent
> >       with bdrv_next().
> > 
> > Changes in v2:
> >     * Ran into another issue while writing the IO test Stefan wanted
> >       to have (good call :)), so include a fix for that and add the
> >       test. I didn't notice during manual testing, because I hadn't
> >       used a scripted QMP 'quit', so there was no race.
> > 
> > Fiona Ebner (3):
> >   block-backend: fix edge case in bdrv_next() where BDS associated to BB
> >     changes
> >   block-backend: fix edge case in bdrv_next_cleanup() where BDS
> >     associated to BB changes
> >   iotests: add test for stream job with an unaligned prefetch read
> > 
> > Stefan Reiter (1):
> >   block/io: accept NULL qiov in bdrv_pad_request
> > 
> >  block/block-backend.c                         | 18 ++--
> >  block/io.c                                    | 31 ++++---
> >  .../tests/stream-unaligned-prefetch           | 86 +++++++++++++++++++
> >  .../tests/stream-unaligned-prefetch.out       |  5 ++
> >  4 files changed, 117 insertions(+), 23 deletions(-)
> >  create mode 100755 tests/qemu-iotests/tests/stream-unaligned-prefetch
> >  create mode 100644 tests/qemu-iotests/tests/stream-unaligned-prefetch.out
> 
> Looks good to me. I will wait until Thursday before merging in case
> Hanna, Vladimir, or Kevin have comments. Thanks!

Let's not delay it to -rc2. If something turns out to be wrong with it,
we can still revert it, but I think getting fixes in earlier is better
during freeze.

Thanks, applied to the block branch.

Kevin