| Message ID | 1374762197-7261-7-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <eblake@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > 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. 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. Kevin
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 <eblake@redhat.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> 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. :( > 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. Paolo
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 <eblake@redhat.com> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >> --- > >> 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
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) { BlockDriverState *intermediate; int ret, n = nb_sectors; @@ -3097,44 +3097,6 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, return 0; } -/* Coroutine wrapper for bdrv_is_allocated_above() */ -static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque) -{ - BdrvCoIsAllocatedData *data = opaque; - BlockDriverState *top = data->bs; - BlockDriverState *base = data->base; - - data->ret = bdrv_co_is_allocated_above(top, base, data->sector_num, - data->nb_sectors, data->pnum); - data->done = true; -} - -/* - * Synchronous wrapper around bdrv_co_is_allocated_above(). - * - * See bdrv_co_is_allocated_above() for details. - */ -int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, - int64_t sector_num, int nb_sectors, int *pnum) -{ - Coroutine *co; - BdrvCoIsAllocatedData data = { - .bs = top, - .base = base, - .sector_num = sector_num, - .nb_sectors = nb_sectors, - .pnum = pnum, - .done = false, - }; - - co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry); - qemu_coroutine_enter(co, &data); - while (!data.done) { - qemu_aio_wait(); - } - return data.ret; -} - const char *bdrv_get_encrypted_filename(BlockDriverState *bs) { if (bs->backing_hd && bs->backing_hd->encrypted) diff --git a/block/commit.c b/block/commit.c index 2227fc2..37572f0 100644 --- a/block/commit.c +++ b/block/commit.c @@ -108,9 +108,9 @@ wait: break; } /* Copy if allocated above the base */ - ret = bdrv_co_is_allocated_above(top, base, sector_num, - COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE, - &n); + ret = bdrv_is_allocated_above(top, base, sector_num, + COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE, + &n); copy = (ret == 1); trace_commit_one_iteration(s, sector_num, n, ret); if (copy) { diff --git a/block/mirror.c b/block/mirror.c index bed4a7e..0b3c2f6 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -338,8 +338,8 @@ static void coroutine_fn mirror_run(void *opaque) base = s->mode == MIRROR_SYNC_MODE_FULL ? NULL : bs->backing_hd; for (sector_num = 0; sector_num < end; ) { int64_t next = (sector_num | (sectors_per_chunk - 1)) + 1; - ret = bdrv_co_is_allocated_above(bs, base, - sector_num, next - sector_num, &n); + ret = bdrv_is_allocated_above(bs, base, + sector_num, next - sector_num, &n); if (ret < 0) { goto immediate_exit; diff --git a/block/stream.c b/block/stream.c index fb1f9c3..60136fb 100644 --- a/block/stream.c +++ b/block/stream.c @@ -123,8 +123,8 @@ wait: } else { /* Copy if allocated in the intermediate images. Limit to the * known-unallocated area [sector_num, sector_num+n). */ - ret = bdrv_co_is_allocated_above(bs->backing_hd, base, - sector_num, n, &n); + ret = bdrv_is_allocated_above(bs->backing_hd, base, + sector_num, n, &n); /* Finish early if end of backing file has been reached */ if (ret == 0 && n == 0) { diff --git a/include/block/block.h b/include/block/block.h index dde745f..64f7730 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -181,10 +181,6 @@ int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num, */ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, int nb_sectors); -int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, - BlockDriverState *base, - int64_t sector_num, - int nb_sectors, int *pnum); BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, const char *backing_file); int bdrv_get_backing_file_depth(BlockDriverState *bs);