Patchwork [02/10] block: add .bdrv_co_is_allocated()

login
register
mail settings
Submitter Stefan Hajnoczi
Date Nov. 11, 2011, 4:47 p.m.
Message ID <1321030042-20446-3-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/125248/
State New
Headers show

Comments

Stefan Hajnoczi - Nov. 11, 2011, 4:47 p.m.
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(-)
Zhiyong Wu - Nov. 14, 2011, 3:04 a.m.
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
>
>
>
Paolo Bonzini - Nov. 14, 2011, 7:32 a.m.
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
Zhiyong Wu - Nov. 14, 2011, 8:10 a.m.
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
>
>
>
Paolo Bonzini - Nov. 14, 2011, 8:37 a.m.
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
Kevin Wolf - Nov. 14, 2011, 8:50 a.m.
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
Zhiyong Wu - Nov. 14, 2011, 8:50 a.m.
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
>
Zhiyong Wu - Nov. 14, 2011, 8:53 a.m.
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
>

Patch

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