* [Qemu-devel] [PATCH v2 1/7] vmdk: fix return values of vmdk_parent_open
2011-10-20 11:16 [Qemu-devel] [PATCH v2 0/7] finish coroutinization of drivers Paolo Bonzini
@ 2011-10-20 11:16 ` Paolo Bonzini
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 2/7] vmdk: clean up open Paolo Bonzini
` (6 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2011-10-20 11:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha
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 | 15 +++++++++------
1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 5d16ec4..ea00938 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -283,10 +283,12 @@ static int vmdk_parent_open(BlockDriverState *bs)
char *p_name;
char desc[DESC_SIZE + 1];
BDRVVmdkState *s = bs->opaque;
+ int ret;
desc[DESC_SIZE] = '\0';
- if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) {
- return -1;
+ ret = bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE);
+ if (ret < 0) {
+ return ret;
}
p_name = strstr(desc, "parentFileNameHint");
@@ -296,10 +298,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 +631,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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 2/7] vmdk: clean up open
2011-10-20 11:16 [Qemu-devel] [PATCH v2 0/7] finish coroutinization of drivers Paolo Bonzini
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 1/7] vmdk: fix return values of vmdk_parent_open Paolo Bonzini
@ 2011-10-20 11:16 ` Paolo Bonzini
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 3/7] block: add a CoMutex to synchronous read drivers Paolo Bonzini
` (5 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2011-10-20 11:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha
Move vmdk_parent_open to vmdk_open. There's another path how
vmdk_parent_open can be reached:
vmdk_parse_extents() -> vmdk_open_sparse() -> vmdk_open_vmdk4() ->
vmdk_open_desc_file().
If that can happen, however, the code is bogus. vmdk_parent_open
reads from bs->file:
if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) {
but it is always called with s->desc_offset == 0 and with the same
bs->file. So the data that vmdk_parent_open reads comes always from the
same place, and anyway there is only one place where it can write it,
namely bs->backing_file.
So, if it cannot happen, the patched code is okay.
It is also possible that the recursive call can happen, but only once. In
that case there would still be a bug in vmdk_open_desc_file setting
s->desc_offset = 0, but the patched code is okay.
Finally, in the case where multiple recursive calls can happen the code
would need to be rewritten anyway. It is likely that this would anyway
involve adding several parameters to vmdk_parent_open, and calling it from
vmdk_open_vmdk4.
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 ea00938..ace2977 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -624,20 +624,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)
@@ -647,17 +634,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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 3/7] block: add a CoMutex to synchronous read drivers
2011-10-20 11:16 [Qemu-devel] [PATCH v2 0/7] finish coroutinization of drivers Paolo Bonzini
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 1/7] vmdk: fix return values of vmdk_parent_open Paolo Bonzini
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 2/7] vmdk: clean up open Paolo Bonzini
@ 2011-10-20 11:16 ` Paolo Bonzini
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 4/7] block: take lock around bdrv_read implementations Paolo Bonzini
` (4 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2011-10-20 11:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha
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
just need to add a CoMutex that is taken around affected operations.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/bochs.c | 2 ++
block/cloop.c | 2 ++
block/cow.c | 2 ++
block/dmg.c | 2 ++
block/nbd.c | 2 ++
block/parallels.c | 2 ++
block/vmdk.c | 2 ++
block/vpc.c | 2 ++
block/vvfat.c | 2 ++
9 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/block/bochs.c b/block/bochs.c
index 3c2f8d1..b0f8072 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -80,6 +80,7 @@ struct bochs_header {
};
typedef struct BDRVBochsState {
+ CoMutex lock;
uint32_t *catalog_bitmap;
int catalog_size;
@@ -150,6 +151,7 @@ static int bochs_open(BlockDriverState *bs, int flags)
s->extent_size = le32_to_cpu(bochs.extra.redolog.extent);
+ qemu_co_mutex_init(&s->lock);
return 0;
fail:
return -1;
diff --git a/block/cloop.c b/block/cloop.c
index 8cff9f2..a91f372 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -27,6 +27,7 @@
#include <zlib.h>
typedef struct BDRVCloopState {
+ CoMutex lock;
uint32_t block_size;
uint32_t n_blocks;
uint64_t* offsets;
@@ -93,6 +94,7 @@ static int cloop_open(BlockDriverState *bs, int flags)
s->sectors_per_block = s->block_size/512;
bs->total_sectors = s->n_blocks*s->sectors_per_block;
+ qemu_co_mutex_init(&s->lock);
return 0;
cloop_close:
diff --git a/block/cow.c b/block/cow.c
index 4cf543c..2f426e7 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -42,6 +42,7 @@ struct cow_header_v2 {
};
typedef struct BDRVCowState {
+ CoMutex 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_mutex_init(&s->lock);
return 0;
fail:
return -1;
diff --git a/block/dmg.c b/block/dmg.c
index 64c3cce..111aeae 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -28,6 +28,7 @@
#include <zlib.h>
typedef struct BDRVDMGState {
+ CoMutex lock;
/* each chunk contains a certain number of sectors,
* offsets[i] is the offset in the .dmg file,
* lengths[i] is the length of the compressed chunk,
@@ -177,6 +178,7 @@ static int dmg_open(BlockDriverState *bs, int flags)
s->current_chunk = s->n_chunks;
+ qemu_co_mutex_init(&s->lock);
return 0;
fail:
return -1;
diff --git a/block/nbd.c b/block/nbd.c
index 76f04d8..14ab225 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -47,6 +47,7 @@
#endif
typedef struct BDRVNBDState {
+ CoMutex 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_mutex_init(&s->lock);
return result;
}
diff --git a/block/parallels.c b/block/parallels.c
index c64103d..b86e87e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -46,6 +46,7 @@ struct parallels_header {
} QEMU_PACKED;
typedef struct BDRVParallelsState {
+ CoMutex lock;
uint32_t *catalog_bitmap;
int catalog_size;
@@ -95,6 +96,7 @@ static int parallels_open(BlockDriverState *bs, int flags)
for (i = 0; i < s->catalog_size; i++)
le32_to_cpus(&s->catalog_bitmap[i]);
+ qemu_co_mutex_init(&s->lock);
return 0;
fail:
if (s->catalog_bitmap)
diff --git a/block/vmdk.c b/block/vmdk.c
index ace2977..1ce220d 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -90,6 +90,7 @@ typedef struct VmdkExtent {
} VmdkExtent;
typedef struct BDRVVmdkState {
+ CoMutex lock;
int desc_offset;
bool cid_updated;
uint32_t parent_cid;
@@ -646,6 +647,7 @@ static int vmdk_open(BlockDriverState *bs, int flags)
goto fail;
}
s->parent_cid = vmdk_read_cid(bs, 1);
+ qemu_co_mutex_init(&s->lock);
return ret;
fail:
diff --git a/block/vpc.c b/block/vpc.c
index 549a632..5414042 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -110,6 +110,7 @@ struct vhd_dyndisk_header {
};
typedef struct BDRVVPCState {
+ CoMutex 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_mutex_init(&s->lock);
return 0;
fail:
return err;
diff --git a/block/vvfat.c b/block/vvfat.c
index c567697..6f666d6 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 {
+ CoMutex 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_mutex_init(&s->lock);
return 0;
}
--
1.7.6
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 4/7] block: take lock around bdrv_read implementations
2011-10-20 11:16 [Qemu-devel] [PATCH v2 0/7] finish coroutinization of drivers Paolo Bonzini
` (2 preceding siblings ...)
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 3/7] block: add a CoMutex to synchronous read drivers Paolo Bonzini
@ 2011-10-20 11:16 ` Paolo Bonzini
2011-11-06 14:27 ` Avi Kivity
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 5/7] block: take lock around bdrv_write implementations Paolo Bonzini
` (3 subsequent siblings)
7 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2011-10-20 11:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha
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/bochs.c | 13 ++++++++++++-
block/cloop.c | 13 ++++++++++++-
block/cow.c | 13 ++++++++++++-
block/dmg.c | 13 ++++++++++++-
block/nbd.c | 13 ++++++++++++-
block/parallels.c | 13 ++++++++++++-
block/vmdk.c | 13 ++++++++++++-
block/vpc.c | 13 ++++++++++++-
block/vvfat.c | 13 ++++++++++++-
9 files changed, 108 insertions(+), 9 deletions(-)
diff --git a/block/bochs.c b/block/bochs.c
index b0f8072..f49b821 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -209,6 +209,17 @@ static int bochs_read(BlockDriverState *bs, int64_t sector_num,
return 0;
}
+static coroutine_fn int bochs_co_read(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors)
+{
+ int ret;
+ BDRVBochsState *s = bs->opaque;
+ qemu_co_mutex_lock(&s->lock);
+ ret = bochs_read(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_unlock(&s->lock);
+ return ret;
+}
+
static void bochs_close(BlockDriverState *bs)
{
BDRVBochsState *s = bs->opaque;
@@ -220,7 +231,7 @@ static BlockDriver bdrv_bochs = {
.instance_size = sizeof(BDRVBochsState),
.bdrv_probe = bochs_probe,
.bdrv_open = bochs_open,
- .bdrv_read = bochs_read,
+ .bdrv_read = bochs_co_read,
.bdrv_close = bochs_close,
};
diff --git a/block/cloop.c b/block/cloop.c
index a91f372..68d45b0 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -146,6 +146,17 @@ static int cloop_read(BlockDriverState *bs, int64_t sector_num,
return 0;
}
+static coroutine_fn int cloop_co_read(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors)
+{
+ int ret;
+ BDRVCloopState *s = bs->opaque;
+ qemu_co_mutex_lock(&s->lock);
+ ret = cloop_read(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_unlock(&s->lock);
+ return ret;
+}
+
static void cloop_close(BlockDriverState *bs)
{
BDRVCloopState *s = bs->opaque;
@@ -161,7 +172,7 @@ static BlockDriver bdrv_cloop = {
.instance_size = sizeof(BDRVCloopState),
.bdrv_probe = cloop_probe,
.bdrv_open = cloop_open,
- .bdrv_read = cloop_read,
+ .bdrv_read = cloop_co_read,
.bdrv_close = cloop_close,
};
diff --git a/block/cow.c b/block/cow.c
index 2f426e7..e57378d 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_mutex_lock(&s->lock);
+ ret = cow_read(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_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/dmg.c b/block/dmg.c
index 111aeae..f46306f 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -282,6 +282,17 @@ static int dmg_read(BlockDriverState *bs, int64_t sector_num,
return 0;
}
+static coroutine_fn int dmg_co_read(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors)
+{
+ int ret;
+ BDRVDMGState *s = bs->opaque;
+ qemu_co_mutex_lock(&s->lock);
+ ret = dmg_read(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_unlock(&s->lock);
+ return ret;
+}
+
static void dmg_close(BlockDriverState *bs)
{
BDRVDMGState *s = bs->opaque;
@@ -302,7 +313,7 @@ static BlockDriver bdrv_dmg = {
.instance_size = sizeof(BDRVDMGState),
.bdrv_probe = dmg_probe,
.bdrv_open = dmg_open,
- .bdrv_read = dmg_read,
+ .bdrv_read = dmg_co_read,
.bdrv_close = dmg_close,
};
diff --git a/block/nbd.c b/block/nbd.c
index 14ab225..d5ba95d 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_mutex_lock(&s->lock);
+ ret = nbd_read(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_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/parallels.c b/block/parallels.c
index b86e87e..3baf617 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -136,6 +136,17 @@ static int parallels_read(BlockDriverState *bs, int64_t sector_num,
return 0;
}
+static coroutine_fn int parallels_co_read(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors)
+{
+ int ret;
+ BDRVParallelsState *s = bs->opaque;
+ qemu_co_mutex_lock(&s->lock);
+ ret = parallels_read(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_unlock(&s->lock);
+ return ret;
+}
+
static void parallels_close(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
@@ -147,7 +158,7 @@ static BlockDriver bdrv_parallels = {
.instance_size = sizeof(BDRVParallelsState),
.bdrv_probe = parallels_probe,
.bdrv_open = parallels_open,
- .bdrv_read = parallels_read,
+ .bdrv_read = parallels_co_read,
.bdrv_close = parallels_close,
};
diff --git a/block/vmdk.c b/block/vmdk.c
index 1ce220d..0e791f2 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1024,6 +1024,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_mutex_lock(&s->lock);
+ ret = vmdk_read(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_unlock(&s->lock);
+ return ret;
+}
+
static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
const uint8_t *buf, int nb_sectors)
{
@@ -1542,7 +1553,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 5414042..e91e397 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_mutex_lock(&s->lock);
+ ret = vpc_read(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_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 6f666d6..4501ccc 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_mutex_lock(&s->lock);
+ ret = vvfat_read(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v2 4/7] block: take lock around bdrv_read implementations
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 4/7] block: take lock around bdrv_read implementations Paolo Bonzini
@ 2011-11-06 14:27 ` Avi Kivity
2011-11-06 17:25 ` Paolo Bonzini
2011-11-07 9:12 ` Kevin Wolf
0 siblings, 2 replies; 15+ messages in thread
From: Avi Kivity @ 2011-11-06 14:27 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, stefanha
On 10/20/2011 01:16 PM, Paolo Bonzini wrote:
> 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.
This patch (2914caa088e3fbbd) breaks autotest when a guest reboots after
install; instead of rebooting, the guest is stuck in the bootloader or
kernel.
This was discovered in qemu-kvm, but applies to plain qemu too. The
commit above is broken, it's parent is good.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/7] block: take lock around bdrv_read implementations
2011-11-06 14:27 ` Avi Kivity
@ 2011-11-06 17:25 ` Paolo Bonzini
2011-11-06 17:29 ` Avi Kivity
2011-11-07 22:26 ` Avi Kivity
2011-11-07 9:12 ` Kevin Wolf
1 sibling, 2 replies; 15+ messages in thread
From: Paolo Bonzini @ 2011-11-06 17:25 UTC (permalink / raw)
To: qemu-devel, Avi Kivity
On 11/06/2011 03:27 PM, Avi Kivity wrote:
> On 10/20/2011 01:16 PM, Paolo Bonzini wrote:
>> 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.
>
> This patch (2914caa088e3fbbd) breaks autotest when a guest reboots after
> install; instead of rebooting, the guest is stuck in the bootloader or
> kernel.
Are any of these formats used by autotest?
block/bochs.c | 13 ++++++++++++-
block/cloop.c | 13 ++++++++++++-
block/cow.c | 13 ++++++++++++-
block/dmg.c | 13 ++++++++++++-
block/nbd.c | 13 ++++++++++++-
block/parallels.c | 13 ++++++++++++-
block/vmdk.c | 13 ++++++++++++-
block/vpc.c | 13 ++++++++++++-
block/vvfat.c | 13 ++++++++++++-
9 files changed, 108 insertions(+), 9 deletions(-)
Perhaps the failure is only reproduced 80-90% of the time and this
screws up the bisection.
Paolo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/7] block: take lock around bdrv_read implementations
2011-11-06 17:25 ` Paolo Bonzini
@ 2011-11-06 17:29 ` Avi Kivity
2011-11-07 22:26 ` Avi Kivity
1 sibling, 0 replies; 15+ messages in thread
From: Avi Kivity @ 2011-11-06 17:29 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On 11/06/2011 07:25 PM, Paolo Bonzini wrote:
> On 11/06/2011 03:27 PM, Avi Kivity wrote:
>> On 10/20/2011 01:16 PM, Paolo Bonzini wrote:
>>> 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.
>>
>> This patch (2914caa088e3fbbd) breaks autotest when a guest reboots after
>> install; instead of rebooting, the guest is stuck in the bootloader or
>> kernel.
>
> Are any of these formats used by autotest?
>
It's configurable; in my case, qcow2.
> block/bochs.c | 13 ++++++++++++-
> block/cloop.c | 13 ++++++++++++-
> block/cow.c | 13 ++++++++++++-
> block/dmg.c | 13 ++++++++++++-
> block/nbd.c | 13 ++++++++++++-
> block/parallels.c | 13 ++++++++++++-
> block/vmdk.c | 13 ++++++++++++-
> block/vpc.c | 13 ++++++++++++-
> block/vvfat.c | 13 ++++++++++++-
> 9 files changed, 108 insertions(+), 9 deletions(-)
>
So no.
> Perhaps the failure is only reproduced 80-90% of the time and this
> screws up the bisection.
I thought I checked the before/after commit, but looking at the
diffstat, that's doesn't make sense.
On a related note, booting with -cdrom http://blah seems broken.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/7] block: take lock around bdrv_read implementations
2011-11-06 17:25 ` Paolo Bonzini
2011-11-06 17:29 ` Avi Kivity
@ 2011-11-07 22:26 ` Avi Kivity
2011-11-07 23:12 ` Avi Kivity
1 sibling, 1 reply; 15+ messages in thread
From: Avi Kivity @ 2011-11-07 22:26 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Kevin Wolf, qemu-devel
On 11/06/2011 07:25 PM, Paolo Bonzini wrote:
>
> Perhaps the failure is only reproduced 80-90% of the time and this
> screws up the bisection.
Correct, bad bisect. It actually reproduces reliably, when the bisecter
is reliable.
The new candidate (disclaimer: unreliable bisecter) is:
commit a5c57d64aa61b700db444c4864a1da11f1165db6
Author: Paolo Bonzini <pbonzini@redhat.com>
Date: Mon Sep 12 14:40:36 2011 +0200
qemu-timer: do not refer to runstate_is_running()
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v2 4/7] block: take lock around bdrv_read implementations
2011-11-07 22:26 ` Avi Kivity
@ 2011-11-07 23:12 ` Avi Kivity
0 siblings, 0 replies; 15+ messages in thread
From: Avi Kivity @ 2011-11-07 23:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Kevin Wolf, Anthony Liguori, qemu-devel
On 11/08/2011 12:26 AM, Avi Kivity wrote:
> On 11/06/2011 07:25 PM, Paolo Bonzini wrote:
> >
> > Perhaps the failure is only reproduced 80-90% of the time and this
> > screws up the bisection.
>
> Correct, bad bisect. It actually reproduces reliably, when the bisecter
> is reliable.
>
> The new candidate (disclaimer: unreliable bisecter) is:
>
> commit a5c57d64aa61b700db444c4864a1da11f1165db6
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date: Mon Sep 12 14:40:36 2011 +0200
>
> qemu-timer: do not refer to runstate_is_running()
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
>
Fixed by 47113ab6b8c5659a, as Anthony pointed out on IRC.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/7] block: take lock around bdrv_read implementations
2011-11-06 14:27 ` Avi Kivity
2011-11-06 17:25 ` Paolo Bonzini
@ 2011-11-07 9:12 ` Kevin Wolf
1 sibling, 0 replies; 15+ messages in thread
From: Kevin Wolf @ 2011-11-07 9:12 UTC (permalink / raw)
To: Avi Kivity; +Cc: Paolo Bonzini, qemu-devel, stefanha
Am 06.11.2011 15:27, schrieb Avi Kivity:
> On 10/20/2011 01:16 PM, Paolo Bonzini wrote:
>> 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.
>
> This patch (2914caa088e3fbbd) breaks autotest when a guest reboots after
> install; instead of rebooting, the guest is stuck in the bootloader or
> kernel.
>
> This was discovered in qemu-kvm, but applies to plain qemu too. The
> commit above is broken, it's parent is good.
Does the autotest case use any of the block drivers that are changed by
this patch? I would be surprised to learn that, but otherwise it doesn't
make sense to me.
block/bochs.c | 13 ++++++++++++-
block/cloop.c | 13 ++++++++++++-
block/cow.c | 13 ++++++++++++-
block/dmg.c | 13 ++++++++++++-
block/nbd.c | 13 ++++++++++++-
block/parallels.c | 13 ++++++++++++-
block/vmdk.c | 13 ++++++++++++-
block/vpc.c | 13 ++++++++++++-
block/vvfat.c | 13 ++++++++++++-
Kevin
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v2 5/7] block: take lock around bdrv_write implementations
2011-10-20 11:16 [Qemu-devel] [PATCH v2 0/7] finish coroutinization of drivers Paolo Bonzini
` (3 preceding siblings ...)
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 4/7] block: take lock around bdrv_read implementations Paolo Bonzini
@ 2011-10-20 11:16 ` Paolo Bonzini
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 6/7] block: change flush to co_flush Paolo Bonzini
` (2 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2011-10-20 11:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha
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 e57378d..bd00042 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_mutex_lock(&s->lock);
+ ret = cow_write(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_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 d5ba95d..0568d2f 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_mutex_lock(&s->lock);
+ ret = nbd_write(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_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 0e791f2..3b376ed 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1116,6 +1116,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_mutex_lock(&s->lock);
+ ret = vmdk_write(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_unlock(&s->lock);
+ return ret;
+}
+
static int vmdk_create_extent(const char *filename, int64_t filesize,
bool flat, bool compress)
@@ -1554,7 +1565,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 e91e397..f900693 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_mutex_lock(&s->lock);
+ ret = vpc_write(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_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 4501ccc..bb9961a 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_mutex_lock(&s->lock);
+ ret = vvfat_write(bs, sector_num, buf, nb_sectors);
+ qemu_co_mutex_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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 6/7] block: change flush to co_flush
2011-10-20 11:16 [Qemu-devel] [PATCH v2 0/7] finish coroutinization of drivers Paolo Bonzini
` (4 preceding siblings ...)
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 5/7] block: take lock around bdrv_write implementations Paolo Bonzini
@ 2011-10-20 11:16 ` Paolo Bonzini
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 7/7] block: change discard to co_discard Paolo Bonzini
2011-10-21 9:42 ` [Qemu-devel] [PATCH v2 0/7] finish coroutinization of drivers Kevin Wolf
7 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2011-10-20 11:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha
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 bd00042..dc3b71d 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..ab36b29 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..883046d 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 3b376ed..6be592f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1494,14 +1494,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;
}
@@ -1568,7 +1568,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 f900693..570361c 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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v2 7/7] block: change discard to co_discard
2011-10-20 11:16 [Qemu-devel] [PATCH v2 0/7] finish coroutinization of drivers Paolo Bonzini
` (5 preceding siblings ...)
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 6/7] block: change flush to co_flush Paolo Bonzini
@ 2011-10-20 11:16 ` Paolo Bonzini
2011-10-21 9:42 ` [Qemu-devel] [PATCH v2 0/7] finish coroutinization of drivers Kevin Wolf
7 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2011-10-20 11:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha
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.
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block.c | 2 --
block/qcow2.c | 14 ++++++++++----
block/raw-posix.c | 5 +++--
block_int.h | 2 --
4 files changed, 13 insertions(+), 10 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..1c441d5 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..a3de373 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 +633,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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/7] finish coroutinization of drivers
2011-10-20 11:16 [Qemu-devel] [PATCH v2 0/7] finish coroutinization of drivers Paolo Bonzini
` (6 preceding siblings ...)
2011-10-20 11:16 ` [Qemu-devel] [PATCH v2 7/7] block: change discard to co_discard Paolo Bonzini
@ 2011-10-21 9:42 ` Kevin Wolf
7 siblings, 0 replies; 15+ messages in thread
From: Kevin Wolf @ 2011-10-21 9:42 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, stefanha
Am 20.10.2011 13:16, schrieb 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-2 are cleanups that I discovered while testing.
>
> v1->v2: rwlock->mutex, convert read-only drivers too, drop vpc change
>
> Paolo Bonzini (7):
> vmdk: fix return values of vmdk_parent_open
> vmdk: clean up open
> block: add a CoMutex to synchronous read 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
Thanks, applied all to the block branch.
Kevin
^ permalink raw reply [flat|nested] 15+ messages in thread