Patchwork [v3,06/19] block: remove bdrv_is_allocated_above/bdrv_co_is_allocated_above distinction

login
register
mail settings
Submitter Paolo Bonzini
Date July 25, 2013, 2:23 p.m.
Message ID <1374762197-7261-7-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/261711/
State New
Headers show

Comments

Paolo Bonzini - July 25, 2013, 2:23 p.m.
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(-)
Kevin Wolf - July 29, 2013, 1:34 p.m.
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
Paolo Bonzini - July 29, 2013, 1:59 p.m.
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
Kevin Wolf - July 29, 2013, 2:15 p.m.
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

Patch

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);