[Qemu-devel] [PATCH 0/8] finish coroutinization of drivers

[Qemu-devel] [PATCH 0/8] finish coroutinization of drivers

[Paolo Bonzini]

Drivers that only implement the bdrv_read and bdrv_write callbacks
were unwillingly converted to be reentrant when bdrv_read and
bdrv_write were changed to always create coroutines.  So,
we need locks aroudn read and write operations.

This series does this (patches 4-6) and removes the flush/discard
callbacks that, as it turns out, are really duplicates of co_flush
and co_discard (patches 7-8).

Patches 1-3 are fixes/cleanups that I discovered while testing.


Paolo Bonzini (8):
  vpc: detect floppy disk geometries
  vmdk: fix return values of vmdk_parent_open
  vmdk: clean up open
  block: add a Rwlock to synchronous read/write drivers
  block: take lock around bdrv_read implementations
  block: take lock around bdrv_write implementations
  block: change flush to co_flush
  block: change discard to co_discard

 block.c           |    4 ---
 block/cow.c       |   34 +++++++++++++++++++---
 block/nbd.c       |   28 +++++++++++++++++-
 block/qcow.c      |   11 +++----
 block/qcow2.c     |   26 +++++++++++-------
 block/raw-posix.c |    4 +-
 block/raw-win32.c |    4 +-
 block/rbd.c       |    4 +-
 block/vdi.c       |    6 ++--
 block/vmdk.c      |   78 ++++++++++++++++++++++++++++++++--------------------
 block/vpc.c       |   42 +++++++++++++++++++++++++---
 block/vvfat.c     |   28 +++++++++++++++++-
 block_int.h       |    3 --
 13 files changed, 196 insertions(+), 76 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH 1/8] vpc: detect floppy disk geometries

[Paolo Bonzini]

Converting a floppy image from RAW to VPC and back will generate
a zero-padded file of the wrong size, because the geometry is not
computed correctly.  Special case floppy disk images, handling
standard MS-DOS capacities (160/180/320/360 for low density
5.25" disks, 1200 for high density 5.25" disks, 720/1440/2880
for 3.5" disks).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vpc.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index cb6c570..549a632 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -463,6 +463,14 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
 {
     uint32_t cyls_times_heads;
 
+    if (total_sectors <= 5760) {
+        /* Floppy disk geometry */
+        *heads = total_sectors < 640 ? 1 : 2; /* 1 = single side 5.25" */
+        *cyls = total_sectors < 1440 ? 40 : 80;  /* 40 = low density 5.25" */
+        *secs_per_cyl = total_sectors / *heads / *cyls;
+        return 0;
+    }
+
     if (total_sectors > 65535 * 16 * 255)
         return -EFBIG;
 
-- 
1.7.6

Re: [Qemu-devel] [PATCH 1/8] vpc: detect floppy disk geometries

[Kevin Wolf]

Am 19.10.2011 16:59, schrieb Paolo Bonzini:
> Converting a floppy image from RAW to VPC and back will generate
> a zero-padded file of the wrong size, because the geometry is not
> computed correctly.  Special case floppy disk images, handling
> standard MS-DOS capacities (160/180/320/360 for low density
> 5.25" disks, 1200 for high density 5.25" disks, 720/1440/2880
> for 3.5" disks).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Unrelated to coroutines, isn't it?

> ---
>  block/vpc.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index cb6c570..549a632 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -463,6 +463,14 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
>  {
>      uint32_t cyls_times_heads;
>  
> +    if (total_sectors <= 5760) {
> +        /* Floppy disk geometry */

Please state in the comment that this part is deviating from the
algorithm in the VHD spec, which this function is generally supposed to
implement.

Kevin

Re: [Qemu-devel] [PATCH 1/8] vpc: detect floppy disk geometries

[Paolo Bonzini]

On 10/20/2011 11:14 AM, Kevin Wolf wrote:
>> >  diff --git a/block/vpc.c b/block/vpc.c
>> >  index cb6c570..549a632 100644
>> >  --- a/block/vpc.c
>> >  +++ b/block/vpc.c
>> >  @@ -463,6 +463,14 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
>> >    {
>> >        uint32_t cyls_times_heads;
>> >
>> >  +    if (total_sectors<= 5760) {
>> >  +        /* Floppy disk geometry */
> Please state in the comment that this part is deviating from the
> algorithm in the VHD spec, which this function is generally supposed to
> implement.

Hmm, looks like virtual PC uses a separate "VFD" image for floppies, so 
I can drop this.

Paolo

[Qemu-devel] [PATCH 2/8] vmdk: fix return values of vmdk_parent_open

[Paolo Bonzini]

While vmdk_open_desc_file (touched by the patch) correctly changed -1
to -EINVAL, vmdk_open did not.  Fix it directly in vmdk_parent_open.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vmdk.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 5d16ec4..21566eb 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -286,7 +286,7 @@ static int vmdk_parent_open(BlockDriverState *bs)
 
     desc[DESC_SIZE] = '\0';
     if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) {
-        return -1;
+        return -EINVAL;
     }
 
     p_name = strstr(desc, "parentFileNameHint");
@@ -296,10 +296,10 @@ static int vmdk_parent_open(BlockDriverState *bs)
         p_name += sizeof("parentFileNameHint") + 1;
         end_name = strchr(p_name, '\"');
         if (end_name == NULL) {
-            return -1;
+            return -EINVAL;
         }
         if ((end_name - p_name) > sizeof(bs->backing_file) - 1) {
-            return -1;
+            return -EINVAL;
         }
 
         pstrcpy(bs->backing_file, end_name - p_name + 1, p_name);
@@ -629,9 +629,10 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
     }
 
     /* try to open parent images, if exist */
-    if (vmdk_parent_open(bs)) {
+    ret = vmdk_parent_open(bs);
+    if (ret) {
         vmdk_free_extents(bs);
-        return -EINVAL;
+        return ret;
     }
     s->parent_cid = vmdk_read_cid(bs, 1);
     return 0;
-- 
1.7.6

Re: [Qemu-devel] [PATCH 2/8] vmdk: fix return values of vmdk_parent_open

[Kevin Wolf]

Am 19.10.2011 16:59, schrieb Paolo Bonzini:
> While vmdk_open_desc_file (touched by the patch) correctly changed -1
> to -EINVAL, vmdk_open did not.  Fix it directly in vmdk_parent_open.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/vmdk.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 5d16ec4..21566eb 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -286,7 +286,7 @@ static int vmdk_parent_open(BlockDriverState *bs)
>  
>      desc[DESC_SIZE] = '\0';
>      if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) {
> -        return -1;
> +        return -EINVAL;
>      }

ret = bdrv_pread(...);
if (ret < 0) {
    return ret;
}

Kevin

[Qemu-devel] [PATCH 3/8] vmdk: clean up open

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vmdk.c |   37 +++++++++++++++----------------------
 1 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 21566eb..12b38d2 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -622,20 +622,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
         return -ENOTSUP;
     }
     s->desc_offset = 0;
-    ret = vmdk_parse_extents(buf, bs, bs->file->filename);
-    if (ret) {
-        vmdk_free_extents(bs);
-        return ret;
-    }
-
-    /* try to open parent images, if exist */
-    ret = vmdk_parent_open(bs);
-    if (ret) {
-        vmdk_free_extents(bs);
-        return ret;
-    }
-    s->parent_cid = vmdk_read_cid(bs, 1);
-    return 0;
+    return vmdk_parse_extents(buf, bs, bs->file->filename);
 }
 
 static int vmdk_open(BlockDriverState *bs, int flags)
@@ -645,17 +632,23 @@ static int vmdk_open(BlockDriverState *bs, int flags)
 
     if (vmdk_open_sparse(bs, bs->file, flags) == 0) {
         s->desc_offset = 0x200;
-        /* try to open parent images, if exist */
-        ret = vmdk_parent_open(bs);
+    } else {
+        ret = vmdk_open_desc_file(bs, flags, 0);
         if (ret) {
-            vmdk_free_extents(bs);
-            return ret;
+            goto fail;
         }
-        s->parent_cid = vmdk_read_cid(bs, 1);
-        return 0;
-    } else {
-        return vmdk_open_desc_file(bs, flags, 0);
     }
+    /* try to open parent images, if exist */
+    ret = vmdk_parent_open(bs);
+    if (ret) {
+        goto fail;
+    }
+    s->parent_cid = vmdk_read_cid(bs, 1);
+    return ret;
+
+fail:
+    vmdk_free_extents(bs);
+    return ret;
 }
 
 static int get_whole_cluster(BlockDriverState *bs,
-- 
1.7.6

Re: [Qemu-devel] [PATCH 3/8] vmdk: clean up open

[Kevin Wolf]

Am 19.10.2011 16:59, schrieb Paolo Bonzini:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/vmdk.c |   37 +++++++++++++++----------------------
>  1 files changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 21566eb..12b38d2 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -622,20 +622,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
>          return -ENOTSUP;
>      }
>      s->desc_offset = 0;
> -    ret = vmdk_parse_extents(buf, bs, bs->file->filename);
> -    if (ret) {
> -        vmdk_free_extents(bs);
> -        return ret;
> -    }
> -
> -    /* try to open parent images, if exist */
> -    ret = vmdk_parent_open(bs);
> -    if (ret) {
> -        vmdk_free_extents(bs);
> -        return ret;
> -    }
> -    s->parent_cid = vmdk_read_cid(bs, 1);
> -    return 0;
> +    return vmdk_parse_extents(buf, bs, bs->file->filename);
>  }

This code is moved into bdrv_open, but there's another path how this
code can be reached:

vmdk_parse_extents() -> vmdk_open_sparse() -> vmdk_open_vmdk4() ->
vmdk_open_desc_file().

Don't we forget to open the parent file there now?

Kevin

>  
>  static int vmdk_open(BlockDriverState *bs, int flags)
> @@ -645,17 +632,23 @@ static int vmdk_open(BlockDriverState *bs, int flags)
>  
>      if (vmdk_open_sparse(bs, bs->file, flags) == 0) {
>          s->desc_offset = 0x200;
> -        /* try to open parent images, if exist */
> -        ret = vmdk_parent_open(bs);
> +    } else {
> +        ret = vmdk_open_desc_file(bs, flags, 0);
>          if (ret) {
> -            vmdk_free_extents(bs);
> -            return ret;
> +            goto fail;
>          }
> -        s->parent_cid = vmdk_read_cid(bs, 1);
> -        return 0;
> -    } else {
> -        return vmdk_open_desc_file(bs, flags, 0);
>      }
> +    /* try to open parent images, if exist */
> +    ret = vmdk_parent_open(bs);
> +    if (ret) {
> +        goto fail;
> +    }
> +    s->parent_cid = vmdk_read_cid(bs, 1);
> +    return ret;
> +
> +fail:
> +    vmdk_free_extents(bs);
> +    return ret;
>  }
>  
>  static int get_whole_cluster(BlockDriverState *bs,

Re: [Qemu-devel] [PATCH 3/8] vmdk: clean up open

[Paolo Bonzini]

On 10/20/2011 11:28 AM, Kevin Wolf wrote:
> This code is moved into bdrv_open, but there's another path how this
> code can be reached:
> 
> vmdk_parse_extents() ->  vmdk_open_sparse() ->  vmdk_open_vmdk4() ->
> vmdk_open_desc_file().
> 
> Don't we forget to open the parent file there now?

Let's look at the call chain:

vmdk_open:
        return vmdk_open_desc_file(bs, flags, 0);

-> vmdk_open_desc_file:
    ret = vmdk_parse_extents(buf, bs, bs->file->filename);

-> vmdk_parse_extents:

p points into vmdk_open_desc_file's buf:

        ret = sscanf(p, "%10s %" SCNd64 " %10s %511s %" SCNd64,
                access, &sectors, type, fname, &flat_offset);
        path_combine(extent_path, sizeof(extent_path),
                desc_file_path, fname);
        ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
            ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);

So here it finds the sub-vmdk, opens it and passes it to vmdk_open_sparse.

-> vmdk_open_sparse:
            return vmdk_open_vmdk4(bs, file, flags);

The extent_file is then passed to vmdk_open_vmdk4:

-> vmdk_open_vmdk4:
    ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
        return vmdk_open_desc_file(bs, flags, header.desc_offset << 9);

This reads the header from extent_file, but vmdk_open_desc_file will
still read from bs->file.

-> vmdk_open_desc_file:
    s->desc_offset = 0;
    if (vmdk_parent_open(bs)) {
-> vmdk_parent_open:
    if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) {

So s->desc_offset is always zero, and bs->file never changes.  So the data
that vmdk_parent_open reads comes always from the same place, and anyway
there have only one place where it can write it:

vmdk_parent_open:
        pstrcpy(bs->backing_file, end_name - p_name + 1, p_name);

So the code is completely bogus in the case of recursive calls to
vmdk_open_desc_file with parent images.  Maybe it simply cannot happen,
and in that case the patched code is less bogus.

It is also possible that it can happen, and the bug is in setting
s->desc_offset to zero.  Even in that case, calling vmdk_parent_open
once would be correct and the patched code is less bogus.

Paolo

Re: [Qemu-devel] [PATCH 3/8] vmdk: clean up open

[Kevin Wolf]

Am 20.10.2011 12:12, schrieb Paolo Bonzini:
> On 10/20/2011 11:28 AM, Kevin Wolf wrote:
>> This code is moved into bdrv_open, but there's another path how this
>> code can be reached:
>>
>> vmdk_parse_extents() ->  vmdk_open_sparse() ->  vmdk_open_vmdk4() ->
>> vmdk_open_desc_file().
>>
>> Don't we forget to open the parent file there now?
> 
> Let's look at the call chain:
> 
> vmdk_open:
>         return vmdk_open_desc_file(bs, flags, 0);
> 
> -> vmdk_open_desc_file:
>     ret = vmdk_parse_extents(buf, bs, bs->file->filename);
> 
> -> vmdk_parse_extents:
> 
> p points into vmdk_open_desc_file's buf:
> 
>         ret = sscanf(p, "%10s %" SCNd64 " %10s %511s %" SCNd64,
>                 access, &sectors, type, fname, &flat_offset);
>         path_combine(extent_path, sizeof(extent_path),
>                 desc_file_path, fname);
>         ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags);
>             ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);
> 
> So here it finds the sub-vmdk, opens it and passes it to vmdk_open_sparse.
> 
> -> vmdk_open_sparse:
>             return vmdk_open_vmdk4(bs, file, flags);
> 
> The extent_file is then passed to vmdk_open_vmdk4:
> 
> -> vmdk_open_vmdk4:
>     ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>         return vmdk_open_desc_file(bs, flags, header.desc_offset << 9);
> 
> This reads the header from extent_file, but vmdk_open_desc_file will
> still read from bs->file.
> -> vmdk_open_desc_file:
>     s->desc_offset = 0;
>     if (vmdk_parent_open(bs)) {
> -> vmdk_parent_open:
>     if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) {
> 
> So s->desc_offset is always zero, and bs->file never changes.  So the data
> that vmdk_parent_open reads comes always from the same place, and anyway
> there have only one place where it can write it:
> 
> vmdk_parent_open:
>         pstrcpy(bs->backing_file, end_name - p_name + 1, p_name);
> 
> So the code is completely bogus in the case of recursive calls to
> vmdk_open_desc_file with parent images.  Maybe it simply cannot happen,
> and in that case the patched code is less bogus.
> 
> It is also possible that it can happen, and the bug is in setting
> s->desc_offset to zero.  Even in that case, calling vmdk_parent_open
> once would be correct and the patched code is less bogus.

Ok, if it can't happen it seems reasonable and could still qualify as
cleanup. Maybe you can add a line or two to the commit message for v2.

Kevin

[Qemu-devel] [PATCH 4/8] block: add a Rwlock to synchronous read/write drivers

[Paolo Bonzini]

The big conversion of bdrv_read/write to coroutines caused the two
homonymous callbacks in BlockDriver to become reentrant.  It goes
like this:

1) bdrv_read is now called in a coroutine, and calls bdrv_read or
bdrv_pread.

2) the nested bdrv_read goes through the fast path in bdrv_rw_co_entry;

3) in the common case when the protocol is file, bdrv_co_do_readv calls
bdrv_co_readv_em (and from here goes to bdrv_co_io_em), which yields
until the AIO operation is complete;

4) if bdrv_read had been called from a bottom half, the main loop
is free to iterate again: a device model or another bottom half
can then come and call bdrv_read again.

This applies to all four of read/write/flush/discard.  It would also
apply to is_allocated, but it is not used from within coroutines:
besides qemu-img.c and qemu-io.c, which operate synchronously, the
only user is the monitor.  Copy-on-read will introduce a use in the
block layer, and will require converting it.

The solution is "simply" to convert all drivers to coroutines!  We
have nothing to do for the read-only drivers.  For the others, we
add a Rwlock that is taken around affected operations.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/cow.c   |    2 ++
 block/nbd.c   |    2 ++
 block/vmdk.c  |    2 ++
 block/vpc.c   |    2 ++
 block/vvfat.c |    2 ++
 5 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/block/cow.c b/block/cow.c
index 4cf543c..d27e0aa 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -42,6 +42,7 @@ struct cow_header_v2 {
 };
 
 typedef struct BDRVCowState {
+    CoRwlock lock;
     int64_t cow_sectors_offset;
 } BDRVCowState;
 
@@ -84,6 +85,7 @@ static int cow_open(BlockDriverState *bs, int flags)
 
     bitmap_size = ((bs->total_sectors + 7) >> 3) + sizeof(cow_header);
     s->cow_sectors_offset = (bitmap_size + 511) & ~511;
+    qemu_co_rwlock_init(&s->lock);
     return 0;
  fail:
     return -1;
diff --git a/block/nbd.c b/block/nbd.c
index 76f04d8..ec8f086 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -47,6 +47,7 @@
 #endif
 
 typedef struct BDRVNBDState {
+    CoRwlock lock;
     int sock;
     uint32_t nbdflags;
     off_t size;
@@ -175,6 +176,7 @@ static int nbd_open(BlockDriverState *bs, const char* filename, int flags)
      */
     result = nbd_establish_connection(bs);
 
+    qemu_co_rwlock_init(&s->lock);
     return result;
 }
 
diff --git a/block/vmdk.c b/block/vmdk.c
index 12b38d2..6afd53e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -90,6 +90,7 @@ typedef struct VmdkExtent {
 } VmdkExtent;
 
 typedef struct BDRVVmdkState {
+    CoRwlock lock;
     int desc_offset;
     bool cid_updated;
     uint32_t parent_cid;
@@ -644,6 +645,7 @@ static int vmdk_open(BlockDriverState *bs, int flags)
         goto fail;
     }
     s->parent_cid = vmdk_read_cid(bs, 1);
+    qemu_co_rwlock_init(&s->lock);
     return ret;
 
 fail:
diff --git a/block/vpc.c b/block/vpc.c
index 549a632..7220488 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -110,6 +110,7 @@ struct vhd_dyndisk_header {
 };
 
 typedef struct BDRVVPCState {
+    CoRwlock lock;
     uint8_t footer_buf[HEADER_SIZE];
     uint64_t free_data_block_offset;
     int max_table_entries;
@@ -226,6 +227,7 @@ static int vpc_open(BlockDriverState *bs, int flags)
     s->last_pagetable = -1;
 #endif
 
+    qemu_co_rwlock_init(&s->lock);
     return 0;
  fail:
     return err;
diff --git a/block/vvfat.c b/block/vvfat.c
index c567697..08a72ee 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -317,6 +317,7 @@ static void print_mapping(const struct mapping_t* mapping);
 /* here begins the real VVFAT driver */
 
 typedef struct BDRVVVFATState {
+    CoRwlock lock;
     BlockDriverState* bs; /* pointer to parent */
     unsigned int first_sectors_number; /* 1 for a single partition, 0x40 for a disk with partition table */
     unsigned char first_sectors[0x40*0x200];
@@ -1063,6 +1064,7 @@ DLOG(if (stderr == NULL) {
     }
 
     //    assert(is_consistent(s));
+    qemu_co_rwlock_init(&s->lock);
     return 0;
 }
 
-- 
1.7.6

Re: [Qemu-devel] [PATCH 4/8] block: add a Rwlock to synchronous read/write drivers

[Kevin Wolf]

Am 19.10.2011 16:59, schrieb Paolo Bonzini:
> The big conversion of bdrv_read/write to coroutines caused the two
> homonymous callbacks in BlockDriver to become reentrant.  It goes
> like this:
> 
> 1) bdrv_read is now called in a coroutine, and calls bdrv_read or
> bdrv_pread.
> 
> 2) the nested bdrv_read goes through the fast path in bdrv_rw_co_entry;
> 
> 3) in the common case when the protocol is file, bdrv_co_do_readv calls
> bdrv_co_readv_em (and from here goes to bdrv_co_io_em), which yields
> until the AIO operation is complete;
> 
> 4) if bdrv_read had been called from a bottom half, the main loop
> is free to iterate again: a device model or another bottom half
> can then come and call bdrv_read again.
> 
> This applies to all four of read/write/flush/discard.  It would also
> apply to is_allocated, but it is not used from within coroutines:
> besides qemu-img.c and qemu-io.c, which operate synchronously, the
> only user is the monitor.  Copy-on-read will introduce a use in the
> block layer, and will require converting it.
> 
> The solution is "simply" to convert all drivers to coroutines!  We
> have nothing to do for the read-only drivers.  For the others, we
> add a Rwlock that is taken around affected operations.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

For the record, we just discussed this on IRC: We'll probably need a
mutex in some/most cases instead. Also, cloop and dmg need locking. That
leaves only bochs and parallels that should be okay without a lock.

We decided to play it the safe way and add locking in every driver and
make it a mutex everywhere.

Kevin

[Qemu-devel] [PATCH 5/8] block: take lock around bdrv_read implementations

[Paolo Bonzini]

This does the first part of the conversion to coroutines, by
wrapping bdrv_read implementations to take the read side of the
rwlock.

Drivers that implement bdrv_read rather than bdrv_co_readv can
then benefit from asynchronous operation (at least if the underlying
protocol supports it, which is not the case for raw-win32), even
though they still operate with a bounce buffer.

raw-win32 does not need the lock, because it cannot yield.
nbd also doesn't probably, but better be safe.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/cow.c   |   13 ++++++++++++-
 block/nbd.c   |   13 ++++++++++++-
 block/vmdk.c  |   13 ++++++++++++-
 block/vpc.c   |   13 ++++++++++++-
 block/vvfat.c |   13 ++++++++++++-
 5 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/block/cow.c b/block/cow.c
index d27e0aa..9571549 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -201,6 +201,17 @@ static int cow_read(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
+static coroutine_fn int cow_co_read(BlockDriverState *bs, int64_t sector_num,
+                                    uint8_t *buf, int nb_sectors)
+{
+    int ret;
+    BDRVCowState *s = bs->opaque;
+    qemu_co_rwlock_rdlock(&s->lock);
+    ret = cow_read(bs, sector_num, buf, nb_sectors);
+    qemu_co_rwlock_unlock(&s->lock);
+    return ret;
+}
+
 static int cow_write(BlockDriverState *bs, int64_t sector_num,
                      const uint8_t *buf, int nb_sectors)
 {
@@ -308,7 +319,7 @@ static BlockDriver bdrv_cow = {
     .instance_size	= sizeof(BDRVCowState),
     .bdrv_probe		= cow_probe,
     .bdrv_open		= cow_open,
-    .bdrv_read		= cow_read,
+    .bdrv_read		= cow_co_read,
     .bdrv_write		= cow_write,
     .bdrv_close		= cow_close,
     .bdrv_create	= cow_create,
diff --git a/block/nbd.c b/block/nbd.c
index ec8f086..f8fed92 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -240,6 +240,17 @@ static int nbd_write(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
+static coroutine_fn int nbd_co_read(BlockDriverState *bs, int64_t sector_num,
+                                    uint8_t *buf, int nb_sectors)
+{
+    int ret;
+    BDRVNBDState *s = bs->opaque;
+    qemu_co_rwlock_rdlock(&s->lock);
+    ret = nbd_read(bs, sector_num, buf, nb_sectors);
+    qemu_co_rwlock_unlock(&s->lock);
+    return ret;
+}
+
 static void nbd_close(BlockDriverState *bs)
 {
     BDRVNBDState *s = bs->opaque;
@@ -260,7 +271,7 @@ static BlockDriver bdrv_nbd = {
     .format_name	= "nbd",
     .instance_size	= sizeof(BDRVNBDState),
     .bdrv_file_open	= nbd_open,
-    .bdrv_read		= nbd_read,
+    .bdrv_read		= nbd_co_read,
     .bdrv_write		= nbd_write,
     .bdrv_close		= nbd_close,
     .bdrv_getlength	= nbd_getlength,
diff --git a/block/vmdk.c b/block/vmdk.c
index 6afd53e..ff78e25 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1022,6 +1022,17 @@ static int vmdk_read(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
+static coroutine_fn int vmdk_co_read(BlockDriverState *bs, int64_t sector_num,
+                                     uint8_t *buf, int nb_sectors)
+{
+    int ret;
+    BDRVVmdkState *s = bs->opaque;
+    qemu_co_rwlock_rdlock(&s->lock);
+    ret = vmdk_read(bs, sector_num, buf, nb_sectors);
+    qemu_co_rwlock_unlock(&s->lock);
+    return ret;
+}
+
 static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
                      const uint8_t *buf, int nb_sectors)
 {
@@ -1540,7 +1551,7 @@ static BlockDriver bdrv_vmdk = {
     .instance_size  = sizeof(BDRVVmdkState),
     .bdrv_probe     = vmdk_probe,
     .bdrv_open      = vmdk_open,
-    .bdrv_read      = vmdk_read,
+    .bdrv_read      = vmdk_co_read,
     .bdrv_write     = vmdk_write,
     .bdrv_close     = vmdk_close,
     .bdrv_create    = vmdk_create,
diff --git a/block/vpc.c b/block/vpc.c
index 7220488..e805769 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -409,6 +409,17 @@ static int vpc_read(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
+static coroutine_fn int vpc_co_read(BlockDriverState *bs, int64_t sector_num,
+                                    uint8_t *buf, int nb_sectors)
+{
+    int ret;
+    BDRVVPCState *s = bs->opaque;
+    qemu_co_rwlock_rdlock(&s->lock);
+    ret = vpc_read(bs, sector_num, buf, nb_sectors);
+    qemu_co_rwlock_unlock(&s->lock);
+    return ret;
+}
+
 static int vpc_write(BlockDriverState *bs, int64_t sector_num,
     const uint8_t *buf, int nb_sectors)
 {
@@ -649,7 +660,7 @@ static BlockDriver bdrv_vpc = {
     .instance_size  = sizeof(BDRVVPCState),
     .bdrv_probe     = vpc_probe,
     .bdrv_open      = vpc_open,
-    .bdrv_read      = vpc_read,
+    .bdrv_read      = vpc_co_read,
     .bdrv_write     = vpc_write,
     .bdrv_flush     = vpc_flush,
     .bdrv_close     = vpc_close,
diff --git a/block/vvfat.c b/block/vvfat.c
index 08a72ee..f1d94ad 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1279,6 +1279,17 @@ DLOG(fprintf(stderr, "sector %d not allocated\n", (int)sector_num));
     return 0;
 }
 
+static coroutine_fn int vvfat_co_read(BlockDriverState *bs, int64_t sector_num,
+                                      uint8_t *buf, int nb_sectors)
+{
+    int ret;
+    BDRVVVFATState *s = bs->opaque;
+    qemu_co_rwlock_rdlock(&s->lock);
+    ret = vvfat_read(bs, sector_num, buf, nb_sectors);
+    qemu_co_rwlock_unlock(&s->lock);
+    return ret;
+}
+
 /* LATER TODO: statify all functions */
 
 /*
@@ -2803,7 +2814,7 @@ static BlockDriver bdrv_vvfat = {
     .format_name	= "vvfat",
     .instance_size	= sizeof(BDRVVVFATState),
     .bdrv_file_open	= vvfat_open,
-    .bdrv_read		= vvfat_read,
+    .bdrv_read		= vvfat_co_read,
     .bdrv_write		= vvfat_write,
     .bdrv_close		= vvfat_close,
     .bdrv_is_allocated	= vvfat_is_allocated,
-- 
1.7.6

[Qemu-devel] [PATCH 6/8] block: take lock around bdrv_write implementations

[Paolo Bonzini]

This does the first part of the conversion to coroutines, by
wrapping bdrv_write implementations to take the write side of the
rwlock.

Drivers that implement bdrv_write rather than bdrv_co_writev can
then benefit from asynchronous operation (at least if the underlying
protocol supports it, which is not the case for raw-win32), even
though they still operate with a bounce buffer.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/cow.c   |   13 ++++++++++++-
 block/nbd.c   |   13 ++++++++++++-
 block/vmdk.c  |   13 ++++++++++++-
 block/vpc.c   |   13 ++++++++++++-
 block/vvfat.c |   13 ++++++++++++-
 5 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/block/cow.c b/block/cow.c
index 9571549..61eaca2 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -226,6 +226,17 @@ static int cow_write(BlockDriverState *bs, int64_t sector_num,
     return cow_update_bitmap(bs, sector_num, nb_sectors);
 }
 
+static coroutine_fn int cow_co_write(BlockDriverState *bs, int64_t sector_num,
+                                     const uint8_t *buf, int nb_sectors)
+{
+    int ret;
+    BDRVCowState *s = bs->opaque;
+    qemu_co_rwlock_wrlock(&s->lock);
+    ret = cow_write(bs, sector_num, buf, nb_sectors);
+    qemu_co_rwlock_unlock(&s->lock);
+    return ret;
+}
+
 static void cow_close(BlockDriverState *bs)
 {
 }
@@ -320,7 +331,7 @@ static BlockDriver bdrv_cow = {
     .bdrv_probe		= cow_probe,
     .bdrv_open		= cow_open,
     .bdrv_read		= cow_co_read,
-    .bdrv_write		= cow_write,
+    .bdrv_write		= cow_co_write,
     .bdrv_close		= cow_close,
     .bdrv_create	= cow_create,
     .bdrv_flush		= cow_flush,
diff --git a/block/nbd.c b/block/nbd.c
index f8fed92..12da988 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -251,6 +251,17 @@ static coroutine_fn int nbd_co_read(BlockDriverState *bs, int64_t sector_num,
     return ret;
 }
 
+static coroutine_fn int nbd_co_write(BlockDriverState *bs, int64_t sector_num,
+                                     const uint8_t *buf, int nb_sectors)
+{
+    int ret;
+    BDRVNBDState *s = bs->opaque;
+    qemu_co_rwlock_wrlock(&s->lock);
+    ret = nbd_write(bs, sector_num, buf, nb_sectors);
+    qemu_co_rwlock_unlock(&s->lock);
+    return ret;
+}
+
 static void nbd_close(BlockDriverState *bs)
 {
     BDRVNBDState *s = bs->opaque;
@@ -272,7 +283,7 @@ static BlockDriver bdrv_nbd = {
     .instance_size	= sizeof(BDRVNBDState),
     .bdrv_file_open	= nbd_open,
     .bdrv_read		= nbd_co_read,
-    .bdrv_write		= nbd_write,
+    .bdrv_write		= nbd_co_write,
     .bdrv_close		= nbd_close,
     .bdrv_getlength	= nbd_getlength,
     .protocol_name	= "nbd",
diff --git a/block/vmdk.c b/block/vmdk.c
index ff78e25..a0c22f1 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1114,6 +1114,17 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
+static coroutine_fn int vmdk_co_write(BlockDriverState *bs, int64_t sector_num,
+                                      const uint8_t *buf, int nb_sectors)
+{
+    int ret;
+    BDRVVmdkState *s = bs->opaque;
+    qemu_co_rwlock_wrlock(&s->lock);
+    ret = vmdk_write(bs, sector_num, buf, nb_sectors);
+    qemu_co_rwlock_unlock(&s->lock);
+    return ret;
+}
+
 
 static int vmdk_create_extent(const char *filename, int64_t filesize,
                               bool flat, bool compress)
@@ -1552,7 +1563,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_probe     = vmdk_probe,
     .bdrv_open      = vmdk_open,
     .bdrv_read      = vmdk_co_read,
-    .bdrv_write     = vmdk_write,
+    .bdrv_write     = vmdk_co_write,
     .bdrv_close     = vmdk_close,
     .bdrv_create    = vmdk_create,
     .bdrv_flush     = vmdk_flush,
diff --git a/block/vpc.c b/block/vpc.c
index e805769..915e30c 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -456,6 +456,17 @@ static int vpc_write(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
+static coroutine_fn int vpc_co_write(BlockDriverState *bs, int64_t sector_num,
+                                     const uint8_t *buf, int nb_sectors)
+{
+    int ret;
+    BDRVVPCState *s = bs->opaque;
+    qemu_co_rwlock_wrlock(&s->lock);
+    ret = vpc_write(bs, sector_num, buf, nb_sectors);
+    qemu_co_rwlock_unlock(&s->lock);
+    return ret;
+}
+
 static int vpc_flush(BlockDriverState *bs)
 {
     return bdrv_flush(bs->file);
@@ -661,7 +672,7 @@ static BlockDriver bdrv_vpc = {
     .bdrv_probe     = vpc_probe,
     .bdrv_open      = vpc_open,
     .bdrv_read      = vpc_co_read,
-    .bdrv_write     = vpc_write,
+    .bdrv_write     = vpc_co_write,
     .bdrv_flush     = vpc_flush,
     .bdrv_close     = vpc_close,
     .bdrv_create    = vpc_create,
diff --git a/block/vvfat.c b/block/vvfat.c
index f1d94ad..be8f990 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2725,6 +2725,17 @@ DLOG(checkpoint());
     return 0;
 }
 
+static coroutine_fn int vvfat_co_write(BlockDriverState *bs, int64_t sector_num,
+                                       const uint8_t *buf, int nb_sectors)
+{
+    int ret;
+    BDRVVVFATState *s = bs->opaque;
+    qemu_co_rwlock_wrlock(&s->lock);
+    ret = vvfat_write(bs, sector_num, buf, nb_sectors);
+    qemu_co_rwlock_unlock(&s->lock);
+    return ret;
+}
+
 static int vvfat_is_allocated(BlockDriverState *bs,
 	int64_t sector_num, int nb_sectors, int* n)
 {
@@ -2815,7 +2826,7 @@ static BlockDriver bdrv_vvfat = {
     .instance_size	= sizeof(BDRVVVFATState),
     .bdrv_file_open	= vvfat_open,
     .bdrv_read		= vvfat_co_read,
-    .bdrv_write		= vvfat_write,
+    .bdrv_write		= vvfat_co_write,
     .bdrv_close		= vvfat_close,
     .bdrv_is_allocated	= vvfat_is_allocated,
     .protocol_name	= "fat",
-- 
1.7.6

[Qemu-devel] [PATCH 7/8] block: change flush to co_flush

[Paolo Bonzini]

Since coroutine operation is now mandatory, convert all bdrv_flush
implementations to coroutines.  For qcow2, this means taking the lock.
Other implementations are simpler and just forward bdrv_flush to the
underlying protocol, so they can avoid the lock.

The bdrv_flush callback is then unused and can be eliminated.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c           |    2 --
 block/cow.c       |    6 +++---
 block/qcow.c      |   11 +++++------
 block/qcow2.c     |   14 +++++++-------
 block/raw-win32.c |    4 ++--
 block/rbd.c       |    4 ++--
 block/vdi.c       |    6 +++---
 block/vmdk.c      |    8 ++++----
 block/vpc.c       |    6 +++---
 block_int.h       |    1 -
 10 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/block.c b/block.c
index 28508f2..81fb709 100644
--- a/block.c
+++ b/block.c
@@ -2892,8 +2892,6 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
             qemu_coroutine_yield();
             return co.ret;
         }
-    } else if (bs->drv->bdrv_flush) {
-        return bs->drv->bdrv_flush(bs);
     } else {
         /*
          * Some block drivers always operate in either writethrough or unsafe
diff --git a/block/cow.c b/block/cow.c
index 61eaca2..77989e8 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -306,9 +306,9 @@ exit:
     return ret;
 }
 
-static int cow_flush(BlockDriverState *bs)
+static coroutine_fn int cow_co_flush(BlockDriverState *bs)
 {
-    return bdrv_flush(bs->file);
+    return bdrv_co_flush(bs->file);
 }
 
 static QEMUOptionParameter cow_create_options[] = {
@@ -334,7 +334,7 @@ static BlockDriver bdrv_cow = {
     .bdrv_write		= cow_co_write,
     .bdrv_close		= cow_close,
     .bdrv_create	= cow_create,
-    .bdrv_flush		= cow_flush,
+    .bdrv_co_flush	= cow_co_flush,
     .bdrv_is_allocated	= cow_is_allocated,
 
     .create_options = cow_create_options,
diff --git a/block/qcow.c b/block/qcow.c
index f93e3eb..61f73d6 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -781,10 +781,9 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
-static BlockDriverAIOCB *qcow_aio_flush(BlockDriverState *bs,
-        BlockDriverCompletionFunc *cb, void *opaque)
+static coroutine_fn int qcow_co_flush(BlockDriverState *bs)
 {
-    return bdrv_aio_flush(bs->file, cb, opaque);
+    return bdrv_co_flush(bs->file);
 }
 
 static int qcow_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
@@ -824,9 +823,9 @@ static BlockDriver bdrv_qcow = {
     .bdrv_is_allocated	= qcow_is_allocated,
     .bdrv_set_key	= qcow_set_key,
     .bdrv_make_empty	= qcow_make_empty,
-    .bdrv_co_readv  = qcow_co_readv,
-    .bdrv_co_writev = qcow_co_writev,
-    .bdrv_aio_flush	= qcow_aio_flush,
+    .bdrv_co_readv      = qcow_co_readv,
+    .bdrv_co_writev     = qcow_co_writev,
+    .bdrv_co_flush	= qcow_co_flush,
     .bdrv_write_compressed = qcow_write_compressed,
     .bdrv_get_info	= qcow_get_info,
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 4dc980c..3758dbf 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1092,24 +1092,24 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
-static BlockDriverAIOCB *qcow2_aio_flush(BlockDriverState *bs,
-                                         BlockDriverCompletionFunc *cb,
-                                         void *opaque)
+static int qcow2_co_flush(BlockDriverState *bs)
 {
     BDRVQcowState *s = bs->opaque;
     int ret;
 
+    qemu_co_mutex_lock(&s->lock);
     ret = qcow2_cache_flush(bs, s->l2_table_cache);
     if (ret < 0) {
-        return NULL;
+        return ret;
     }
 
     ret = qcow2_cache_flush(bs, s->refcount_block_cache);
     if (ret < 0) {
-        return NULL;
+        return ret;
     }
+    qemu_co_mutex_unlock(&s->lock);
 
-    return bdrv_aio_flush(bs->file, cb, opaque);
+    return bdrv_co_flush(bs->file);
 }
 
 static int64_t qcow2_vm_state_offset(BDRVQcowState *s)
@@ -1230,7 +1230,7 @@ static BlockDriver bdrv_qcow2 = {
 
     .bdrv_co_readv      = qcow2_co_readv,
     .bdrv_co_writev     = qcow2_co_writev,
-    .bdrv_aio_flush     = qcow2_aio_flush,
+    .bdrv_co_flush      = qcow2_co_flush,
 
     .bdrv_discard           = qcow2_discard,
     .bdrv_truncate          = qcow2_truncate,
diff --git a/block/raw-win32.c b/block/raw-win32.c
index b7dd357..2fa7437 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -281,7 +281,7 @@ static BlockDriver bdrv_file = {
     .bdrv_file_open	= raw_open,
     .bdrv_close		= raw_close,
     .bdrv_create	= raw_create,
-    .bdrv_flush		= raw_flush,
+    .bdrv_co_flush	= raw_flush,
     .bdrv_read		= raw_read,
     .bdrv_write		= raw_write,
     .bdrv_truncate	= raw_truncate,
@@ -409,7 +409,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_probe_device	= hdev_probe_device,
     .bdrv_file_open	= hdev_open,
     .bdrv_close		= raw_close,
-    .bdrv_flush		= raw_flush,
+    .bdrv_co_flush	= raw_flush,
     .bdrv_has_zero_init = hdev_has_zero_init,
 
     .bdrv_read		= raw_read,
diff --git a/block/rbd.c b/block/rbd.c
index 3068c82..c684e0c 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -705,7 +705,7 @@ static BlockDriverAIOCB *qemu_rbd_aio_writev(BlockDriverState *bs,
     return rbd_aio_rw_vector(bs, sector_num, qiov, nb_sectors, cb, opaque, 1);
 }
 
-static int qemu_rbd_flush(BlockDriverState *bs)
+static int qemu_rbd_co_flush(BlockDriverState *bs)
 {
 #if LIBRBD_VERSION_CODE >= LIBRBD_VERSION(0, 1, 1)
     /* rbd_flush added in 0.1.1 */
@@ -851,7 +851,7 @@ static BlockDriver bdrv_rbd = {
     .bdrv_file_open     = qemu_rbd_open,
     .bdrv_close         = qemu_rbd_close,
     .bdrv_create        = qemu_rbd_create,
-    .bdrv_flush         = qemu_rbd_flush,
+    .bdrv_co_flush      = qemu_rbd_co_flush,
     .bdrv_get_info      = qemu_rbd_getinfo,
     .create_options     = qemu_rbd_create_options,
     .bdrv_getlength     = qemu_rbd_getlength,
diff --git a/block/vdi.c b/block/vdi.c
index 1d5ad2b..24e90f7 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -936,10 +936,10 @@ static void vdi_close(BlockDriverState *bs)
 {
 }
 
-static int vdi_flush(BlockDriverState *bs)
+static coroutine_fn int vdi_co_flush(BlockDriverState *bs)
 {
     logout("\n");
-    return bdrv_flush(bs->file);
+    return bdrv_co_flush(bs->file);
 }
 
 
@@ -975,7 +975,7 @@ static BlockDriver bdrv_vdi = {
     .bdrv_open = vdi_open,
     .bdrv_close = vdi_close,
     .bdrv_create = vdi_create,
-    .bdrv_flush = vdi_flush,
+    .bdrv_co_flush = vdi_co_flush,
     .bdrv_is_allocated = vdi_is_allocated,
     .bdrv_make_empty = vdi_make_empty,
 
diff --git a/block/vmdk.c b/block/vmdk.c
index a0c22f1..0af41fa 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1492,14 +1492,14 @@ static void vmdk_close(BlockDriverState *bs)
     vmdk_free_extents(bs);
 }
 
-static int vmdk_flush(BlockDriverState *bs)
+static coroutine_fn int vmdk_co_flush(BlockDriverState *bs)
 {
     int i, ret, err;
     BDRVVmdkState *s = bs->opaque;
 
-    ret = bdrv_flush(bs->file);
+    ret = bdrv_co_flush(bs->file);
     for (i = 0; i < s->num_extents; i++) {
-        err = bdrv_flush(s->extents[i].file);
+        err = bdrv_co_flush(s->extents[i].file);
         if (err < 0) {
             ret = err;
         }
@@ -1566,7 +1566,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_write     = vmdk_co_write,
     .bdrv_close     = vmdk_close,
     .bdrv_create    = vmdk_create,
-    .bdrv_flush     = vmdk_flush,
+    .bdrv_co_flush  = vmdk_co_flush,
     .bdrv_is_allocated  = vmdk_is_allocated,
     .bdrv_get_allocated_file_size  = vmdk_get_allocated_file_size,
 
diff --git a/block/vpc.c b/block/vpc.c
index 915e30c..830e7e0 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -467,9 +467,9 @@ static coroutine_fn int vpc_co_write(BlockDriverState *bs, int64_t sector_num,
     return ret;
 }
 
-static int vpc_flush(BlockDriverState *bs)
+static coroutine_fn int vpc_co_flush(BlockDriverState *bs)
 {
-    return bdrv_flush(bs->file);
+    return bdrv_co_flush(bs->file);
 }
 
 /*
@@ -673,7 +673,7 @@ static BlockDriver bdrv_vpc = {
     .bdrv_open      = vpc_open,
     .bdrv_read      = vpc_co_read,
     .bdrv_write     = vpc_co_write,
-    .bdrv_flush     = vpc_flush,
+    .bdrv_co_flush  = vpc_co_flush,
     .bdrv_close     = vpc_close,
     .bdrv_create    = vpc_create,
 
diff --git a/block_int.h b/block_int.h
index 384598f..bc3b07e 100644
--- a/block_int.h
+++ b/block_int.h
@@ -62,7 +62,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_flush)(BlockDriverState *bs);
     int (*bdrv_discard)(BlockDriverState *bs, int64_t sector_num,
                         int nb_sectors);
     int (*bdrv_is_allocated)(BlockDriverState *bs, int64_t sector_num,
-- 
1.7.6

Re: [Qemu-devel] [PATCH 7/8] block: change flush to co_flush

[Kevin Wolf]

Am 19.10.2011 16:59, schrieb Paolo Bonzini:
> Since coroutine operation is now mandatory, convert all bdrv_flush
> implementations to coroutines.  For qcow2, this means taking the lock.
> Other implementations are simpler and just forward bdrv_flush to the
> underlying protocol, so they can avoid the lock.
> 
> The bdrv_flush callback is then unused and can be eliminated.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> diff --git a/block/qcow.c b/block/qcow.c
> index f93e3eb..61f73d6 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -781,10 +781,9 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
>      return 0;
>  }
>  
> -static BlockDriverAIOCB *qcow_aio_flush(BlockDriverState *bs,
> -        BlockDriverCompletionFunc *cb, void *opaque)
> +static coroutine_fn int qcow_co_flush(BlockDriverState *bs)
>  {
> -    return bdrv_aio_flush(bs->file, cb, opaque);
> +    return bdrv_co_flush(bs->file);
>  }
>  
>  static int qcow_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> @@ -824,9 +823,9 @@ static BlockDriver bdrv_qcow = {
>      .bdrv_is_allocated	= qcow_is_allocated,
>      .bdrv_set_key	= qcow_set_key,
>      .bdrv_make_empty	= qcow_make_empty,
> -    .bdrv_co_readv  = qcow_co_readv,
> -    .bdrv_co_writev = qcow_co_writev,
> -    .bdrv_aio_flush	= qcow_aio_flush,
> +    .bdrv_co_readv      = qcow_co_readv,
> +    .bdrv_co_writev     = qcow_co_writev,
> +    .bdrv_co_flush	= qcow_co_flush,
>      .bdrv_write_compressed = qcow_write_compressed,
>      .bdrv_get_info	= qcow_get_info,

Please don't add new tabs, use spaces instead. (Same thing in other drivers)

The logic looks good to me.

Kevin

[Qemu-devel] [PATCH 8/8] block: change discard to co_discard

[Paolo Bonzini]

Since coroutine operation is now mandatory, convert both bdrv_discard
implementations to coroutines.  For qcow2, this means taking the lock
around the operation.  raw-posix remains synchronous.

The bdrv_discard callback is then unused and can be eliminated.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c           |    2 --
 block/qcow2.c     |   12 +++++++++---
 block/raw-posix.c |    4 ++--
 block_int.h       |    2 --
 4 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 81fb709..70aab63 100644
--- a/block.c
+++ b/block.c
@@ -2962,8 +2962,6 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
             qemu_coroutine_yield();
             return co.ret;
         }
-    } else if (bs->drv->bdrv_discard) {
-        return bs->drv->bdrv_discard(bs, sector_num, nb_sectors);
     } else {
         return 0;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 3758dbf..0832d11 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -978,11 +978,17 @@ static int qcow2_make_empty(BlockDriverState *bs)
     return 0;
 }
 
-static int qcow2_discard(BlockDriverState *bs, int64_t sector_num,
-    int nb_sectors)
+static coroutine_fn int qcow2_co_discard(BlockDriverState *bs,
+    int64_t sector_num, int nb_sectors)
 {
-    return qcow2_discard_clusters(bs, sector_num << BDRV_SECTOR_BITS,
+    int ret;
+    BDRVQcowState *s = bs->opaque;
+
+    qemu_co_mutex_lock(&s->lock);
+    ret = qcow2_discard_clusters(bs, sector_num << BDRV_SECTOR_BITS,
         nb_sectors);
+    qemu_co_mutex_unlock(&s->lock);
+    return ret;
 }
 
 static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
@@ -1232,7 +1238,7 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_co_writev     = qcow2_co_writev,
     .bdrv_co_flush      = qcow2_co_flush,
 
-    .bdrv_discard           = qcow2_discard,
+    .bdrv_co_discard        = qcow2_co_discard,
     .bdrv_truncate          = qcow2_truncate,
     .bdrv_write_compressed  = qcow2_write_compressed,
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index afcb4c1..cf337f7 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -602,7 +602,8 @@ static int xfs_discard(BDRVRawState *s, int64_t sector_num, int nb_sectors)
 }
 #endif
 
-static int raw_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
+static coroutine_fn int raw_co_discard(BlockDriverState *bs,
+    int64_t sector_num, int nb_sectors)
 {
 #ifdef CONFIG_XFS
     BDRVRawState *s = bs->opaque;
@@ -632,7 +632,7 @@ static BlockDriver bdrv_file = {
     .bdrv_file_open = raw_open,
     .bdrv_close = raw_close,
     .bdrv_create = raw_create,
-    .bdrv_discard = raw_discard,
+    .bdrv_co_discard = raw_co_discard,
 
     .bdrv_aio_readv = raw_aio_readv,
     .bdrv_aio_writev = raw_aio_writev,
diff --git a/block_int.h b/block_int.h
index bc3b07e..dac00f5 100644
--- a/block_int.h
+++ b/block_int.h
@@ -62,8 +62,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_discard)(BlockDriverState *bs, int64_t sector_num,
-                        int nb_sectors);
     int (*bdrv_is_allocated)(BlockDriverState *bs, int64_t sector_num,
                              int nb_sectors, int *pnum);
     int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
-- 
1.7.6

Re: [Qemu-devel] [PATCH 8/8] block: change discard to co_discard

[Kevin Wolf]

Am 19.10.2011 16:59, schrieb Paolo Bonzini:
> Since coroutine operation is now mandatory, convert both bdrv_discard
> implementations to coroutines.  For qcow2, this means taking the lock
> around the operation.  raw-posix remains synchronous.
> 
> The bdrv_discard callback is then unused and can be eliminated.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>