diff mbox

[v3,05/19] block: make bdrv_co_is_allocated static

Message ID 1374762197-7261-6-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini July 25, 2013, 2:23 p.m. UTC 
bdrv_is_allocated can detect coroutine context and go through a fast
path, similar to other block layer functions.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c               | 24 +++++++++++++++---------
 block/raw.c           |  2 +-
 block/stream.c        |  4 ++--
 include/block/block.h |  2 --
 4 files changed, 18 insertions(+), 14 deletions(-)

Comments

Kevin Wolf July 29, 2013, 1:21 p.m. UTC | #1 
Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> bdrv_is_allocated can detect coroutine context and go through a fast
> path, similar to other block layer functions.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

It can, but why is this a good idea?

The other block layer functions do this because existing drivers call
bdrv_(p)read/write(v) and we don't want them to block the vcpu. So it's
basically just a method of avoiding massive changes to existing callers.

The only such caller of bdrv_is_allocated() that I can see is vvfat, and
I don't think optimising that is our top priority.

Charlie is working in exactly the opposite direction, namely splitting
the existing functions in a coroutine-only and a synchronous-only part.
We should be clear what we really want here.

Kevin
Paolo Bonzini July 29, 2013, 1:56 p.m. UTC | #2 
Il 29/07/2013 15:21, Kevin Wolf ha scritto:
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>> bdrv_is_allocated can detect coroutine context and go through a fast
>> path, similar to other block layer functions.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> It can, but why is this a good idea?

What exactly?  Dropping bdrv_co_is_allocated from the public API, or not
creating always a new coroutine while still dropping bdrv_co_is_allocated?

In the end, the answer is simply for consistency.  bdrv_co_is_allocated
is different from all other APIs in this respect, and I didn't want to
choose between making get_block_status the same as is_allocated vs. the
same as everything else.

> Charlie is working in exactly the opposite direction, namely splitting
> the existing functions in a coroutine-only and a synchronous-only part.
> We should be clear what we really want here.

That's fine---but whatever we do we should do it for all APIs.  So this
patch is just making things consistent before adding another API.

Paolo
diff mbox

Patch

diff --git a/block.c b/block.c
index ebac2fa..dd4c570 100644
--- a/block.c
+++ b/block.c
@@ -2527,7 +2527,7 @@  static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
     if (flags & BDRV_REQ_COPY_ON_READ) {
         int pnum;
 
-        ret = bdrv_co_is_allocated(bs, sector_num, nb_sectors, &pnum);
+        ret = bdrv_is_allocated(bs, sector_num, nb_sectors, &pnum);
         if (ret < 0) {
             goto out;
         }
@@ -2981,8 +2981,9 @@  typedef struct BdrvCoIsAllocatedData {
  * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
  * beyond the end of the disk image it will be clamped.
  */
-int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs, int64_t sector_num,
-                                      int nb_sectors, int *pnum)
+static int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs,
+                                             int64_t sector_num,
+                                             int nb_sectors, int *pnum)
 {
     int64_t n;
 
@@ -3032,10 +3033,15 @@  int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
         .done = false,
     };
 
-    co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
-    qemu_coroutine_enter(co, &data);
-    while (!data.done) {
-        qemu_aio_wait();
+    if (qemu_in_coroutine()) {
+        /* Fast-path if already in coroutine context */
+        bdrv_is_allocated_co_entry(&data);
+    } else {
+        co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
+        qemu_coroutine_enter(co, &data);
+        while (!data.done) {
+            qemu_aio_wait();
+        }
     }
     return data.ret;
 }
@@ -3063,8 +3069,8 @@  int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
     intermediate = top;
     while (intermediate && intermediate != base) {
         int pnum_inter;
-        ret = bdrv_co_is_allocated(intermediate, sector_num, nb_sectors,
-                                   &pnum_inter);
+        ret = bdrv_is_allocated(intermediate, sector_num, nb_sectors,
+                                &pnum_inter);
         if (ret < 0) {
             return ret;
         } else if (ret) {
diff --git a/block/raw.c b/block/raw.c
index f1682d4..4517a81 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -39,7 +39,7 @@  static int coroutine_fn raw_co_is_allocated(BlockDriverState *bs,
                                             int64_t sector_num,
                                             int nb_sectors, int *pnum)
 {
-    return bdrv_co_is_allocated(bs->file, sector_num, nb_sectors, pnum);
+    return bdrv_is_allocated(bs->file, sector_num, nb_sectors, pnum);
 }
 
 static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
diff --git a/block/stream.c b/block/stream.c
index 7fe9e48..fb1f9c3 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -115,8 +115,8 @@  wait:
             break;
         }
 
-        ret = bdrv_co_is_allocated(bs, sector_num,
-                                   STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n);
+        ret = bdrv_is_allocated(bs, sector_num,
+                                STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n);
         if (ret == 1) {
             /* Allocated in the top, no need to copy.  */
             copy = false;
diff --git a/include/block/block.h b/include/block/block.h
index 742fce5..dde745f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -181,8 +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(BlockDriverState *bs, int64_t sector_num,
-    int nb_sectors, int *pnum);
 int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
                                             BlockDriverState *base,
                                             int64_t sector_num,