[Qemu-devel] [PATCH 00/10] block: replace .bdrv_is_allocated() with .bdrv_co_is_allocated()

[Qemu-devel] [PATCH 00/10] block: replace .bdrv_is_allocated() with .bdrv_co_is_allocated()

[Stefan Hajnoczi]

The bdrv_is_allocated() interface is not suitable for use while the VM is
running.  It is a synchronous interface so it may block the running VM for
arbitrary amounts of time.  It also assumes it is the only block driver
operation and there is a risk that internal state could be corrupted if
asynchronous I/O is currently pending.

This patch series refactors the block layer so that the bdrv_is_allocated()
operation is performed internally in coroutine context.  We can then introduce
a new public bdrv_co_is_allocated() function that is suitable for use when the
VM is running.

Making these changes without breaking git bisect means we need to first
introduce BlockDriver .bdrv_co_is_allocated() and add wrapper code into
bdrv_is_allocated() so existing callers can use converted BlockDrivers.

After converting all block drivers there are no .bdrv_is_allocated() functions
left and the interface can be removed from BlockDriver.

Finally, we can add the new public bdrv_co_is_allocated() public interface and
turn the old bdrv_is_allocated() into a wrapper.

This series is a prerequisite for copy-on-read, which needs to check whether
sectors are allocated while the VM is running.

Stefan Hajnoczi (10):
  block: use public bdrv_is_allocated() interface
  block: add .bdrv_co_is_allocated()
  qed: convert to .bdrv_co_is_allocated()
  block: convert qcow2, qcow2, and vmdk to .bdrv_co_is_allocated()
  vvfat: use public block layer interface
  vvfat: convert to .bdrv_co_is_allocated()
  vdi: convert to .bdrv_co_is_allocated()
  cow: convert to .bdrv_co_is_allocated()
  block: drop .bdrv_is_allocated() interface
  block: add bdrv_co_is_allocated() interface

 block.c       |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 block.h       |    2 +
 block/cow.c   |    8 +++---
 block/qcow.c  |    8 ++++--
 block/qcow2.c |   13 +++++++----
 block/qed.c   |   15 ++++++++++---
 block/vdi.c   |    6 ++--
 block/vmdk.c  |    8 ++++--
 block/vvfat.c |   19 +++++++----------
 block_int.h   |    4 +-
 10 files changed, 100 insertions(+), 41 deletions(-)

-- 
1.7.7.1

[Qemu-devel] [PATCH 01/10] block: use public bdrv_is_allocated() interface

[Stefan Hajnoczi]

There is no need for bdrv_commit() to use the BlockDriver
.bdrv_is_allocated() interface directly.  Converting to the public
interface gives us the freedom to drop .bdrv_is_allocated() entirely in
favor of a new .bdrv_co_is_allocated() in the future.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block.c b/block.c
index 9bb236c..e6ac6d3 100644
--- a/block.c
+++ b/block.c
@@ -915,7 +915,7 @@ int bdrv_commit(BlockDriverState *bs)
     buf = g_malloc(COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
 
     for (sector = 0; sector < total_sectors; sector += n) {
-        if (drv->bdrv_is_allocated(bs, sector, COMMIT_BUF_SECTORS, &n)) {
+        if (bdrv_is_allocated(bs, sector, COMMIT_BUF_SECTORS, &n)) {
 
             if (bdrv_read(bs, sector, buf, n) != 0) {
                 ret = -EIO;
-- 
1.7.7.1

[Qemu-devel] [PATCH 02/10] block: add .bdrv_co_is_allocated()

[Stefan Hajnoczi]

This patch adds the .bdrv_co_is_allocated() interface which is identical
to .bdrv_is_allocated() but runs in coroutine context.  Running in
coroutine context implies that other coroutines might be performing I/O
at the same time.   Therefore it must be safe to run while the following
BlockDriver functions are in-flight:

    .bdrv_co_readv()
    .bdrv_co_writev()
    .bdrv_co_flush()
    .bdrv_co_is_allocated()

The new .bdrv_co_is_allocated() interface is useful because it can be
used when a VM is running, whereas .bdrv_is_allocated() is a synchronous
interface that does not cope with parallel requests.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c     |   37 +++++++++++++++++++++++++++++++++++++
 block_int.h |    2 ++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index e6ac6d3..f8705b7 100644
--- a/block.c
+++ b/block.c
@@ -1771,6 +1771,26 @@ int bdrv_has_zero_init(BlockDriverState *bs)
     return 1;
 }
 
+typedef struct BdrvCoIsAllocatedData {
+    BlockDriverState *bs;
+    int64_t sector_num;
+    int nb_sectors;
+    int *pnum;
+    int ret;
+    bool done;
+} BdrvCoIsAllocatedData;
+
+/* Coroutine wrapper for bdrv_is_allocated() */
+static void coroutine_fn bdrv_is_allocated_co_entry(void *opaque)
+{
+    BdrvCoIsAllocatedData *data = opaque;
+    BlockDriverState *bs = data->bs;
+
+    data->ret = bs->drv->bdrv_co_is_allocated(bs, data->sector_num,
+                                              data->nb_sectors, data->pnum);
+    data->done = true;
+}
+
 /*
  * Returns true iff the specified sector is present in the disk image. Drivers
  * not implementing the functionality are assumed to not support backing files,
@@ -1786,6 +1806,23 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
 	int *pnum)
 {
     int64_t n;
+    if (bs->drv->bdrv_co_is_allocated) {
+        Coroutine *co;
+        BdrvCoIsAllocatedData data = {
+            .bs = bs,
+            .sector_num = sector_num,
+            .nb_sectors = nb_sectors,
+            .pnum = pnum,
+            .done = false,
+        };
+
+        co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
+        qemu_coroutine_enter(co, &data);
+        while (!data.done) {
+            qemu_aio_wait();
+        }
+        return data.ret;
+    }
     if (!bs->drv->bdrv_is_allocated) {
         if (sector_num >= bs->total_sectors) {
             *pnum = 0;
diff --git a/block_int.h b/block_int.h
index f4547f6..1c1351c 100644
--- a/block_int.h
+++ b/block_int.h
@@ -87,6 +87,8 @@ struct BlockDriver {
     int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
     int coroutine_fn (*bdrv_co_discard)(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 (*bdrv_aio_multiwrite)(BlockDriverState *bs, BlockRequest *reqs,
         int num_reqs);
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH 02/10] block: add .bdrv_co_is_allocated()

[Zhi Yong Wu]

On Sat, Nov 12, 2011 at 12:47 AM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> This patch adds the .bdrv_co_is_allocated() interface which is identical
> to .bdrv_is_allocated() but runs in coroutine context.  Running in
> coroutine context implies that other coroutines might be performing I/O
> at the same time.   Therefore it must be safe to run while the following
> BlockDriver functions are in-flight:
>
>    .bdrv_co_readv()
>    .bdrv_co_writev()
>    .bdrv_co_flush()
>    .bdrv_co_is_allocated()
>
> The new .bdrv_co_is_allocated() interface is useful because it can be
> used when a VM is running, whereas .bdrv_is_allocated() is a synchronous
> interface that does not cope with parallel requests.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block.c     |   37 +++++++++++++++++++++++++++++++++++++
>  block_int.h |    2 ++
>  2 files changed, 39 insertions(+), 0 deletions(-)
>
> diff --git a/block.c b/block.c
> index e6ac6d3..f8705b7 100644
> --- a/block.c
> +++ b/block.c
> @@ -1771,6 +1771,26 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>     return 1;
>  }
>
> +typedef struct BdrvCoIsAllocatedData {
> +    BlockDriverState *bs;
> +    int64_t sector_num;
> +    int nb_sectors;
> +    int *pnum;
> +    int ret;
> +    bool done;
> +} BdrvCoIsAllocatedData;
> +
> +/* Coroutine wrapper for bdrv_is_allocated() */
> +static void coroutine_fn bdrv_is_allocated_co_entry(void *opaque)
> +{
> +    BdrvCoIsAllocatedData *data = opaque;
> +    BlockDriverState *bs = data->bs;
> +
> +    data->ret = bs->drv->bdrv_co_is_allocated(bs, data->sector_num,
> +                                              data->nb_sectors, data->pnum);
> +    data->done = true;
> +}
> +
>  /*
>  * Returns true iff the specified sector is present in the disk image. Drivers
>  * not implementing the functionality are assumed to not support backing files,
> @@ -1786,6 +1806,23 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>        int *pnum)
>  {
>     int64_t n;
> +    if (bs->drv->bdrv_co_is_allocated) {
> +        Coroutine *co;
> +        BdrvCoIsAllocatedData data = {
> +            .bs = bs,
> +            .sector_num = sector_num,
> +            .nb_sectors = nb_sectors,
> +            .pnum = pnum,
> +            .done = false,
> +        };
> +
> +        co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
> +        qemu_coroutine_enter(co, &data);
Since this main process will stop within qemu_coroutine_enter() until
bdrv_is_allocated_co_entry() is completed, three lines of condition
codes below are unnecessary, right?
> +        while (!data.done) {
> +            qemu_aio_wait();
> +        }
> +        return data.ret;
> +    }
>     if (!bs->drv->bdrv_is_allocated) {
>         if (sector_num >= bs->total_sectors) {
>             *pnum = 0;
> diff --git a/block_int.h b/block_int.h
> index f4547f6..1c1351c 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -87,6 +87,8 @@ struct BlockDriver {
>     int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
>     int coroutine_fn (*bdrv_co_discard)(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 (*bdrv_aio_multiwrite)(BlockDriverState *bs, BlockRequest *reqs,
>         int num_reqs);
> --
> 1.7.7.1
>
>
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH 02/10] block: add .bdrv_co_is_allocated()

[Paolo Bonzini]

On 11/14/2011 04:04 AM, Zhi Yong Wu wrote:
>> >  +        co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
>> >  +        qemu_coroutine_enter(co,&data);
> Since this main process will stop within qemu_coroutine_enter() until
> bdrv_is_allocated_co_entry() is completed, three lines of condition
> codes below are unnecessary, right?

No, they are necessary.  They are executed when 
bdrv_is_allocated_co_entry calls qemu_coroutine_yield.

>> >  +        while (!data.done) {
>> >  +            qemu_aio_wait();
>> >  +        }

Paolo

Re: [Qemu-devel] [PATCH 02/10] block: add .bdrv_co_is_allocated()

[Zhi Yong Wu]

On Mon, Nov 14, 2011 at 3:32 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 11/14/2011 04:04 AM, Zhi Yong Wu wrote:
>>>
>>> >  +        co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
>>> >  +        qemu_coroutine_enter(co,&data);
>>
>> Since this main process will stop within qemu_coroutine_enter() until
>> bdrv_is_allocated_co_entry() is completed, three lines of condition
>> codes below are unnecessary, right?
>
> No, they are necessary.  They are executed when bdrv_is_allocated_co_entry
> calls qemu_coroutine_yield.
Right, But i don't think that they are necessary.

after bdrv_is_allocated_co_entry has basically completed all main
task, it call qemu_coroutine_yield to wake up this current process; At
that point, it is equal to the setting of data.done. Why need you
still the three lines of codes below?

>
>>> >  +        while (!data.done) {
>>> >  +            qemu_aio_wait();
>>> >  +        }
>
> Paolo
>
>
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH 02/10] block: add .bdrv_co_is_allocated()

[Paolo Bonzini]

On 11/14/2011 09:10 AM, Zhi Yong Wu wrote:
>> >  No, they are necessary.  They are executed when bdrv_is_allocated_co_entry
>> >  calls qemu_coroutine_yield.
> Right, But i don't think that they are necessary.
>
> after bdrv_is_allocated_co_entry has basically completed all main
> task, it call qemu_coroutine_yield to wake up this current process; At
> that point, it is equal to the setting of data.done. Why need you
> still the three lines of codes below?

Any function _called_ by the driver's is_allocated member could call 
qemu_coroutine_yield, for example bdrv_read.

Paolo

Re: [Qemu-devel] [PATCH 02/10] block: add .bdrv_co_is_allocated()

[Zhi Yong Wu]

On Mon, Nov 14, 2011 at 4:37 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 11/14/2011 09:10 AM, Zhi Yong Wu wrote:
>>>
>>> >  No, they are necessary.  They are executed when
>>> > bdrv_is_allocated_co_entry
>>> >  calls qemu_coroutine_yield.
>>
>> Right, But i don't think that they are necessary.
>>
>> after bdrv_is_allocated_co_entry has basically completed all main
>> task, it call qemu_coroutine_yield to wake up this current process; At
>> that point, it is equal to the setting of data.done. Why need you
>> still the three lines of codes below?
>
> Any function _called_ by the driver's is_allocated member could call
> qemu_coroutine_yield, for example bdrv_read.
Good example. thanks.
>
> Paolo
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH 02/10] block: add .bdrv_co_is_allocated()

[Kevin Wolf]

Am 14.11.2011 04:04, schrieb Zhi Yong Wu:
> On Sat, Nov 12, 2011 at 12:47 AM, Stefan Hajnoczi
> <stefanha@linux.vnet.ibm.com> wrote:
>> This patch adds the .bdrv_co_is_allocated() interface which is identical
>> to .bdrv_is_allocated() but runs in coroutine context.  Running in
>> coroutine context implies that other coroutines might be performing I/O
>> at the same time.   Therefore it must be safe to run while the following
>> BlockDriver functions are in-flight:
>>
>>    .bdrv_co_readv()
>>    .bdrv_co_writev()
>>    .bdrv_co_flush()
>>    .bdrv_co_is_allocated()
>>
>> The new .bdrv_co_is_allocated() interface is useful because it can be
>> used when a VM is running, whereas .bdrv_is_allocated() is a synchronous
>> interface that does not cope with parallel requests.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> ---
>>  block.c     |   37 +++++++++++++++++++++++++++++++++++++
>>  block_int.h |    2 ++
>>  2 files changed, 39 insertions(+), 0 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index e6ac6d3..f8705b7 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1771,6 +1771,26 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>>     return 1;
>>  }
>>
>> +typedef struct BdrvCoIsAllocatedData {
>> +    BlockDriverState *bs;
>> +    int64_t sector_num;
>> +    int nb_sectors;
>> +    int *pnum;
>> +    int ret;
>> +    bool done;
>> +} BdrvCoIsAllocatedData;
>> +
>> +/* Coroutine wrapper for bdrv_is_allocated() */
>> +static void coroutine_fn bdrv_is_allocated_co_entry(void *opaque)
>> +{
>> +    BdrvCoIsAllocatedData *data = opaque;
>> +    BlockDriverState *bs = data->bs;
>> +
>> +    data->ret = bs->drv->bdrv_co_is_allocated(bs, data->sector_num,
>> +                                              data->nb_sectors, data->pnum);
>> +    data->done = true;
>> +}
>> +
>>  /*
>>  * Returns true iff the specified sector is present in the disk image. Drivers
>>  * not implementing the functionality are assumed to not support backing files,
>> @@ -1786,6 +1806,23 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>>        int *pnum)
>>  {
>>     int64_t n;
>> +    if (bs->drv->bdrv_co_is_allocated) {
>> +        Coroutine *co;
>> +        BdrvCoIsAllocatedData data = {
>> +            .bs = bs,
>> +            .sector_num = sector_num,
>> +            .nb_sectors = nb_sectors,
>> +            .pnum = pnum,
>> +            .done = false,
>> +        };
>> +
>> +        co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
>> +        qemu_coroutine_enter(co, &data);
> Since this main process will stop within qemu_coroutine_enter() until
> bdrv_is_allocated_co_entry() is completed, three lines of condition
> codes below are unnecessary, right?

No, they are needed. We want to wait until the whole operation has
completed, but qemu_coroutine_enter() returns after the first
qemu_coroutine_yield().

Kevin

Re: [Qemu-devel] [PATCH 02/10] block: add .bdrv_co_is_allocated()

[Zhi Yong Wu]

On Mon, Nov 14, 2011 at 4:50 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 14.11.2011 04:04, schrieb Zhi Yong Wu:
>> On Sat, Nov 12, 2011 at 12:47 AM, Stefan Hajnoczi
>> <stefanha@linux.vnet.ibm.com> wrote:
>>> This patch adds the .bdrv_co_is_allocated() interface which is identical
>>> to .bdrv_is_allocated() but runs in coroutine context.  Running in
>>> coroutine context implies that other coroutines might be performing I/O
>>> at the same time.   Therefore it must be safe to run while the following
>>> BlockDriver functions are in-flight:
>>>
>>>    .bdrv_co_readv()
>>>    .bdrv_co_writev()
>>>    .bdrv_co_flush()
>>>    .bdrv_co_is_allocated()
>>>
>>> The new .bdrv_co_is_allocated() interface is useful because it can be
>>> used when a VM is running, whereas .bdrv_is_allocated() is a synchronous
>>> interface that does not cope with parallel requests.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>> ---
>>>  block.c     |   37 +++++++++++++++++++++++++++++++++++++
>>>  block_int.h |    2 ++
>>>  2 files changed, 39 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index e6ac6d3..f8705b7 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -1771,6 +1771,26 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>>>     return 1;
>>>  }
>>>
>>> +typedef struct BdrvCoIsAllocatedData {
>>> +    BlockDriverState *bs;
>>> +    int64_t sector_num;
>>> +    int nb_sectors;
>>> +    int *pnum;
>>> +    int ret;
>>> +    bool done;
>>> +} BdrvCoIsAllocatedData;
>>> +
>>> +/* Coroutine wrapper for bdrv_is_allocated() */
>>> +static void coroutine_fn bdrv_is_allocated_co_entry(void *opaque)
>>> +{
>>> +    BdrvCoIsAllocatedData *data = opaque;
>>> +    BlockDriverState *bs = data->bs;
>>> +
>>> +    data->ret = bs->drv->bdrv_co_is_allocated(bs, data->sector_num,
>>> +                                              data->nb_sectors, data->pnum);
>>> +    data->done = true;
>>> +}
>>> +
>>>  /*
>>>  * Returns true iff the specified sector is present in the disk image. Drivers
>>>  * not implementing the functionality are assumed to not support backing files,
>>> @@ -1786,6 +1806,23 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>>>        int *pnum)
>>>  {
>>>     int64_t n;
>>> +    if (bs->drv->bdrv_co_is_allocated) {
>>> +        Coroutine *co;
>>> +        BdrvCoIsAllocatedData data = {
>>> +            .bs = bs,
>>> +            .sector_num = sector_num,
>>> +            .nb_sectors = nb_sectors,
>>> +            .pnum = pnum,
>>> +            .done = false,
>>> +        };
>>> +
>>> +        co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
>>> +        qemu_coroutine_enter(co, &data);
>> Since this main process will stop within qemu_coroutine_enter() until
>> bdrv_is_allocated_co_entry() is completed, three lines of condition
>> codes below are unnecessary, right?
>
> No, they are needed. We want to wait until the whole operation has
> completed, but qemu_coroutine_enter() returns after the first
> qemu_coroutine_yield().
got it. thanks.
>
> Kevin
>



-- 
Regards,

Zhi Yong Wu

[Qemu-devel] [PATCH 03/10] qed: convert to .bdrv_co_is_allocated()

[Stefan Hajnoczi]

The bdrv_qed_is_allocated() function is a synchronous wrapper around
qed_find_cluster(), which performs the cluster lookup.  In order to
convert the synchronous function to a coroutine function we yield
instead of using qemu_aio_wait().  Note that QED's cache is already safe
for parallel requests so no locking is needed.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block/qed.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index d032a45..8a05445 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -651,6 +651,7 @@ static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options)
 }
 
 typedef struct {
+    Coroutine *co;
     int is_allocated;
     int *pnum;
 } QEDIsAllocatedCB;
@@ -660,10 +661,14 @@ static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t l
     QEDIsAllocatedCB *cb = opaque;
     *cb->pnum = len / BDRV_SECTOR_SIZE;
     cb->is_allocated = (ret == QED_CLUSTER_FOUND || ret == QED_CLUSTER_ZERO);
+    if (cb->co) {
+        qemu_coroutine_enter(cb->co, NULL);
+    }
 }
 
-static int bdrv_qed_is_allocated(BlockDriverState *bs, int64_t sector_num,
-                                  int nb_sectors, int *pnum)
+static int coroutine_fn bdrv_qed_co_is_allocated(BlockDriverState *bs,
+                                                 int64_t sector_num,
+                                                 int nb_sectors, int *pnum)
 {
     BDRVQEDState *s = bs->opaque;
     uint64_t pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE;
@@ -676,8 +681,10 @@ static int bdrv_qed_is_allocated(BlockDriverState *bs, int64_t sector_num,
 
     qed_find_cluster(s, &request, pos, len, qed_is_allocated_cb, &cb);
 
+    /* Now sleep if the callback wasn't invoked immediately */
     while (cb.is_allocated == -1) {
-        qemu_aio_wait();
+        cb.co = qemu_coroutine_self();
+        qemu_coroutine_yield();
     }
 
     qed_unref_l2_cache_entry(request.l2_table);
@@ -1475,7 +1482,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_open                = bdrv_qed_open,
     .bdrv_close               = bdrv_qed_close,
     .bdrv_create              = bdrv_qed_create,
-    .bdrv_is_allocated        = bdrv_qed_is_allocated,
+    .bdrv_co_is_allocated     = bdrv_qed_co_is_allocated,
     .bdrv_make_empty          = bdrv_qed_make_empty,
     .bdrv_aio_readv           = bdrv_qed_aio_readv,
     .bdrv_aio_writev          = bdrv_qed_aio_writev,
-- 
1.7.7.1

[Qemu-devel] [PATCH 04/10] block: convert qcow2, qcow2, and vmdk to .bdrv_co_is_allocated()

[Stefan Hajnoczi]

The qcow2, qcow, and vmdk block drivers are based on coroutines.  They have a
coroutine mutex which protects internal state.  We can convert the
.bdrv_is_allocated() function to .bdrv_co_is_allocated() by holding the mutex
around the cluster lookup operation.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block/qcow.c  |    8 +++++---
 block/qcow2.c |   13 ++++++++-----
 block/vmdk.c  |    8 +++++---
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 35e21eb..9e57440 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -360,14 +360,16 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
     return cluster_offset;
 }
 
-static int qcow_is_allocated(BlockDriverState *bs, int64_t sector_num,
-                             int nb_sectors, int *pnum)
+static int coroutine_fn qcow_co_is_allocated(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, int *pnum)
 {
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster, n;
     uint64_t cluster_offset;
 
+    qemu_co_mutex_lock(&s->lock);
     cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
+    qemu_co_mutex_unlock(&s->lock);
     index_in_cluster = sector_num & (s->cluster_sectors - 1);
     n = s->cluster_sectors - index_in_cluster;
     if (n > nb_sectors)
@@ -828,7 +830,7 @@ static BlockDriver bdrv_qcow = {
     .bdrv_open		= qcow_open,
     .bdrv_close		= qcow_close,
     .bdrv_create	= qcow_create,
-    .bdrv_is_allocated	= qcow_is_allocated,
+    .bdrv_co_is_allocated = qcow_co_is_allocated,
     .bdrv_set_key	= qcow_set_key,
     .bdrv_make_empty	= qcow_make_empty,
     .bdrv_co_readv      = qcow_co_readv,
diff --git a/block/qcow2.c b/block/qcow2.c
index ef057d3..c818c7f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -342,16 +342,19 @@ static int qcow2_set_key(BlockDriverState *bs, const char *key)
     return 0;
 }
 
-static int qcow2_is_allocated(BlockDriverState *bs, int64_t sector_num,
-                              int nb_sectors, int *pnum)
+static int coroutine_fn qcow2_co_is_allocated(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, int *pnum)
 {
+    BDRVQcowState *s = bs->opaque;
     uint64_t cluster_offset;
     int ret;
 
     *pnum = nb_sectors;
-    /* FIXME We can get errors here, but the bdrv_is_allocated interface can't
-     * pass them on today */
+    /* FIXME We can get errors here, but the bdrv_co_is_allocated interface
+     * can't pass them on today */
+    qemu_co_mutex_lock(&s->lock);
     ret = qcow2_get_cluster_offset(bs, sector_num << 9, pnum, &cluster_offset);
+    qemu_co_mutex_unlock(&s->lock);
     if (ret < 0) {
         *pnum = 0;
     }
@@ -1239,7 +1242,7 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_open          = qcow2_open,
     .bdrv_close         = qcow2_close,
     .bdrv_create        = qcow2_create,
-    .bdrv_is_allocated  = qcow2_is_allocated,
+    .bdrv_co_is_allocated = qcow2_co_is_allocated,
     .bdrv_set_key       = qcow2_set_key,
     .bdrv_make_empty    = qcow2_make_empty,
 
diff --git a/block/vmdk.c b/block/vmdk.c
index 985006e..2e62e97 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -852,8 +852,8 @@ static VmdkExtent *find_extent(BDRVVmdkState *s,
     return NULL;
 }
 
-static int vmdk_is_allocated(BlockDriverState *bs, int64_t sector_num,
-                             int nb_sectors, int *pnum)
+static int coroutine_fn vmdk_co_is_allocated(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, int *pnum)
 {
     BDRVVmdkState *s = bs->opaque;
     int64_t index_in_cluster, n, ret;
@@ -864,8 +864,10 @@ static int vmdk_is_allocated(BlockDriverState *bs, int64_t sector_num,
     if (!extent) {
         return 0;
     }
+    qemu_co_mutex_lock(&s->lock);
     ret = get_cluster_offset(bs, extent, NULL,
                             sector_num * 512, 0, &offset);
+    qemu_co_mutex_unlock(&s->lock);
     /* get_cluster_offset returning 0 means success */
     ret = !ret;
 
@@ -1582,7 +1584,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_close     = vmdk_close,
     .bdrv_create    = vmdk_create,
     .bdrv_co_flush  = vmdk_co_flush,
-    .bdrv_is_allocated  = vmdk_is_allocated,
+    .bdrv_co_is_allocated  = vmdk_co_is_allocated,
     .bdrv_get_allocated_file_size  = vmdk_get_allocated_file_size,
 
     .create_options = vmdk_create_options,
-- 
1.7.7.1

[Qemu-devel] [PATCH 05/10] vvfat: use public block layer interface

[Stefan Hajnoczi]

For some reason vvfat.c invokes BlockDriver functions directly and does
not go through block.h functions.  Change all direct calls except for
the .bdrv_make_empty() call for which there is no public interface.

This change makes the conversion to .bdrv_co_is_allocated() possible.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block/vvfat.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 8511fe5..ce8049f 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1254,10 +1254,9 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
 	   return -1;
 	if (s->qcow) {
 	    int n;
-	    if (s->qcow->drv->bdrv_is_allocated(s->qcow,
-			sector_num, nb_sectors-i, &n)) {
+	    if (bdrv_is_allocated(s->qcow, sector_num, nb_sectors-i, &n)) {
 DLOG(fprintf(stderr, "sectors %d+%d allocated\n", (int)sector_num, n));
-		if (s->qcow->drv->bdrv_read(s->qcow, sector_num, buf+i*0x200, n))
+		if (bdrv_read(s->qcow, sector_num, buf+i*0x200, n))
 		    return -1;
 		i += n - 1;
 		sector_num += n - 1;
@@ -1516,7 +1515,7 @@ static inline int cluster_was_modified(BDRVVVFATState* s, uint32_t cluster_num)
 	return 0;
 
     for (i = 0; !was_modified && i < s->sectors_per_cluster; i++)
-	was_modified = s->qcow->drv->bdrv_is_allocated(s->qcow,
+	was_modified = bdrv_is_allocated(s->qcow,
 		cluster2sector(s, cluster_num) + i, 1, &dummy);
 
     return was_modified;
@@ -1666,13 +1665,11 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
 
 		vvfat_close_current_file(s);
 		for (i = 0; i < s->sectors_per_cluster; i++)
-		    if (!s->qcow->drv->bdrv_is_allocated(s->qcow,
-				offset + i, 1, &dummy)) {
+		    if (!bdrv_is_allocated(s->qcow, offset + i, 1, &dummy)) {
 			if (vvfat_read(s->bs,
 				    offset, s->cluster_buffer, 1))
 			    return -1;
-			if (s->qcow->drv->bdrv_write(s->qcow,
-				    offset, s->cluster_buffer, 1))
+			if (bdrv_write(s->qcow, offset, s->cluster_buffer, 1))
 			    return -2;
 		    }
 	    }
@@ -2714,7 +2711,7 @@ DLOG(checkpoint());
      * Use qcow backend. Commit later.
      */
 DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors));
-    ret = s->qcow->drv->bdrv_write(s->qcow, sector_num, buf, nb_sectors);
+    ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors);
     if (ret < 0) {
 	fprintf(stderr, "Error writing to qcow backend\n");
 	return ret;
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH 05/10] vvfat: use public block layer interface

[Kevin Wolf]

Am 11.11.2011 17:47, schrieb Stefan Hajnoczi:
> For some reason vvfat.c invokes BlockDriver functions directly and does
> not go through block.h functions.  Change all direct calls except for
> the .bdrv_make_empty() call for which there is no public interface.
> 
> This change makes the conversion to .bdrv_co_is_allocated() possible.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

This conflicts with the block branch (I made more or less the same
change in order to fix read-write vvfat disks)

Kevin

Re: [Qemu-devel] [PATCH 05/10] vvfat: use public block layer interface

[Stefan Hajnoczi]

On Fri, Nov 11, 2011 at 05:59:19PM +0100, Kevin Wolf wrote:
> Am 11.11.2011 17:47, schrieb Stefan Hajnoczi:
> > For some reason vvfat.c invokes BlockDriver functions directly and does
> > not go through block.h functions.  Change all direct calls except for
> > the .bdrv_make_empty() call for which there is no public interface.
> > 
> > This change makes the conversion to .bdrv_co_is_allocated() possible.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> 
> This conflicts with the block branch (I made more or less the same
> change in order to fix read-write vvfat disks)

Okay, I'll rebase on your block branch.

Stefan

[Qemu-devel] [PATCH 06/10] vvfat: convert to .bdrv_co_is_allocated()

[Stefan Hajnoczi]

It is trivial to switch from the synchronous .bdrv_is_allocated()
interface to .bdrv_co_is_allocated() since vvfat_is_allocated() does not
block.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block/vvfat.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index ce8049f..f8d62d4 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2741,7 +2741,7 @@ static coroutine_fn int vvfat_co_write(BlockDriverState *bs, int64_t sector_num,
     return ret;
 }
 
-static int vvfat_is_allocated(BlockDriverState *bs,
+static int coroutine_fn vvfat_co_is_allocated(BlockDriverState *bs,
 	int64_t sector_num, int nb_sectors, int* n)
 {
     BDRVVVFATState* s = bs->opaque;
@@ -2833,7 +2833,7 @@ static BlockDriver bdrv_vvfat = {
     .bdrv_read          = vvfat_co_read,
     .bdrv_write         = vvfat_co_write,
     .bdrv_close		= vvfat_close,
-    .bdrv_is_allocated	= vvfat_is_allocated,
+    .bdrv_co_is_allocated = vvfat_co_is_allocated,
     .protocol_name	= "fat",
 };
 
-- 
1.7.7.1

[Qemu-devel] [PATCH 07/10] vdi: convert to .bdrv_co_is_allocated()

[Stefan Hajnoczi]

It is trivial to switch from the synchronous .bdrv_is_allocated()
interface to .bdrv_co_is_allocated() since vdi_is_allocated() does not
block.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block/vdi.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 523a640..1f2bc2c 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -463,8 +463,8 @@ static int vdi_open(BlockDriverState *bs, int flags)
     return -1;
 }
 
-static int vdi_is_allocated(BlockDriverState *bs, int64_t sector_num,
-                             int nb_sectors, int *pnum)
+static int coroutine_fn vdi_co_is_allocated(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, int *pnum)
 {
     /* TODO: Check for too large sector_num (in bdrv_is_allocated or here). */
     BDRVVdiState *s = (BDRVVdiState *)bs->opaque;
@@ -981,7 +981,7 @@ static BlockDriver bdrv_vdi = {
     .bdrv_close = vdi_close,
     .bdrv_create = vdi_create,
     .bdrv_co_flush = vdi_co_flush,
-    .bdrv_is_allocated = vdi_is_allocated,
+    .bdrv_co_is_allocated = vdi_co_is_allocated,
     .bdrv_make_empty = vdi_make_empty,
 
     .bdrv_aio_readv = vdi_aio_readv,
-- 
1.7.7.1

[Qemu-devel] [PATCH 08/10] cow: convert to .bdrv_co_is_allocated()

[Stefan Hajnoczi]

The cow block driver does not keep internal state for cluster lookups.
This means it is safe to perform cluster lookups in coroutine context
without risk of race conditions that corrupt internal state.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block/cow.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/cow.c b/block/cow.c
index 707c0aa..9ab21f6 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -132,8 +132,8 @@ static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
 /* Return true if first block has been changed (ie. current version is
  * in COW file).  Set the number of continuous blocks for which that
  * is true. */
-static int cow_is_allocated(BlockDriverState *bs, int64_t sector_num,
-        int nb_sectors, int *num_same)
+static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, int *num_same)
 {
     int changed;
 
@@ -178,7 +178,7 @@ static int cow_read(BlockDriverState *bs, int64_t sector_num,
     int ret, n;
 
     while (nb_sectors > 0) {
-        if (cow_is_allocated(bs, sector_num, nb_sectors, &n)) {
+        if (bdrv_is_allocated(bs, sector_num, nb_sectors, &n)) {
             ret = bdrv_pread(bs->file,
                         s->cow_sectors_offset + sector_num * 512,
                         buf, n * 512);
@@ -335,7 +335,7 @@ static BlockDriver bdrv_cow = {
     .bdrv_close		= cow_close,
     .bdrv_create	= cow_create,
     .bdrv_co_flush      = cow_co_flush,
-    .bdrv_is_allocated	= cow_is_allocated,
+    .bdrv_co_is_allocated = cow_co_is_allocated,
 
     .create_options = cow_create_options,
 };
-- 
1.7.7.1

[Qemu-devel] [PATCH 09/10] block: drop .bdrv_is_allocated() interface

[Stefan Hajnoczi]

Now that all block drivers have been converted to
.bdrv_co_is_allocated() we can drop .bdrv_is_allocated().

Note that the public bdrv_is_allocated() interface is still available
but is in fact a synchronous wrapper around .bdrv_co_is_allocated().

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c     |   38 ++++++++++++++++++--------------------
 block_int.h |    2 --
 2 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index f8705b7..082734b 100644
--- a/block.c
+++ b/block.c
@@ -1805,25 +1805,8 @@ static void coroutine_fn bdrv_is_allocated_co_entry(void *opaque)
 int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
 	int *pnum)
 {
-    int64_t n;
-    if (bs->drv->bdrv_co_is_allocated) {
-        Coroutine *co;
-        BdrvCoIsAllocatedData data = {
-            .bs = bs,
-            .sector_num = sector_num,
-            .nb_sectors = nb_sectors,
-            .pnum = pnum,
-            .done = false,
-        };
-
-        co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
-        qemu_coroutine_enter(co, &data);
-        while (!data.done) {
-            qemu_aio_wait();
-        }
-        return data.ret;
-    }
-    if (!bs->drv->bdrv_is_allocated) {
+    if (!bs->drv->bdrv_co_is_allocated) {
+        int64_t n;
         if (sector_num >= bs->total_sectors) {
             *pnum = 0;
             return 0;
@@ -1832,7 +1815,22 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
         *pnum = (n < nb_sectors) ? (n) : (nb_sectors);
         return 1;
     }
-    return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
+
+    Coroutine *co;
+    BdrvCoIsAllocatedData data = {
+        .bs = bs,
+        .sector_num = sector_num,
+        .nb_sectors = nb_sectors,
+        .pnum = pnum,
+        .done = false,
+    };
+
+    co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
+    qemu_coroutine_enter(co, &data);
+    while (!data.done) {
+        qemu_aio_wait();
+    }
+    return data.ret;
 }
 
 void bdrv_mon_event(const BlockDriverState *bdrv,
diff --git a/block_int.h b/block_int.h
index 1c1351c..821237b 100644
--- a/block_int.h
+++ b/block_int.h
@@ -63,8 +63,6 @@ struct BlockDriver {
                       const uint8_t *buf, int nb_sectors);
     void (*bdrv_close)(BlockDriverState *bs);
     int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
-    int (*bdrv_is_allocated)(BlockDriverState *bs, int64_t sector_num,
-                             int nb_sectors, int *pnum);
     int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
     int (*bdrv_make_empty)(BlockDriverState *bs);
     /* aio */
-- 
1.7.7.1

[Qemu-devel] [PATCH 10/10] block: add bdrv_co_is_allocated() interface

[Stefan Hajnoczi]

This patch introduce the public bdrv_co_is_allocated() interface which
can be used to query image allocation status while the VM is running.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |   37 ++++++++++++++++++++++++-------------
 block.h |    2 ++
 2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 082734b..83bdfb6 100644
--- a/block.c
+++ b/block.c
@@ -1780,17 +1780,6 @@ typedef struct BdrvCoIsAllocatedData {
     bool done;
 } BdrvCoIsAllocatedData;
 
-/* Coroutine wrapper for bdrv_is_allocated() */
-static void coroutine_fn bdrv_is_allocated_co_entry(void *opaque)
-{
-    BdrvCoIsAllocatedData *data = opaque;
-    BlockDriverState *bs = data->bs;
-
-    data->ret = bs->drv->bdrv_co_is_allocated(bs, data->sector_num,
-                                              data->nb_sectors, data->pnum);
-    data->done = true;
-}
-
 /*
  * Returns true iff the specified sector is present in the disk image. Drivers
  * not implementing the functionality are assumed to not support backing files,
@@ -1802,8 +1791,8 @@ static void coroutine_fn bdrv_is_allocated_co_entry(void *opaque)
  *
  * 'nb_sectors' is the max value 'pnum' should be set to.
  */
-int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
-	int *pnum)
+int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs, int64_t sector_num,
+                                      int nb_sectors, int *pnum)
 {
     if (!bs->drv->bdrv_co_is_allocated) {
         int64_t n;
@@ -1816,6 +1805,28 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
         return 1;
     }
 
+    return bs->drv->bdrv_co_is_allocated(bs, sector_num, nb_sectors, pnum);
+}
+
+/* Coroutine wrapper for bdrv_is_allocated() */
+static void coroutine_fn bdrv_is_allocated_co_entry(void *opaque)
+{
+    BdrvCoIsAllocatedData *data = opaque;
+    BlockDriverState *bs = data->bs;
+
+    data->ret = bdrv_co_is_allocated(bs, data->sector_num, data->nb_sectors,
+                                     data->pnum);
+    data->done = true;
+}
+
+/*
+ * Synchronous wrapper around bdrv_co_is_allocated().
+ *
+ * See bdrv_co_is_allocated() for details.
+ */
+int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
+                      int *pnum)
+{
     Coroutine *co;
     BdrvCoIsAllocatedData data = {
         .bs = bs,
diff --git a/block.h b/block.h
index 38cd748..c459fad 100644
--- a/block.h
+++ b/block.h
@@ -128,6 +128,8 @@ int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
     int nb_sectors, QEMUIOVector *qiov);
 int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
     int nb_sectors, QEMUIOVector *qiov);
+int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs, int64_t sector_num,
+    int nb_sectors, int *pnum);
 int bdrv_truncate(BlockDriverState *bs, int64_t offset);
 int64_t bdrv_getlength(BlockDriverState *bs);
 int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
-- 
1.7.7.1