[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 06/19] block: remove bdrv_is_allocated_above/
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v3 06/19] block: remove bdrv_is_allocated_above/bdrv_co_is_allocated_above distinction |
Date: |
Mon, 29 Jul 2013 16:15:47 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 29.07.2013 um 15:59 hat Paolo Bonzini geschrieben:
> Il 29/07/2013 15:34, Kevin Wolf ha scritto:
> > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> >> Now that bdrv_is_allocated detects coroutine context, the two can
> >> use the same code.
> >>
> >> Reviewed-by: Eric Blake <address@hidden>
> >> Signed-off-by: Paolo Bonzini <address@hidden>
> >> ---
> >> block.c | 46 ++++------------------------------------------
> >> block/commit.c | 6 +++---
> >> block/mirror.c | 4 ++--
> >> block/stream.c | 4 ++--
> >> include/block/block.h | 4 ----
> >> 5 files changed, 11 insertions(+), 53 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index dd4c570..1ee1d93 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -3058,10 +3058,10 @@ int bdrv_is_allocated(BlockDriverState *bs,
> >> int64_t sector_num, int nb_sectors,
> >> * allocated/unallocated state.
> >> *
> >> */
> >> -int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
> >> - BlockDriverState *base,
> >> - int64_t sector_num,
> >> - int nb_sectors, int *pnum)
> >> +int coroutine_fn bdrv_is_allocated_above(BlockDriverState *top,
> >> + BlockDriverState *base,
> >> + int64_t sector_num,
> >> + int nb_sectors, int *pnum)
> >
> > This is no longer a coroutine_fn.
>
> I'm always confused about must-yield vs. may-yield. :(
A coroutine_fn may yield without checking whether it runs in a
coroutine. Or phrased differently, coroutine_fns may only be called from
coroutine context.
> > However, if this was the only reason for making bdrv_is_allocated() a
> > hybrid function, it may be easier to simply let the only caller
> > (img_compare in qemu-img) run in a coroutine and drop the synchronous
> > version entirely, keeping only a coroutine_fn.
>
> The reason is to avoid having the same (IMHO pretty gratuitous)
> distinction in get_block_status. "qemu-img map" will also add another
> synchronous user of get_block_status.
>
> If we want to keep only the coroutine_fn, we should make all of qemu-img
> run in a coroutine.
I think in this case that's really a good option, because qemu-img is
the only caller for the synchronous version (if I didn't miss any
other). It may turn out useful for other functions as well.
And letting qemu-img's main() call into a coroutine doesn't really seem
too bad compared to doing the optional coroutine dance for each block.c
function. And it finally gets us a main loop in qemu-img, too.
Kevin
- Re: [Qemu-devel] [PATCH v3 04/19] block: update bs->total_sectors on writes, (continued)
[Qemu-devel] [PATCH v3 03/19] cow: do not call bdrv_co_is_allocated, Paolo Bonzini, 2013/07/25
[Qemu-devel] [PATCH v3 05/19] block: make bdrv_co_is_allocated static, Paolo Bonzini, 2013/07/25
[Qemu-devel] [PATCH v3 06/19] block: remove bdrv_is_allocated_above/bdrv_co_is_allocated_above distinction, Paolo Bonzini, 2013/07/25
[Qemu-devel] [PATCH v3 07/19] block: expect errors from bdrv_co_is_allocated, Paolo Bonzini, 2013/07/25
[Qemu-devel] [PATCH v3 09/19] block: make bdrv_has_zero_init return false for copy-on-write-images, Paolo Bonzini, 2013/07/25
[Qemu-devel] [PATCH v3 08/19] qemu-img: always probe the input image for allocated sectors, Paolo Bonzini, 2013/07/25
[Qemu-devel] [PATCH v3 10/19] block: introduce bdrv_get_block_status API, Paolo Bonzini, 2013/07/25
[Qemu-devel] [PATCH v3 12/19] block: return get_block_status data and flags for formats, Paolo Bonzini, 2013/07/25