* [Qemu-devel] [PATCH 1/3] Combine bdrv_read and bdrv_write to bdrv_rw
[not found] <1330473276-8975-1-git-send-email-mjt@tls.msk.ru>
@ 2012-02-28 23:54 ` Michael Tokarev
2012-02-29 15:53 ` Paolo Bonzini
2012-02-28 23:54 ` [Qemu-devel] [PATCH 2/3] Combine bdrv_aio_readv and bdrv_aio_writev into bdrv_aio_rw_vector Michael Tokarev
2012-02-28 23:54 ` [Qemu-devel] [PATCH 3/3] Combine bdrv_co_readv and bdrv_co_writev into bdrv_co_rw_vector Michael Tokarev
2 siblings, 1 reply; 13+ messages in thread
From: Michael Tokarev @ 2012-02-28 23:54 UTC (permalink / raw)
To: kwolf; +Cc: mjt, qemu-devel
More or less trivial conversion.
This also fixes a few bugs in several block drivers which
provide only read but not write or the reverse. The code
allowed to use such drivers in readwrite mode. Affected
drivers: bochs, cloop, dmg, parallels, vvfat_write_target.
Some block drivers gained multiplexors. These are: cow,
vmdk, vpc, vvfat.
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
block.c | 4 +---
block/bochs.c | 9 ++++++---
block/cloop.c | 9 ++++++---
block/cow.c | 21 +++++----------------
block/dmg.c | 9 ++++++---
block/parallels.c | 9 ++++++---
block/vmdk.c | 21 +++++----------------
block/vpc.c | 21 +++++----------------
block/vvfat.c | 30 +++++++++++-------------------
block_int.h | 6 ++----
10 files changed, 53 insertions(+), 86 deletions(-)
diff --git a/block.c b/block.c
index e27d528..8869583 100644
--- a/block.c
+++ b/block.c
@@ -3199,10 +3199,8 @@ static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
if (is_write) {
qemu_iovec_to_buffer(acb->qiov, acb->bounce);
- acb->ret = bs->drv->bdrv_write(bs, sector_num, acb->bounce, nb_sectors);
- } else {
- acb->ret = bs->drv->bdrv_read(bs, sector_num, acb->bounce, nb_sectors);
}
+ acb->ret = bs->drv->bdrv_rw(bs, sector_num, acb->bounce, nb_sectors, is_write);
qemu_bh_schedule(acb->bh);
diff --git a/block/bochs.c b/block/bochs.c
index ab7944d..bdeb167 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -209,11 +209,14 @@ 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)
+static coroutine_fn int bochs_co_rw(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors, bool is_write)
{
int ret;
BDRVBochsState *s = bs->opaque;
+ if (is_write) {
+ return -EROFS;
+ }
qemu_co_mutex_lock(&s->lock);
ret = bochs_read(bs, sector_num, buf, nb_sectors);
qemu_co_mutex_unlock(&s->lock);
@@ -231,7 +234,7 @@ static BlockDriver bdrv_bochs = {
.instance_size = sizeof(BDRVBochsState),
.bdrv_probe = bochs_probe,
.bdrv_open = bochs_open,
- .bdrv_read = bochs_co_read,
+ .bdrv_rw = bochs_co_rw,
.bdrv_close = bochs_close,
};
diff --git a/block/cloop.c b/block/cloop.c
index 7570eb8..71e0007 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -156,11 +156,14 @@ 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)
+static coroutine_fn int cloop_co_rw(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors, bool is_write)
{
int ret;
BDRVCloopState *s = bs->opaque;
+ if (is_write) {
+ return -EROFS;
+ }
qemu_co_mutex_lock(&s->lock);
ret = cloop_read(bs, sector_num, buf, nb_sectors);
qemu_co_mutex_unlock(&s->lock);
@@ -183,7 +186,7 @@ static BlockDriver bdrv_cloop = {
.instance_size = sizeof(BDRVCloopState),
.bdrv_probe = cloop_probe,
.bdrv_open = cloop_open,
- .bdrv_read = cloop_co_read,
+ .bdrv_rw = cloop_co_rw,
.bdrv_close = cloop_close,
};
diff --git a/block/cow.c b/block/cow.c
index bb5927c..7cdfe87 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -214,17 +214,6 @@ static int coroutine_fn 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)
{
@@ -240,13 +229,14 @@ 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)
+static coroutine_fn int cow_co_rw(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors, bool is_write)
{
int ret;
BDRVCowState *s = bs->opaque;
qemu_co_mutex_lock(&s->lock);
- ret = cow_write(bs, sector_num, buf, nb_sectors);
+ ret = is_write ? cow_write(bs, sector_num, buf, nb_sectors) :
+ cow_read(bs, sector_num, buf, nb_sectors);
qemu_co_mutex_unlock(&s->lock);
return ret;
}
@@ -346,8 +336,7 @@ static BlockDriver bdrv_cow = {
.bdrv_close = cow_close,
.bdrv_create = cow_create,
- .bdrv_read = cow_co_read,
- .bdrv_write = cow_co_write,
+ .bdrv_rw = cow_co_rw,
.bdrv_co_flush_to_disk = cow_co_flush,
.bdrv_co_is_allocated = cow_co_is_allocated,
diff --git a/block/dmg.c b/block/dmg.c
index 37902a4..df7c8e4 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -282,11 +282,14 @@ 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)
+static coroutine_fn int dmg_co_rw(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors, bool is_write)
{
int ret;
BDRVDMGState *s = bs->opaque;
+ if (is_write) {
+ return EROFS;
+ }
qemu_co_mutex_lock(&s->lock);
ret = dmg_read(bs, sector_num, buf, nb_sectors);
qemu_co_mutex_unlock(&s->lock);
@@ -313,7 +316,7 @@ static BlockDriver bdrv_dmg = {
.instance_size = sizeof(BDRVDMGState),
.bdrv_probe = dmg_probe,
.bdrv_open = dmg_open,
- .bdrv_read = dmg_co_read,
+ .bdrv_rw = dmg_co_rw,
.bdrv_close = dmg_close,
};
diff --git a/block/parallels.c b/block/parallels.c
index d30f0ec..0081780 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -136,11 +136,14 @@ 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)
+static coroutine_fn int parallels_co_rw(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors, bool is_write)
{
int ret;
BDRVParallelsState *s = bs->opaque;
+ if (is_write) {
+ return -EROFS;
+ }
qemu_co_mutex_lock(&s->lock);
ret = parallels_read(bs, sector_num, buf, nb_sectors);
qemu_co_mutex_unlock(&s->lock);
@@ -158,7 +161,7 @@ static BlockDriver bdrv_parallels = {
.instance_size = sizeof(BDRVParallelsState),
.bdrv_probe = parallels_probe,
.bdrv_open = parallels_open,
- .bdrv_read = parallels_co_read,
+ .bdrv_rw = parallels_co_rw,
.bdrv_close = parallels_close,
};
diff --git a/block/vmdk.c b/block/vmdk.c
index 5623ac1..1759a42 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1046,17 +1046,6 @@ 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)
{
@@ -1141,13 +1130,14 @@ 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)
+static coroutine_fn int vmdk_co_rw(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors, bool is_write)
{
int ret;
BDRVVmdkState *s = bs->opaque;
qemu_co_mutex_lock(&s->lock);
- ret = vmdk_write(bs, sector_num, buf, nb_sectors);
+ ret = is_write ? vmdk_write(bs, sector_num, buf, nb_sectors)
+ : vmdk_read(bs, sector_num, buf, nb_sectors);
qemu_co_mutex_unlock(&s->lock);
return ret;
}
@@ -1593,8 +1583,7 @@ static BlockDriver bdrv_vmdk = {
.instance_size = sizeof(BDRVVmdkState),
.bdrv_probe = vmdk_probe,
.bdrv_open = vmdk_open,
- .bdrv_read = vmdk_co_read,
- .bdrv_write = vmdk_co_write,
+ .bdrv_rw = vmdk_co_rw,
.bdrv_close = vmdk_close,
.bdrv_create = vmdk_create,
.bdrv_co_flush_to_disk = vmdk_co_flush,
diff --git a/block/vpc.c b/block/vpc.c
index 6b4816f..837439a 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -445,17 +445,6 @@ 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)
{
@@ -496,13 +485,14 @@ 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)
+static coroutine_fn int vpc_co_rw(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors, bool is_write)
{
int ret;
BDRVVPCState *s = bs->opaque;
qemu_co_mutex_lock(&s->lock);
- ret = vpc_write(bs, sector_num, buf, nb_sectors);
+ ret = is_write ? vpc_write(bs, sector_num, buf, nb_sectors)
+ : vpc_read(bs, sector_num, buf, nb_sectors);
qemu_co_mutex_unlock(&s->lock);
return ret;
}
@@ -787,8 +777,7 @@ static BlockDriver bdrv_vpc = {
.bdrv_close = vpc_close,
.bdrv_create = vpc_create,
- .bdrv_read = vpc_co_read,
- .bdrv_write = vpc_co_write,
+ .bdrv_rw = vpc_co_rw,
.bdrv_co_flush_to_disk = vpc_co_flush,
.create_options = vpc_create_options,
diff --git a/block/vvfat.c b/block/vvfat.c
index 9ef21dd..f582250 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1299,17 +1299,6 @@ 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 */
/*
@@ -2750,13 +2739,14 @@ DLOG(checkpoint());
return 0;
}
-static coroutine_fn int vvfat_co_write(BlockDriverState *bs, int64_t sector_num,
- const uint8_t *buf, int nb_sectors)
+static coroutine_fn int vvfat_co_rw(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors, bool is_write)
{
int ret;
BDRVVVFATState *s = bs->opaque;
qemu_co_mutex_lock(&s->lock);
- ret = vvfat_write(bs, sector_num, buf, nb_sectors);
+ ret = is_write ? vvfat_write(bs, sector_num, buf, nb_sectors)
+ : vvfat_read(bs, sector_num, buf, nb_sectors);
qemu_co_mutex_unlock(&s->lock);
return ret;
}
@@ -2773,9 +2763,12 @@ static int coroutine_fn vvfat_co_is_allocated(BlockDriverState *bs,
return 1;
}
-static int write_target_commit(BlockDriverState *bs, int64_t sector_num,
- const uint8_t* buffer, int nb_sectors) {
+static int write_target_commit_rw(BlockDriverState *bs, int64_t sector_num,
+ uint8_t* buffer, int nb_sectors, bool is_write) {
BDRVVVFATState* s = *((BDRVVVFATState**) bs->opaque);
+ if (!is_write) {
+ return -ENOSYS;
+ }
return try_commit(s);
}
@@ -2787,7 +2780,7 @@ static void write_target_close(BlockDriverState *bs) {
static BlockDriver vvfat_write_target = {
.format_name = "vvfat_write_target",
- .bdrv_write = write_target_commit,
+ .bdrv_rw = write_target_commit_rw,
.bdrv_close = write_target_close,
};
@@ -2855,8 +2848,7 @@ static BlockDriver bdrv_vvfat = {
.format_name = "vvfat",
.instance_size = sizeof(BDRVVVFATState),
.bdrv_file_open = vvfat_open,
- .bdrv_read = vvfat_co_read,
- .bdrv_write = vvfat_co_write,
+ .bdrv_rw = vvfat_co_rw,
.bdrv_close = vvfat_close,
.bdrv_co_is_allocated = vvfat_co_is_allocated,
.protocol_name = "fat",
diff --git a/block_int.h b/block_int.h
index 04f4b83..f3930df 100644
--- a/block_int.h
+++ b/block_int.h
@@ -106,10 +106,8 @@ struct BlockDriver {
int (*bdrv_probe_device)(const char *filename);
int (*bdrv_open)(BlockDriverState *bs, int flags);
int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);
- int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
- uint8_t *buf, int nb_sectors);
- int (*bdrv_write)(BlockDriverState *bs, int64_t sector_num,
- const uint8_t *buf, int nb_sectors);
+ int (*bdrv_rw)(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors, bool is_write);
void (*bdrv_close)(BlockDriverState *bs);
int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
--
1.7.9
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH 1/3] Combine bdrv_read and bdrv_write to bdrv_rw
2012-02-28 23:54 ` [Qemu-devel] [PATCH 1/3] Combine bdrv_read and bdrv_write to bdrv_rw Michael Tokarev
@ 2012-02-29 15:53 ` Paolo Bonzini
2012-02-29 16:00 ` Michael Tokarev
0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2012-02-29 15:53 UTC (permalink / raw)
To: Michael Tokarev; +Cc: kwolf, qemu-devel
Il 29/02/2012 00:54, Michael Tokarev ha scritto:
> -static coroutine_fn int cow_co_write(BlockDriverState *bs, int64_t sector_num,
> - const uint8_t *buf, int nb_sectors)
> +static coroutine_fn int cow_co_rw(BlockDriverState *bs, int64_t sector_num,
> + uint8_t *buf, int nb_sectors, bool is_write)
> {
> int ret;
> BDRVCowState *s = bs->opaque;
> qemu_co_mutex_lock(&s->lock);
> - ret = cow_write(bs, sector_num, buf, nb_sectors);
> + ret = is_write ? cow_write(bs, sector_num, buf, nb_sectors) :
> + cow_read(bs, sector_num, buf, nb_sectors);
> qemu_co_mutex_unlock(&s->lock);
> return ret;
NACK,
the real cleanup here would be to move the lock/unlock inside cow_read
and cow_write.
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH 1/3] Combine bdrv_read and bdrv_write to bdrv_rw
2012-02-29 15:53 ` Paolo Bonzini
@ 2012-02-29 16:00 ` Michael Tokarev
2012-02-29 16:07 ` Paolo Bonzini
0 siblings, 1 reply; 13+ messages in thread
From: Michael Tokarev @ 2012-02-29 16:00 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel
On 29.02.2012 19:53, Paolo Bonzini wrote:
> Il 29/02/2012 00:54, Michael Tokarev ha scritto:
>> -static coroutine_fn int cow_co_write(BlockDriverState *bs, int64_t sector_num,
>> - const uint8_t *buf, int nb_sectors)
>> +static coroutine_fn int cow_co_rw(BlockDriverState *bs, int64_t sector_num,
>> + uint8_t *buf, int nb_sectors, bool is_write)
>> {
>> int ret;
>> BDRVCowState *s = bs->opaque;
>> qemu_co_mutex_lock(&s->lock);
>> - ret = cow_write(bs, sector_num, buf, nb_sectors);
>> + ret = is_write ? cow_write(bs, sector_num, buf, nb_sectors) :
>> + cow_read(bs, sector_num, buf, nb_sectors);
>> qemu_co_mutex_unlock(&s->lock);
>> return ret;
>
> NACK,
>
> the real cleanup here would be to move the lock/unlock inside cow_read
> and cow_write.
And how it will be a cleanup?
The whole cow code (and a few others) is not reenterant. Merely
moving this lock/unlock stuff inth actual methods eliminates two
current wrappers in cow_co_write() and cow_co_read(), which are
exactly the same now, and moves this exactly the same code into
actual methods, which has nothing to do with locking - they're
not reenterant, and they deal with internal to the format stuff.
Having this common locking layer on top and _outside_ of the
actual work helps removing irrelevant code from important paths.
Also, it will be too easy to forgot to unlock it there by doing
just "return" somewhere.
So that'll be not a cleanup at all.
Thanks,
/mjt
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH 1/3] Combine bdrv_read and bdrv_write to bdrv_rw
2012-02-29 16:00 ` Michael Tokarev
@ 2012-02-29 16:07 ` Paolo Bonzini
2012-02-29 16:36 ` Michael Tokarev
0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2012-02-29 16:07 UTC (permalink / raw)
To: qemu-devel
Il 29/02/2012 17:00, Michael Tokarev ha scritto:
> And how it will be a cleanup?
>
> The whole cow code (and a few others) is not reenterant. Merely
> moving this lock/unlock stuff inth actual methods eliminates two
> current wrappers in cow_co_write() and cow_co_read(), which are
> exactly the same now, and moves this exactly the same code into
> actual methods, which has nothing to do with locking - they're
> not reenterant, and they deal with internal to the format stuff.
> Having this common locking layer on top and _outside_ of the
> actual work helps removing irrelevant code from important paths.
> Also, it will be too easy to forgot to unlock it there by doing
> just "return" somewhere.
It's not very different from leaking memory. It's just the way C works.
In the future, you may add unlock around image access like in qcow2, and
then an unlock/lock pair would be confusing without the lock/unlock outside.
If you are worried about forgetting to unlock, add owner tracking to
qemu-coroutine-lock.c, similar to PTHREAD_MUTEX_ERRORCHECK. That would
be quite useful.
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] Combine bdrv_read and bdrv_write to bdrv_rw
2012-02-29 16:07 ` Paolo Bonzini
@ 2012-02-29 16:36 ` Michael Tokarev
0 siblings, 0 replies; 13+ messages in thread
From: Michael Tokarev @ 2012-02-29 16:36 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On 29.02.2012 20:07, Paolo Bonzini wrote:
> Il 29/02/2012 17:00, Michael Tokarev ha scritto:
>> And how it will be a cleanup?
>>
>> The whole cow code (and a few others) is not reenterant. Merely
>> moving this lock/unlock stuff inth actual methods eliminates two
>> current wrappers in cow_co_write() and cow_co_read(), which are
>> exactly the same now, and moves this exactly the same code into
>> actual methods, which has nothing to do with locking - they're
>> not reenterant, and they deal with internal to the format stuff.
>> Having this common locking layer on top and _outside_ of the
>> actual work helps removing irrelevant code from important paths.
>> Also, it will be too easy to forgot to unlock it there by doing
>> just "return" somewhere.
>
> It's not very different from leaking memory. It's just the way C works.
>
> In the future, you may add unlock around image access like in qcow2, and
> then an unlock/lock pair would be confusing without the lock/unlock outside.
>
> If you are worried about forgetting to unlock, add owner tracking to
> qemu-coroutine-lock.c, similar to PTHREAD_MUTEX_ERRORCHECK. That would
> be quite useful.
You're replying to the lest significant part of my whole comment.
I'm not worrying about forgetting about unlock or about freeing
memory. It is just one of the more things that needs remembering,
that's all.
I'm "worrying" about adding irrelevant code into already complex
routine, and adding the same code to two places, too, while it
very naturally fits into one common wrapper with this proposed
infrastructure.
Only when we actually start making these drivers less "lock-dependent"
and let them to be "a bit more" reenterant, -- only then it will
make sense to move the lock/unlock logic _down_ to these routines,
to take locks for as little time as possible.
This is what happened with qcow and qcow2 drivers in the 3/3 patch, --
the change which you missed. There, the drivers actually gained
_new_ wrapper, without any additional cleanups, which does
dispatching from rw to read or write. And only for these two
examples your original NACK for this 1/3 part can be applied :)
The "cleanup" you're referring to above is not a cleanup, it
is particular driver development, and I can try to do that
too, just not in this series, it is a completely separate
issue.
Thanks,
/mjt
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 2/3] Combine bdrv_aio_readv and bdrv_aio_writev into bdrv_aio_rw_vector
[not found] <1330473276-8975-1-git-send-email-mjt@tls.msk.ru>
2012-02-28 23:54 ` [Qemu-devel] [PATCH 1/3] Combine bdrv_read and bdrv_write to bdrv_rw Michael Tokarev
@ 2012-02-28 23:54 ` Michael Tokarev
2012-02-29 15:54 ` Paolo Bonzini
2012-02-28 23:54 ` [Qemu-devel] [PATCH 3/3] Combine bdrv_co_readv and bdrv_co_writev into bdrv_co_rw_vector Michael Tokarev
2 siblings, 1 reply; 13+ messages in thread
From: Michael Tokarev @ 2012-02-28 23:54 UTC (permalink / raw)
To: kwolf; +Cc: mjt, qemu-devel
iscsi block driver may receive some additional work. For now, some
common code has been moved out of iscsi_aio_writev() and iscsi_aio_readv()
into iscsi_aio_rw_vector(). Leftovers there can be optimized further,
and consolidated into the rw_vector too. Read and write callbacks are
consolidated as well, and once the XXX "todo" bounce-buffer change is
complete the only difference there should go away too.
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
block.c | 39 ++-------------
block.h | 3 +
block/blkdebug.c | 24 ++--------
block/blkverify.c | 74 +++++++++--------------------
block/curl.c | 19 +++++---
block/iscsi.c | 134 ++++++++++++++++++++--------------------------------
block/qed.c | 20 ++------
block/raw-posix.c | 36 +++-----------
block/rbd.c | 46 ++++--------------
block/vdi.c | 39 ++++-----------
block_int.h | 7 +--
trace-events | 6 +--
12 files changed, 140 insertions(+), 307 deletions(-)
diff --git a/block.c b/block.c
index 8869583..25ce28b 100644
--- a/block.c
+++ b/block.c
@@ -54,12 +54,6 @@ typedef enum {
} BdrvRequestFlags;
static void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load);
-static BlockDriverAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque);
-static BlockDriverAIOCB *bdrv_aio_writev_em(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque);
static int coroutine_fn bdrv_co_readv_em(BlockDriverState *bs,
int64_t sector_num, int nb_sectors,
QEMUIOVector *iov);
@@ -280,10 +274,9 @@ void bdrv_register(BlockDriver *bdrv)
/* bdrv_co_readv_em()/brdv_co_writev_em() work in terms of aio, so if
* the block driver lacks aio we need to emulate that too.
*/
- if (!bdrv->bdrv_aio_readv) {
+ if (!bdrv->bdrv_aio_rw_vector) {
/* add AIO emulation layer */
- bdrv->bdrv_aio_readv = bdrv_aio_readv_em;
- bdrv->bdrv_aio_writev = bdrv_aio_writev_em;
+ bdrv->bdrv_aio_rw_vector = bdrv_aio_rw_vector;
}
}
@@ -3180,13 +3173,13 @@ static void bdrv_aio_bh_cb(void *opaque)
qemu_aio_release(acb);
}
-static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
+BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
int64_t sector_num,
QEMUIOVector *qiov,
int nb_sectors,
BlockDriverCompletionFunc *cb,
void *opaque,
- int is_write)
+ bool is_write)
{
BlockDriverAIOCBSync *acb;
@@ -3207,21 +3200,6 @@ static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
return &acb->common;
}
-static BlockDriverAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
-{
- return bdrv_aio_rw_vector(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
-}
-
-static BlockDriverAIOCB *bdrv_aio_writev_em(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
-{
- return bdrv_aio_rw_vector(bs, sector_num, qiov, nb_sectors, cb, opaque, 1);
-}
-
-
typedef struct BlockDriverAIOCBCoroutine {
BlockDriverAIOCB common;
BlockRequest req;
@@ -3404,13 +3382,8 @@ static int coroutine_fn bdrv_co_io_em(BlockDriverState *bs, int64_t sector_num,
};
BlockDriverAIOCB *acb;
- if (is_write) {
- acb = bs->drv->bdrv_aio_writev(bs, sector_num, iov, nb_sectors,
- bdrv_co_io_em_complete, &co);
- } else {
- acb = bs->drv->bdrv_aio_readv(bs, sector_num, iov, nb_sectors,
- bdrv_co_io_em_complete, &co);
- }
+ acb = bs->drv->bdrv_aio_rw_vector(bs, sector_num, iov, nb_sectors,
+ bdrv_co_io_em_complete, &co, is_write);
trace_bdrv_co_io_em(bs, sector_num, nb_sectors, is_write, acb);
if (!acb) {
diff --git a/block.h b/block.h
index 49bca5a..f0f68e0 100644
--- a/block.h
+++ b/block.h
@@ -189,6 +189,9 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
QEMUIOVector *iov, int nb_sectors,
BlockDriverCompletionFunc *cb, void *opaque);
+BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs, int64_t sector_num,
+ QEMUIOVector *iov, int nb_sectors,
+ BlockDriverCompletionFunc *cb, void *opaque, bool is_write);
BlockDriverAIOCB *bdrv_aio_flush(BlockDriverState *bs,
BlockDriverCompletionFunc *cb, void *opaque);
BlockDriverAIOCB *bdrv_aio_discard(BlockDriverState *bs,
diff --git a/block/blkdebug.c b/block/blkdebug.c
index a251802..ae936eb 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -353,24 +353,9 @@ static BlockDriverAIOCB *inject_error(BlockDriverState *bs,
return &acb->common;
}
-static BlockDriverAIOCB *blkdebug_aio_readv(BlockDriverState *bs,
+static BlockDriverAIOCB *blkdebug_aio_rw_vector(BlockDriverState *bs,
int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
-{
- BDRVBlkdebugState *s = bs->opaque;
-
- if (s->vars.inject_errno) {
- return inject_error(bs, cb, opaque);
- }
-
- BlockDriverAIOCB *acb =
- bdrv_aio_readv(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
- return acb;
-}
-
-static BlockDriverAIOCB *blkdebug_aio_writev(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
+ BlockDriverCompletionFunc *cb, void *opaque, bool is_write)
{
BDRVBlkdebugState *s = bs->opaque;
@@ -379,7 +364,7 @@ static BlockDriverAIOCB *blkdebug_aio_writev(BlockDriverState *bs,
}
BlockDriverAIOCB *acb =
- bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
+ bdrv_aio_rw_vector(bs->file, sector_num, qiov, nb_sectors, cb, opaque, is_write);
return acb;
}
@@ -450,8 +435,7 @@ static BlockDriver bdrv_blkdebug = {
.bdrv_file_open = blkdebug_open,
.bdrv_close = blkdebug_close,
- .bdrv_aio_readv = blkdebug_aio_readv,
- .bdrv_aio_writev = blkdebug_aio_writev,
+ .bdrv_aio_rw_vector = blkdebug_aio_rw_vector,
.bdrv_aio_flush = blkdebug_aio_flush,
.bdrv_debug_event = blkdebug_debug_event,
diff --git a/block/blkverify.c b/block/blkverify.c
index 9d5f1ec..6877a23 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -227,27 +227,6 @@ static void blkverify_iovec_clone(QEMUIOVector *dest, const QEMUIOVector *src,
}
}
-static BlkverifyAIOCB *blkverify_aio_get(BlockDriverState *bs, bool is_write,
- int64_t sector_num, QEMUIOVector *qiov,
- int nb_sectors,
- BlockDriverCompletionFunc *cb,
- void *opaque)
-{
- BlkverifyAIOCB *acb = qemu_aio_get(&blkverify_aio_pool, bs, cb, opaque);
-
- acb->bh = NULL;
- acb->is_write = is_write;
- acb->sector_num = sector_num;
- acb->nb_sectors = nb_sectors;
- acb->ret = -EINPROGRESS;
- acb->done = 0;
- acb->qiov = qiov;
- acb->buf = NULL;
- acb->verify = NULL;
- acb->finished = NULL;
- return acb;
-}
-
static void blkverify_aio_bh(void *opaque)
{
BlkverifyAIOCB *acb = opaque;
@@ -297,38 +276,34 @@ static void blkverify_verify_readv(BlkverifyAIOCB *acb)
}
}
-static BlockDriverAIOCB *blkverify_aio_readv(BlockDriverState *bs,
+static BlockDriverAIOCB *blkverify_aio_rw_vector(BlockDriverState *bs,
int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
+ BlockDriverCompletionFunc *cb, void *opaque, bool is_write)
{
BDRVBlkverifyState *s = bs->opaque;
- BlkverifyAIOCB *acb = blkverify_aio_get(bs, false, sector_num, qiov,
- nb_sectors, cb, opaque);
-
- acb->verify = blkverify_verify_readv;
- acb->buf = qemu_blockalign(bs->file, qiov->size);
- qemu_iovec_init(&acb->raw_qiov, acb->qiov->niov);
- blkverify_iovec_clone(&acb->raw_qiov, qiov, acb->buf);
-
- bdrv_aio_readv(s->test_file, sector_num, qiov, nb_sectors,
- blkverify_aio_cb, acb);
- bdrv_aio_readv(bs->file, sector_num, &acb->raw_qiov, nb_sectors,
- blkverify_aio_cb, acb);
- return &acb->common;
-}
+ BlkverifyAIOCB *acb = qemu_aio_get(&blkverify_aio_pool, bs, cb, opaque);
-static BlockDriverAIOCB *blkverify_aio_writev(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
-{
- BDRVBlkverifyState *s = bs->opaque;
- BlkverifyAIOCB *acb = blkverify_aio_get(bs, true, sector_num, qiov,
- nb_sectors, cb, opaque);
+ acb->bh = NULL;
+ acb->is_write = is_write;
+ acb->sector_num = sector_num;
+ acb->nb_sectors = nb_sectors;
+ acb->ret = -EINPROGRESS;
+ acb->done = 0;
+ acb->qiov = qiov;
+ acb->buf = NULL;
+ acb->verify = NULL;
+ acb->finished = NULL;
- bdrv_aio_writev(s->test_file, sector_num, qiov, nb_sectors,
- blkverify_aio_cb, acb);
- bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors,
- blkverify_aio_cb, acb);
+ if (!is_write) {
+ acb->verify = blkverify_verify_readv;
+ acb->buf = qemu_blockalign(bs->file, qiov->size);
+ qemu_iovec_init(&acb->raw_qiov, acb->qiov->niov);
+ blkverify_iovec_clone(&acb->raw_qiov, qiov, acb->buf);
+ }
+ bdrv_aio_rw_vector(s->test_file, sector_num, qiov, nb_sectors,
+ blkverify_aio_cb, acb, is_write);
+ bdrv_aio_rw_vector(bs->file, sector_num, &acb->raw_qiov, nb_sectors,
+ blkverify_aio_cb, acb, is_write);
return &acb->common;
}
@@ -353,8 +328,7 @@ static BlockDriver bdrv_blkverify = {
.bdrv_file_open = blkverify_open,
.bdrv_close = blkverify_close,
- .bdrv_aio_readv = blkverify_aio_readv,
- .bdrv_aio_writev = blkverify_aio_writev,
+ .bdrv_aio_rw_vector = blkverify_aio_rw_vector,
.bdrv_aio_flush = blkverify_aio_flush,
};
diff --git a/block/curl.c b/block/curl.c
index e9102e3..ed58dd3 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -501,12 +501,17 @@ static void curl_readv_bh_cb(void *p)
}
-static BlockDriverAIOCB *curl_aio_readv(BlockDriverState *bs,
+static BlockDriverAIOCB *curl_aio_rw_vector(BlockDriverState *bs,
int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
+ BlockDriverCompletionFunc *cb, void *opaque, bool is_write)
{
CURLAIOCB *acb;
+ if (is_write) {
+ errno = EROFS;
+ return NULL;
+ }
+
acb = qemu_aio_get(&curl_aio_pool, bs, cb, opaque);
acb->qiov = qiov;
@@ -563,7 +568,7 @@ static BlockDriver bdrv_http = {
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
- .bdrv_aio_readv = curl_aio_readv,
+ .bdrv_aio_rw_vector = curl_aio_rw_vector,
};
static BlockDriver bdrv_https = {
@@ -575,7 +580,7 @@ static BlockDriver bdrv_https = {
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
- .bdrv_aio_readv = curl_aio_readv,
+ .bdrv_aio_rw_vector = curl_aio_rw_vector,
};
static BlockDriver bdrv_ftp = {
@@ -587,7 +592,7 @@ static BlockDriver bdrv_ftp = {
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
- .bdrv_aio_readv = curl_aio_readv,
+ .bdrv_aio_rw_vector = curl_aio_rw_vector,
};
static BlockDriver bdrv_ftps = {
@@ -599,7 +604,7 @@ static BlockDriver bdrv_ftps = {
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
- .bdrv_aio_readv = curl_aio_readv,
+ .bdrv_aio_rw_vector = curl_aio_rw_vector,
};
static BlockDriver bdrv_tftp = {
@@ -611,7 +616,7 @@ static BlockDriver bdrv_tftp = {
.bdrv_close = curl_close,
.bdrv_getlength = curl_getlength,
- .bdrv_aio_readv = curl_aio_readv,
+ .bdrv_aio_rw_vector = curl_aio_rw_vector,
};
static void curl_block_init(void)
diff --git a/block/iscsi.c b/block/iscsi.c
index bd3ca11..e780b5f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -161,14 +161,16 @@ iscsi_readv_writev_bh_cb(void *p)
static void
-iscsi_aio_write10_cb(struct iscsi_context *iscsi, int status,
- void *command_data, void *opaque)
+iscsi_aio_rw10_cb(struct iscsi_context *iscsi, int status,
+ void *command_data, void *opaque)
{
IscsiAIOCB *acb = opaque;
- trace_iscsi_aio_write10_cb(iscsi, status, acb, acb->canceled);
+ trace_iscsi_aio_rw10_cb(iscsi, status, acb, acb->canceled);
- g_free(acb->buf);
+ if (acb->buf) {
+ g_free(acb->buf);
+ }
if (acb->canceled != 0) {
qemu_aio_release(acb);
@@ -179,7 +181,7 @@ iscsi_aio_write10_cb(struct iscsi_context *iscsi, int status,
acb->status = 0;
if (status < 0) {
- error_report("Failed to write10 data to iSCSI lun. %s",
+ error_report("Failed to read/write10 data from/to iSCSI lun. %s",
iscsi_get_error(iscsi));
acb->status = -EIO;
}
@@ -194,15 +196,10 @@ static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
return sector * BDRV_SECTOR_SIZE / iscsilun->block_size;
}
-static BlockDriverAIOCB *
-iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
- QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb,
- void *opaque)
+static int
+iscsi_aio_writev(BlockDriverState *bs, IscsiAIOCB *acb, int64_t sector_num, int nb_sectors)
{
- IscsiLun *iscsilun = bs->opaque;
- struct iscsi_context *iscsi = iscsilun->iscsi;
- IscsiAIOCB *acb;
+ IscsiLun *iscsilun = acb->iscsilun;
size_t size;
int fua = 0;
@@ -211,86 +208,32 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
fua = 1;
}
- acb = qemu_aio_get(&iscsi_aio_pool, bs, cb, opaque);
- trace_iscsi_aio_writev(iscsi, sector_num, nb_sectors, opaque, acb);
-
- acb->iscsilun = iscsilun;
- acb->qiov = qiov;
-
- acb->canceled = 0;
-
/* XXX we should pass the iovec to write10 to avoid the extra copy */
/* this will allow us to get rid of 'buf' completely */
size = nb_sectors * BDRV_SECTOR_SIZE;
acb->buf = g_malloc(size);
qemu_iovec_to_buffer(acb->qiov, acb->buf);
- acb->task = iscsi_write10_task(iscsi, iscsilun->lun, acb->buf, size,
+ acb->task = iscsi_write10_task(iscsilun->iscsi, iscsilun->lun, acb->buf, size,
sector_qemu2lun(sector_num, iscsilun),
fua, 0, iscsilun->block_size,
- iscsi_aio_write10_cb, acb);
+ iscsi_aio_rw10_cb, acb);
if (acb->task == NULL) {
- error_report("iSCSI: Failed to send write10 command. %s",
- iscsi_get_error(iscsi));
g_free(acb->buf);
- qemu_aio_release(acb);
- return NULL;
- }
-
- iscsi_set_events(iscsilun);
-
- return &acb->common;
-}
-
-static void
-iscsi_aio_read10_cb(struct iscsi_context *iscsi, int status,
- void *command_data, void *opaque)
-{
- IscsiAIOCB *acb = opaque;
-
- trace_iscsi_aio_read10_cb(iscsi, status, acb, acb->canceled);
-
- if (acb->canceled != 0) {
- qemu_aio_release(acb);
- scsi_free_scsi_task(acb->task);
- acb->task = NULL;
- return;
- }
-
- acb->status = 0;
- if (status != 0) {
- error_report("Failed to read10 data from iSCSI lun. %s",
- iscsi_get_error(iscsi));
- acb->status = -EIO;
+ return -1;
}
-
- iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
- scsi_free_scsi_task(acb->task);
- acb->task = NULL;
+ return 0;
}
-static BlockDriverAIOCB *
-iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num,
- QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb,
- void *opaque)
+static int
+iscsi_aio_readv(BlockDriverState *bs, IscsiAIOCB *acb, int64_t sector_num, int nb_sectors)
{
- IscsiLun *iscsilun = bs->opaque;
- struct iscsi_context *iscsi = iscsilun->iscsi;
- IscsiAIOCB *acb;
+ IscsiLun *iscsilun = acb->iscsilun;
size_t qemu_read_size, lun_read_size;
int i;
qemu_read_size = BDRV_SECTOR_SIZE * (size_t)nb_sectors;
- acb = qemu_aio_get(&iscsi_aio_pool, bs, cb, opaque);
- trace_iscsi_aio_readv(iscsi, sector_num, nb_sectors, opaque, acb);
-
- acb->iscsilun = iscsilun;
- acb->qiov = qiov;
-
- acb->canceled = 0;
acb->read_size = qemu_read_size;
- acb->buf = NULL;
/* If LUN blocksize is bigger than BDRV_BLOCK_SIZE a read from QEMU
* may be misaligned to the LUN, so we may need to read some extra
@@ -306,15 +249,12 @@ iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num,
lun_read_size = (qemu_read_size + iscsilun->block_size
+ acb->read_offset - 1)
/ iscsilun->block_size * iscsilun->block_size;
- acb->task = iscsi_read10_task(iscsi, iscsilun->lun,
+ acb->task = iscsi_read10_task(iscsilun->iscsi, iscsilun->lun,
sector_qemu2lun(sector_num, iscsilun),
lun_read_size, iscsilun->block_size,
- iscsi_aio_read10_cb, acb);
+ iscsi_aio_rw10_cb, acb);
if (acb->task == NULL) {
- error_report("iSCSI: Failed to send read10 command. %s",
- iscsi_get_error(iscsi));
- qemu_aio_release(acb);
- return NULL;
+ return -1;
}
for (i = 0; i < acb->qiov->niov; i++) {
@@ -323,6 +263,37 @@ iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num,
acb->qiov->iov[i].iov_base);
}
+ return 0;
+}
+
+static BlockDriverAIOCB *
+iscsi_aio_rw_vector(BlockDriverState *bs, int64_t sector_num,
+ QEMUIOVector *qiov, int nb_sectors,
+ BlockDriverCompletionFunc *cb,
+ void *opaque, bool is_write)
+{
+ IscsiLun *iscsilun = bs->opaque;
+ IscsiAIOCB *acb;
+ int ret;
+
+ acb = qemu_aio_get(&iscsi_aio_pool, bs, cb, opaque);
+ trace_iscsi_aio_rw_vector(iscsilun->iscsi, sector_num, nb_sectors, opaque, acb, is_write);
+
+ acb->iscsilun = iscsilun;
+ acb->qiov = qiov;
+
+ acb->canceled = 0;
+ acb->buf = NULL;
+
+ ret = is_write ? iscsi_aio_writev(bs, acb, sector_num, nb_sectors)
+ : iscsi_aio_readv(bs, acb, sector_num, nb_sectors);
+ if (ret != 0) {
+ error_report("iSCSI: Failed to send read10/write10 command. %s",
+ iscsi_get_error(iscsilun->iscsi));
+ qemu_aio_release(acb);
+ return NULL;
+ }
+
iscsi_set_events(iscsilun);
return &acb->common;
@@ -697,8 +668,7 @@ static BlockDriver bdrv_iscsi = {
.bdrv_getlength = iscsi_getlength,
- .bdrv_aio_readv = iscsi_aio_readv,
- .bdrv_aio_writev = iscsi_aio_writev,
+ .bdrv_aio_rw_vector = iscsi_aio_rw_vector,
.bdrv_aio_flush = iscsi_aio_flush,
};
diff --git a/block/qed.c b/block/qed.c
index a041d31..155fa59 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1331,23 +1331,14 @@ static BlockDriverAIOCB *qed_aio_setup(BlockDriverState *bs,
return &acb->common;
}
-static BlockDriverAIOCB *bdrv_qed_aio_readv(BlockDriverState *bs,
+static BlockDriverAIOCB *bdrv_qed_aio_rw_vector(BlockDriverState *bs,
int64_t sector_num,
QEMUIOVector *qiov, int nb_sectors,
BlockDriverCompletionFunc *cb,
- void *opaque)
-{
- return qed_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
-}
-
-static BlockDriverAIOCB *bdrv_qed_aio_writev(BlockDriverState *bs,
- int64_t sector_num,
- QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb,
- void *opaque)
+ void *opaque, bool is_write)
{
- return qed_aio_setup(bs, sector_num, qiov, nb_sectors, cb,
- opaque, QED_AIOCB_WRITE);
+ return
+ qed_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, is_write ? QED_AIOCB_WRITE : 0);
}
static BlockDriverAIOCB *bdrv_qed_aio_flush(BlockDriverState *bs,
@@ -1560,8 +1551,7 @@ static BlockDriver bdrv_qed = {
.bdrv_create = bdrv_qed_create,
.bdrv_co_is_allocated = bdrv_qed_co_is_allocated,
.bdrv_make_empty = bdrv_qed_make_empty,
- .bdrv_aio_readv = bdrv_qed_aio_readv,
- .bdrv_aio_writev = bdrv_qed_aio_writev,
+ .bdrv_aio_rw_vector = bdrv_qed_aio_rw_vector,
.bdrv_aio_flush = bdrv_qed_aio_flush,
.bdrv_co_write_zeroes = bdrv_qed_co_write_zeroes,
.bdrv_truncate = bdrv_qed_truncate,
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 2d1bc13..dc93911 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -312,11 +312,12 @@ static int qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
return 1;
}
-static BlockDriverAIOCB *raw_aio_submit(BlockDriverState *bs,
+static BlockDriverAIOCB *raw_aio_rw_vector(BlockDriverState *bs,
int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque, int type)
+ BlockDriverCompletionFunc *cb, void *opaque, bool is_write)
{
BDRVRawState *s = bs->opaque;
+ int type = is_write ? QEMU_AIO_WRITE : QEMU_AIO_READ;
if (fd_open(bs) < 0)
return NULL;
@@ -341,22 +342,6 @@ static BlockDriverAIOCB *raw_aio_submit(BlockDriverState *bs,
cb, opaque, type);
}
-static BlockDriverAIOCB *raw_aio_readv(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
-{
- return raw_aio_submit(bs, sector_num, qiov, nb_sectors,
- cb, opaque, QEMU_AIO_READ);
-}
-
-static BlockDriverAIOCB *raw_aio_writev(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
-{
- return raw_aio_submit(bs, sector_num, qiov, nb_sectors,
- cb, opaque, QEMU_AIO_WRITE);
-}
-
static BlockDriverAIOCB *raw_aio_flush(BlockDriverState *bs,
BlockDriverCompletionFunc *cb, void *opaque)
{
@@ -635,8 +620,7 @@ static BlockDriver bdrv_file = {
.bdrv_create = raw_create,
.bdrv_co_discard = raw_co_discard,
- .bdrv_aio_readv = raw_aio_readv,
- .bdrv_aio_writev = raw_aio_writev,
+ .bdrv_aio_rw_vector = raw_aio_rw_vector,
.bdrv_aio_flush = raw_aio_flush,
.bdrv_truncate = raw_truncate,
@@ -903,8 +887,7 @@ static BlockDriver bdrv_host_device = {
.create_options = raw_create_options,
.bdrv_has_zero_init = hdev_has_zero_init,
- .bdrv_aio_readv = raw_aio_readv,
- .bdrv_aio_writev = raw_aio_writev,
+ .bdrv_aio_rw_vector = raw_aio_rw_vector,
.bdrv_aio_flush = raw_aio_flush,
.bdrv_truncate = raw_truncate,
@@ -1022,8 +1005,7 @@ static BlockDriver bdrv_host_floppy = {
.create_options = raw_create_options,
.bdrv_has_zero_init = hdev_has_zero_init,
- .bdrv_aio_readv = raw_aio_readv,
- .bdrv_aio_writev = raw_aio_writev,
+ .bdrv_aio_rw_vector = raw_aio_rw_vector,
.bdrv_aio_flush = raw_aio_flush,
.bdrv_truncate = raw_truncate,
@@ -1121,8 +1103,7 @@ static BlockDriver bdrv_host_cdrom = {
.create_options = raw_create_options,
.bdrv_has_zero_init = hdev_has_zero_init,
- .bdrv_aio_readv = raw_aio_readv,
- .bdrv_aio_writev = raw_aio_writev,
+ .bdrv_aio_rw_vector = raw_aio_rw_vector,
.bdrv_aio_flush = raw_aio_flush,
.bdrv_truncate = raw_truncate,
@@ -1240,8 +1221,7 @@ static BlockDriver bdrv_host_cdrom = {
.create_options = raw_create_options,
.bdrv_has_zero_init = hdev_has_zero_init,
- .bdrv_aio_readv = raw_aio_readv,
- .bdrv_aio_writev = raw_aio_writev,
+ .bdrv_aio_rw_vector = raw_aio_rw_vector,
.bdrv_aio_flush = raw_aio_flush,
.bdrv_truncate = raw_truncate,
diff --git a/block/rbd.c b/block/rbd.c
index 46a8579..5078b0d 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -617,12 +617,12 @@ static void rbd_aio_bh_cb(void *opaque)
qemu_aio_release(acb);
}
-static BlockDriverAIOCB *rbd_aio_rw_vector(BlockDriverState *bs,
- int64_t sector_num,
- QEMUIOVector *qiov,
- int nb_sectors,
- BlockDriverCompletionFunc *cb,
- void *opaque, int write)
+static BlockDriverAIOCB *qemu_rbd_aio_rw_vector(BlockDriverState *bs,
+ int64_t sector_num,
+ QEMUIOVector *qiov,
+ int nb_sectors,
+ BlockDriverCompletionFunc *cb,
+ void *opaque, bool is_write)
{
RBDAIOCB *acb;
RADOSCB *rcb;
@@ -634,7 +634,7 @@ static BlockDriverAIOCB *rbd_aio_rw_vector(BlockDriverState *bs,
BDRVRBDState *s = bs->opaque;
acb = qemu_aio_get(&rbd_aio_pool, bs, cb, opaque);
- acb->write = write;
+ acb->write = is_write;
acb->qiov = qiov;
acb->bounce = qemu_blockalign(bs, qiov->size);
acb->ret = 0;
@@ -643,7 +643,7 @@ static BlockDriverAIOCB *rbd_aio_rw_vector(BlockDriverState *bs,
acb->cancelled = 0;
acb->bh = NULL;
- if (write) {
+ if (is_write) {
qemu_iovec_to_buffer(acb->qiov, acb->bounce);
}
@@ -665,11 +665,8 @@ static BlockDriverAIOCB *rbd_aio_rw_vector(BlockDriverState *bs,
goto failed;
}
- if (write) {
- r = rbd_aio_write(s->image, off, size, buf, c);
- } else {
- r = rbd_aio_read(s->image, off, size, buf, c);
- }
+ r = is_write ? rbd_aio_write(s->image, off, size, buf, c)
+ : rbd_aio_read(s->image, off, size, buf, c);
if (r < 0) {
goto failed;
@@ -684,26 +681,6 @@ failed:
return NULL;
}
-static BlockDriverAIOCB *qemu_rbd_aio_readv(BlockDriverState *bs,
- int64_t sector_num,
- QEMUIOVector *qiov,
- int nb_sectors,
- BlockDriverCompletionFunc *cb,
- void *opaque)
-{
- return rbd_aio_rw_vector(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
-}
-
-static BlockDriverAIOCB *qemu_rbd_aio_writev(BlockDriverState *bs,
- int64_t sector_num,
- QEMUIOVector *qiov,
- int nb_sectors,
- BlockDriverCompletionFunc *cb,
- void *opaque)
-{
- return rbd_aio_rw_vector(bs, sector_num, qiov, nb_sectors, cb, opaque, 1);
-}
-
static int qemu_rbd_co_flush(BlockDriverState *bs)
{
#if LIBRBD_VERSION_CODE >= LIBRBD_VERSION(0, 1, 1)
@@ -877,8 +854,7 @@ static BlockDriver bdrv_rbd = {
.bdrv_truncate = qemu_rbd_truncate,
.protocol_name = "rbd",
- .bdrv_aio_readv = qemu_rbd_aio_readv,
- .bdrv_aio_writev = qemu_rbd_aio_writev,
+ .bdrv_aio_rw_vector = qemu_rbd_aio_rw_vector,
.bdrv_co_flush_to_disk = qemu_rbd_co_flush,
.bdrv_snapshot_create = qemu_rbd_snap_create,
diff --git a/block/vdi.c b/block/vdi.c
index 6a0011f..4fd56d7 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -642,15 +642,22 @@ done:
qemu_aio_release(acb);
}
-static BlockDriverAIOCB *vdi_aio_readv(BlockDriverState *bs,
+static BlockDriverAIOCB *vdi_aio_rw_vector(BlockDriverState *bs,
int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
+ BlockDriverCompletionFunc *cb, void *opaque, bool is_write)
{
VdiAIOCB *acb;
int ret;
+#if !defined(CONFIG_VDI_WRITE)
+ if (is_write) {
+ errno = ENOSYS;
+ return NULL;
+ }
+#endif
+
logout("\n");
- acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
+ acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, is_write);
ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
if (ret < 0) {
if (acb->qiov->niov > 1) {
@@ -796,27 +803,6 @@ done:
qemu_aio_release(acb);
}
-static BlockDriverAIOCB *vdi_aio_writev(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque)
-{
- VdiAIOCB *acb;
- int ret;
-
- logout("\n");
- acb = vdi_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 1);
- ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
- if (ret < 0) {
- if (acb->qiov->niov > 1) {
- qemu_vfree(acb->orig_buf);
- }
- qemu_aio_release(acb);
- return NULL;
- }
-
- return &acb->common;
-}
-
static int vdi_create(const char *filename, QEMUOptionParameter *options)
{
int fd;
@@ -973,10 +959,7 @@ static BlockDriver bdrv_vdi = {
.bdrv_co_is_allocated = vdi_co_is_allocated,
.bdrv_make_empty = vdi_make_empty,
- .bdrv_aio_readv = vdi_aio_readv,
-#if defined(CONFIG_VDI_WRITE)
- .bdrv_aio_writev = vdi_aio_writev,
-#endif
+ .bdrv_aio_rw_vector = vdi_aio_rw_vector,
.bdrv_get_info = vdi_get_info,
diff --git a/block_int.h b/block_int.h
index f3930df..aeafa2e 100644
--- a/block_int.h
+++ b/block_int.h
@@ -113,12 +113,9 @@ struct BlockDriver {
int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
int (*bdrv_make_empty)(BlockDriverState *bs);
/* aio */
- BlockDriverAIOCB *(*bdrv_aio_readv)(BlockDriverState *bs,
+ BlockDriverAIOCB *(*bdrv_aio_rw_vector)(BlockDriverState *bs,
int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque);
- BlockDriverAIOCB *(*bdrv_aio_writev)(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
- BlockDriverCompletionFunc *cb, void *opaque);
+ BlockDriverCompletionFunc *cb, void *opaque, bool is_write);
BlockDriverAIOCB *(*bdrv_aio_flush)(BlockDriverState *bs,
BlockDriverCompletionFunc *cb, void *opaque);
BlockDriverAIOCB *(*bdrv_aio_discard)(BlockDriverState *bs,
diff --git a/trace-events b/trace-events
index e918ff6..e5d3c27 100644
--- a/trace-events
+++ b/trace-events
@@ -524,10 +524,8 @@ escc_kbd_command(int val) "Command %d"
escc_sunmouse_event(int dx, int dy, int buttons_state) "dx=%d dy=%d buttons=%01x"
# block/iscsi.c
-iscsi_aio_write10_cb(void *iscsi, int status, void *acb, int canceled) "iscsi %p status %d acb %p canceled %d"
-iscsi_aio_writev(void *iscsi, int64_t sector_num, int nb_sectors, void *opaque, void *acb) "iscsi %p sector_num %"PRId64" nb_sectors %d opaque %p acb %p"
-iscsi_aio_read10_cb(void *iscsi, int status, void *acb, int canceled) "iscsi %p status %d acb %p canceled %d"
-iscsi_aio_readv(void *iscsi, int64_t sector_num, int nb_sectors, void *opaque, void *acb) "iscsi %p sector_num %"PRId64" nb_sectors %d opaque %p acb %p"
+iscsi_aio_rw10_cb(void *iscsi, int status, void *acb, int canceled) "iscsi %p status %d acb %p canceled %d"
+iscsi_aio_rw_vector(void *iscsi, int64_t sector_num, int nb_sectors, void *opaque, void *acb, bool is_write) "iscsi %p sector_num %"PRId64" nb_sectors %d opaque %p acb %p is_write %d"
# hw/esp.c
esp_raise_irq(void) "Raise IRQ"
--
1.7.9
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH 2/3] Combine bdrv_aio_readv and bdrv_aio_writev into bdrv_aio_rw_vector
2012-02-28 23:54 ` [Qemu-devel] [PATCH 2/3] Combine bdrv_aio_readv and bdrv_aio_writev into bdrv_aio_rw_vector Michael Tokarev
@ 2012-02-29 15:54 ` Paolo Bonzini
2012-02-29 16:16 ` Michael Tokarev
0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2012-02-29 15:54 UTC (permalink / raw)
To: qemu-devel
Il 29/02/2012 00:54, Michael Tokarev ha scritto:
> iscsi block driver may receive some additional work. For now, some
> common code has been moved out of iscsi_aio_writev() and iscsi_aio_readv()
> into iscsi_aio_rw_vector(). Leftovers there can be optimized further,
> and consolidated into the rw_vector too. Read and write callbacks are
> consolidated as well, and once the XXX "todo" bounce-buffer change is
> complete the only difference there should go away too.
What about flush, discard, etc.?
It seems to me that either we make a single entry point that takes some
kind of BlockRequest, or there is no reason to do partial unification.
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] Combine bdrv_aio_readv and bdrv_aio_writev into bdrv_aio_rw_vector
2012-02-29 15:54 ` Paolo Bonzini
@ 2012-02-29 16:16 ` Michael Tokarev
0 siblings, 0 replies; 13+ messages in thread
From: Michael Tokarev @ 2012-02-29 16:16 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On 29.02.2012 19:54, Paolo Bonzini wrote:
> Il 29/02/2012 00:54, Michael Tokarev ha scritto:
>> iscsi block driver may receive some additional work. For now, some
>> common code has been moved out of iscsi_aio_writev() and iscsi_aio_readv()
>> into iscsi_aio_rw_vector(). Leftovers there can be optimized further,
>> and consolidated into the rw_vector too. Read and write callbacks are
>> consolidated as well, and once the XXX "todo" bounce-buffer change is
>> complete the only difference there should go away too.
>
> What about flush, discard, etc.?
>
> It seems to me that either we make a single entry point that takes some
> kind of BlockRequest, or there is no reason to do partial unification.
Flush and discard are quite special. _All_ drivers provide reads and
writes (well, except of the case when the device is read-only by definition).
But very few provides discard, and discard is different from reads and
writes because it does not take any data. Flush is of the same nature --
it is just request, no data. So for these, separate methods exists
and are in use now -- only in these drivers where it is appropriate.
The only additional flag or operation which can be passed - which I can
think of, anyway - is WRITE_ZEROES. But I think it is more of discard
variety than read/write. So there, maybe it is discard method which
may be combined with write_zeroes, but not discard and flush combined
with reads and writes.
Thanks,
/mjt
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 3/3] Combine bdrv_co_readv and bdrv_co_writev into bdrv_co_rw_vector
[not found] <1330473276-8975-1-git-send-email-mjt@tls.msk.ru>
2012-02-28 23:54 ` [Qemu-devel] [PATCH 1/3] Combine bdrv_read and bdrv_write to bdrv_rw Michael Tokarev
2012-02-28 23:54 ` [Qemu-devel] [PATCH 2/3] Combine bdrv_aio_readv and bdrv_aio_writev into bdrv_aio_rw_vector Michael Tokarev
@ 2012-02-28 23:54 ` Michael Tokarev
2012-02-29 16:01 ` Paolo Bonzini
2 siblings, 1 reply; 13+ messages in thread
From: Michael Tokarev @ 2012-02-28 23:54 UTC (permalink / raw)
To: kwolf; +Cc: mjt, qemu-devel
In block.c, we combine bdrv_co_do_readv(), bdrv_co_do_writev() and
bdrv_co_do_write_zeroes() into single routine, bdrv_co_do_rw_vector(),
and remove lots of now unneded helper functions.
sheepdog driver already had sd_co_rw_vector() which half-combined
read and write paths.
qcow and qcow2 block drivers now contains multiplexing functions
(rw_vector() -> readv()+writev()), since for these drivers, the
read and write routines actually have very little in common.
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
block.c | 206 ++++++++++++++++++-------------------------------
block.h | 6 ++
block/nbd.c | 95 ++++++-----------------
block/qcow.c | 10 ++-
block/qcow2-cluster.c | 4 +-
block/qcow2.c | 13 +++-
block/raw.c | 15 +---
block/sheepdog.c | 125 +++++++++++++-----------------
block_int.h | 6 +-
9 files changed, 186 insertions(+), 294 deletions(-)
diff --git a/block.c b/block.c
index 25ce28b..7a0eed7 100644
--- a/block.c
+++ b/block.c
@@ -49,21 +49,17 @@
#define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
typedef enum {
- BDRV_REQ_COPY_ON_READ = 0x1,
- BDRV_REQ_ZERO_WRITE = 0x2,
+ BDRV_REQ_READ = 0x0 /* the same as false */,
+ BDRV_REQ_WRITE = 0x1 /* the same as true */,
+ BDRV_REQ_COPY_ON_READ = 0x2,
+ BDRV_REQ_ZERO_WRITE = 0x4,
} BdrvRequestFlags;
static void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load);
-static int coroutine_fn bdrv_co_readv_em(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors,
- QEMUIOVector *iov);
-static int coroutine_fn bdrv_co_writev_em(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors,
- QEMUIOVector *iov);
-static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
- BdrvRequestFlags flags);
-static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
+static int coroutine_fn bdrv_co_io_em(BlockDriverState *bs, int64_t sector_num,
+ int nb_sectors, QEMUIOVector *iov,
+ bool is_write);
+static int coroutine_fn bdrv_co_do_rw_vector(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
BdrvRequestFlags flags);
static BlockDriverAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
@@ -267,9 +263,8 @@ void path_combine(char *dest, int dest_size,
void bdrv_register(BlockDriver *bdrv)
{
/* Block drivers without coroutine functions need emulation */
- if (!bdrv->bdrv_co_readv) {
- bdrv->bdrv_co_readv = bdrv_co_readv_em;
- bdrv->bdrv_co_writev = bdrv_co_writev_em;
+ if (!bdrv->bdrv_co_rw_vector) {
+ bdrv->bdrv_co_rw_vector = bdrv_co_io_em;
/* bdrv_co_readv_em()/brdv_co_writev_em() work in terms of aio, so if
* the block driver lacks aio we need to emulate that too.
@@ -1338,14 +1333,9 @@ typedef struct RwCo {
static void coroutine_fn bdrv_rw_co_entry(void *opaque)
{
RwCo *rwco = opaque;
-
- if (!rwco->is_write) {
- rwco->ret = bdrv_co_do_readv(rwco->bs, rwco->sector_num,
- rwco->nb_sectors, rwco->qiov, 0);
- } else {
- rwco->ret = bdrv_co_do_writev(rwco->bs, rwco->sector_num,
- rwco->nb_sectors, rwco->qiov, 0);
- }
+ rwco->ret = bdrv_co_do_rw_vector(rwco->bs, rwco->sector_num,
+ rwco->nb_sectors, rwco->qiov,
+ rwco->is_write ? BDRV_REQ_WRITE : BDRV_REQ_READ);
}
/*
@@ -1580,8 +1570,8 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BlockDriverState *bs,
iov.iov_base = bounce_buffer = qemu_blockalign(bs, iov.iov_len);
qemu_iovec_init_external(&bounce_qiov, &iov, 1);
- ret = drv->bdrv_co_readv(bs, cluster_sector_num, cluster_nb_sectors,
- &bounce_qiov);
+ ret = drv->bdrv_co_rw_vector(bs, cluster_sector_num, cluster_nb_sectors,
+ &bounce_qiov, BDRV_READ);
if (ret < 0) {
goto err;
}
@@ -1591,8 +1581,8 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BlockDriverState *bs,
ret = drv->bdrv_co_write_zeroes(bs, cluster_sector_num,
cluster_nb_sectors);
} else {
- ret = drv->bdrv_co_writev(bs, cluster_sector_num, cluster_nb_sectors,
- &bounce_qiov);
+ ret = drv->bdrv_co_rw_vector(bs, cluster_sector_num, cluster_nb_sectors,
+ &bounce_qiov, BDRV_WRITE);
}
if (ret < 0) {
@@ -1613,29 +1603,36 @@ err:
}
/*
- * Handle a read request in coroutine context
+ * Handle a read or write request in coroutine context
+ * This is internal function which is called with flags being
+ * one of READ,WRITE,COPY_ON_READ,ZERO_WRITE, but not any combination.
+ * For ZERO_WRITE, the data (qiov) is not provided.
*/
-static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
+static int coroutine_fn bdrv_co_do_rw_vector(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
BdrvRequestFlags flags)
{
BlockDriver *drv = bs->drv;
BdrvTrackedRequest req;
+ bool is_write = flags & (BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE);
int ret;
if (!drv) {
return -ENOMEDIUM;
}
+ if (is_write && bs->read_only) {
+ return -EACCES;
+ }
if (bdrv_check_request(bs, sector_num, nb_sectors)) {
return -EIO;
}
/* throttling disk read I/O */
if (bs->io_limits_enabled) {
- bdrv_io_limits_intercept(bs, false, nb_sectors);
+ bdrv_io_limits_intercept(bs, is_write, nb_sectors);
}
- if (bs->copy_on_read) {
+ if (!is_write && bs->copy_on_read) {
flags |= BDRV_REQ_COPY_ON_READ;
}
if (flags & BDRV_REQ_COPY_ON_READ) {
@@ -1646,7 +1643,7 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
wait_for_overlapping_requests(bs, sector_num, nb_sectors);
}
- tracked_request_begin(&req, bs, sector_num, nb_sectors, false);
+ tracked_request_begin(&req, bs, sector_num, nb_sectors, is_write);
if (flags & BDRV_REQ_COPY_ON_READ) {
int pnum;
@@ -1662,7 +1659,39 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
}
}
- ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
+ if (flags & BDRV_REQ_ZERO_WRITE) {
+
+ /* First try the efficient write zeroes operation */
+ if (drv->bdrv_co_write_zeroes) {
+ ret = drv->bdrv_co_write_zeroes(bs, sector_num, nb_sectors);
+ } else {
+ /* Fall back to bounce buffer if write zeroes is unsupported */
+ QEMUIOVector qiov;
+ struct iovec iov;
+ /*XXX may do better by allocating small buffer and repeating it several times */
+ iov.iov_len = nb_sectors * BDRV_SECTOR_SIZE;
+ iov.iov_base = qemu_blockalign(bs, iov.iov_len);
+ memset(iov.iov_base, 0, iov.iov_len);
+ qemu_iovec_init_external(&qiov, &iov, 1);
+
+ ret = drv->bdrv_co_rw_vector(bs, sector_num, nb_sectors, &qiov, BDRV_WRITE);
+
+ qemu_vfree(iov.iov_base);
+ }
+
+ } else {
+ ret = drv->bdrv_co_rw_vector(bs, sector_num, nb_sectors, qiov, is_write);
+ }
+
+ if (is_write) {
+ if (bs->dirty_bitmap) {
+ set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
+ }
+
+ if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
+ bs->wr_highest_sector = sector_num + nb_sectors - 1;
+ }
+ }
out:
tracked_request_end(&req);
@@ -1679,7 +1708,7 @@ int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
{
trace_bdrv_co_readv(bs, sector_num, nb_sectors);
- return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov, 0);
+ return bdrv_co_do_rw_vector(bs, sector_num, nb_sectors, qiov, BDRV_REQ_READ);
}
int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
@@ -1687,92 +1716,27 @@ int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
{
trace_bdrv_co_copy_on_readv(bs, sector_num, nb_sectors);
- return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov,
- BDRV_REQ_COPY_ON_READ);
+ return bdrv_co_do_rw_vector(bs, sector_num, nb_sectors, qiov,
+ BDRV_REQ_COPY_ON_READ);
}
-static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors)
+int coroutine_fn bdrv_co_rw_vector(BlockDriverState *bs, int64_t sector_num,
+ int nb_sectors, QEMUIOVector *qiov, bool is_write)
{
- BlockDriver *drv = bs->drv;
- QEMUIOVector qiov;
- struct iovec iov;
- int ret;
-
- /* First try the efficient write zeroes operation */
- if (drv->bdrv_co_write_zeroes) {
- return drv->bdrv_co_write_zeroes(bs, sector_num, nb_sectors);
- }
-
- /* Fall back to bounce buffer if write zeroes is unsupported */
- iov.iov_len = nb_sectors * BDRV_SECTOR_SIZE;
- iov.iov_base = qemu_blockalign(bs, iov.iov_len);
- memset(iov.iov_base, 0, iov.iov_len);
- qemu_iovec_init_external(&qiov, &iov, 1);
-
- ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, &qiov);
-
- qemu_vfree(iov.iov_base);
- return ret;
+ return bdrv_co_do_rw_vector(bs, sector_num, nb_sectors, qiov,
+ is_write ? BDRV_REQ_WRITE : BDRV_REQ_READ);
}
/*
* Handle a write request in coroutine context
*/
-static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
- BdrvRequestFlags flags)
-{
- BlockDriver *drv = bs->drv;
- BdrvTrackedRequest req;
- int ret;
-
- if (!bs->drv) {
- return -ENOMEDIUM;
- }
- if (bs->read_only) {
- return -EACCES;
- }
- if (bdrv_check_request(bs, sector_num, nb_sectors)) {
- return -EIO;
- }
-
- /* throttling disk write I/O */
- if (bs->io_limits_enabled) {
- bdrv_io_limits_intercept(bs, true, nb_sectors);
- }
-
- if (bs->copy_on_read_in_flight) {
- wait_for_overlapping_requests(bs, sector_num, nb_sectors);
- }
-
- tracked_request_begin(&req, bs, sector_num, nb_sectors, true);
-
- if (flags & BDRV_REQ_ZERO_WRITE) {
- ret = bdrv_co_do_write_zeroes(bs, sector_num, nb_sectors);
- } else {
- ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
- }
-
- if (bs->dirty_bitmap) {
- set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
- }
-
- if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
- bs->wr_highest_sector = sector_num + nb_sectors - 1;
- }
-
- tracked_request_end(&req);
-
- return ret;
-}
-
int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *qiov)
{
trace_bdrv_co_writev(bs, sector_num, nb_sectors);
- return bdrv_co_do_writev(bs, sector_num, nb_sectors, qiov, 0);
+ return bdrv_co_do_rw_vector(bs, sector_num, nb_sectors, qiov,
+ BDRV_REQ_WRITE);
}
int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
@@ -1780,8 +1744,8 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
{
trace_bdrv_co_write_zeroes(bs, sector_num, nb_sectors);
- return bdrv_co_do_writev(bs, sector_num, nb_sectors, NULL,
- BDRV_REQ_ZERO_WRITE);
+ return bdrv_co_do_rw_vector(bs, sector_num, nb_sectors, NULL,
+ BDRV_REQ_ZERO_WRITE);
}
/**
@@ -3232,13 +3196,9 @@ static void coroutine_fn bdrv_co_do_rw(void *opaque)
BlockDriverAIOCBCoroutine *acb = opaque;
BlockDriverState *bs = acb->common.bs;
- if (!acb->is_write) {
- acb->req.error = bdrv_co_do_readv(bs, acb->req.sector,
- acb->req.nb_sectors, acb->req.qiov, 0);
- } else {
- acb->req.error = bdrv_co_do_writev(bs, acb->req.sector,
- acb->req.nb_sectors, acb->req.qiov, 0);
- }
+ acb->req.error = bdrv_co_do_rw_vector(bs, acb->req.sector,
+ acb->req.nb_sectors, acb->req.qiov,
+ acb->is_write ? BDRV_REQ_WRITE : BDRV_REQ_READ);
acb->bh = qemu_bh_new(bdrv_co_em_bh, acb);
qemu_bh_schedule(acb->bh);
@@ -3394,20 +3354,6 @@ static int coroutine_fn bdrv_co_io_em(BlockDriverState *bs, int64_t sector_num,
return co.ret;
}
-static int coroutine_fn bdrv_co_readv_em(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors,
- QEMUIOVector *iov)
-{
- return bdrv_co_io_em(bs, sector_num, nb_sectors, iov, false);
-}
-
-static int coroutine_fn bdrv_co_writev_em(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors,
- QEMUIOVector *iov)
-{
- return bdrv_co_io_em(bs, sector_num, nb_sectors, iov, true);
-}
-
static void coroutine_fn bdrv_flush_co_entry(void *opaque)
{
RwCo *rwco = opaque;
diff --git a/block.h b/block.h
index f0f68e0..f15d3ce 100644
--- a/block.h
+++ b/block.h
@@ -78,6 +78,10 @@ typedef struct BlockDevOps {
#define BDRV_SECTOR_SIZE (1ULL << BDRV_SECTOR_BITS)
#define BDRV_SECTOR_MASK ~(BDRV_SECTOR_SIZE - 1)
+/* defines for is_write for bdrv_*_rw_vector */
+#define BDRV_READ false
+#define BDRV_WRITE true
+
typedef enum {
BLOCK_ERR_REPORT, BLOCK_ERR_IGNORE, BLOCK_ERR_STOP_ENOSPC,
BLOCK_ERR_STOP_ANY
@@ -140,6 +144,8 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
const void *buf, int count);
int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
const void *buf, int count);
+int coroutine_fn bdrv_co_rw_vector(BlockDriverState *bs, int64_t sector_num,
+ int nb_sectors, QEMUIOVector *qiov, bool is_write);
int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *qiov);
int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
diff --git a/block/nbd.c b/block/nbd.c
index 161b299..7b757bb 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -320,91 +320,45 @@ static int nbd_open(BlockDriverState *bs, const char* filename, int flags)
return result;
}
-static int nbd_co_readv_1(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, QEMUIOVector *qiov,
- int offset)
-{
- BDRVNBDState *s = bs->opaque;
- struct nbd_request request;
- struct nbd_reply reply;
-
- request.type = NBD_CMD_READ;
- request.from = sector_num * 512;
- request.len = nb_sectors * 512;
-
- nbd_coroutine_start(s, &request);
- if (nbd_co_send_request(s, &request, NULL, 0) == -1) {
- reply.error = errno;
- } else {
- nbd_co_receive_reply(s, &request, &reply, qiov->iov, offset);
- }
- nbd_coroutine_end(s, &request);
- return -reply.error;
-
-}
+/* qemu-nbd has a limit of slightly less than 1M per request. Try to
+ * remain aligned to 4K. */
+#define NBD_MAX_SECTORS 2040
-static int nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, QEMUIOVector *qiov,
- int offset)
+static int nbd_co_rw_vector(BlockDriverState *bs, int64_t sector_num,
+ int nb_sectors, QEMUIOVector *qiov, bool is_write)
{
BDRVNBDState *s = bs->opaque;
+ int offset = 0;
struct nbd_request request;
struct nbd_reply reply;
- request.type = NBD_CMD_WRITE;
- if (!bdrv_enable_write_cache(bs) && (s->nbdflags & NBD_FLAG_SEND_FUA)) {
+ request.type = is_write ? NBD_CMD_WRITE : NBD_CMD_READ;
+ if (is_write && !bdrv_enable_write_cache(bs) && (s->nbdflags & NBD_FLAG_SEND_FUA)) {
request.type |= NBD_CMD_FLAG_FUA;
}
+ reply.error = 0;
- request.from = sector_num * 512;
- request.len = nb_sectors * 512;
+ do {
- nbd_coroutine_start(s, &request);
- if (nbd_co_send_request(s, &request, qiov->iov, offset) == -1) {
- reply.error = errno;
- } else {
- nbd_co_receive_reply(s, &request, &reply, NULL, 0);
- }
- nbd_coroutine_end(s, &request);
- return -reply.error;
-}
+ request.from = sector_num * 512;
+ request.len = MIN(nb_sectors, NBD_MAX_SECTORS) * 512;
-/* qemu-nbd has a limit of slightly less than 1M per request. Try to
- * remain aligned to 4K. */
-#define NBD_MAX_SECTORS 2040
+ nbd_coroutine_start(s, &request);
+ /* send_request() and receive_reply() ignores offset if iov is NULL */
+ if (nbd_co_send_request(s, &request, is_write ? qiov->iov : NULL, offset) == -1) {
+ reply.error = errno;
+ } else {
+ nbd_co_receive_reply(s, &request, &reply, is_write ? NULL : qiov->iov, offset);
+ }
+ nbd_coroutine_end(s, &request);
-static int nbd_co_readv(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, QEMUIOVector *qiov)
-{
- int offset = 0;
- int ret;
- while (nb_sectors > NBD_MAX_SECTORS) {
- ret = nbd_co_readv_1(bs, sector_num, NBD_MAX_SECTORS, qiov, offset);
- if (ret < 0) {
- return ret;
- }
offset += NBD_MAX_SECTORS * 512;
sector_num += NBD_MAX_SECTORS;
nb_sectors -= NBD_MAX_SECTORS;
- }
- return nbd_co_readv_1(bs, sector_num, nb_sectors, qiov, offset);
-}
-static int nbd_co_writev(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, QEMUIOVector *qiov)
-{
- int offset = 0;
- int ret;
- while (nb_sectors > NBD_MAX_SECTORS) {
- ret = nbd_co_writev_1(bs, sector_num, NBD_MAX_SECTORS, qiov, offset);
- if (ret < 0) {
- return ret;
- }
- offset += NBD_MAX_SECTORS * 512;
- sector_num += NBD_MAX_SECTORS;
- nb_sectors -= NBD_MAX_SECTORS;
- }
- return nbd_co_writev_1(bs, sector_num, nb_sectors, qiov, offset);
+ } while (reply.error == 0 && nb_sectors > 0);
+
+ return -reply.error;
}
static int nbd_co_flush(BlockDriverState *bs)
@@ -479,8 +433,7 @@ static BlockDriver bdrv_nbd = {
.format_name = "nbd",
.instance_size = sizeof(BDRVNBDState),
.bdrv_file_open = nbd_open,
- .bdrv_co_readv = nbd_co_readv,
- .bdrv_co_writev = nbd_co_writev,
+ .bdrv_co_rw_vector = nbd_co_rw_vector,
.bdrv_close = nbd_close,
.bdrv_co_flush_to_os = nbd_co_flush,
.bdrv_co_discard = nbd_co_discard,
diff --git a/block/qcow.c b/block/qcow.c
index b1cfe1f..1c4191a 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -629,6 +629,13 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
return ret;
}
+static coroutine_fn int qcow_co_rw_vector(BlockDriverState *bs, int64_t sector_num,
+ int nb_sectors, QEMUIOVector *qiov, bool is_write)
+{
+ return is_write ? qcow_co_writev(bs, sector_num, nb_sectors, qiov)
+ : qcow_co_readv(bs, sector_num, nb_sectors, qiov);
+}
+
static void qcow_close(BlockDriverState *bs)
{
BDRVQcowState *s = bs->opaque;
@@ -875,8 +882,7 @@ static BlockDriver bdrv_qcow = {
.bdrv_close = qcow_close,
.bdrv_create = qcow_create,
- .bdrv_co_readv = qcow_co_readv,
- .bdrv_co_writev = qcow_co_writev,
+ .bdrv_co_rw_vector = qcow_co_rw_vector,
.bdrv_co_flush_to_disk = qcow_co_flush,
.bdrv_co_is_allocated = qcow_co_is_allocated,
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 07a2e93..b5011e0 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -320,11 +320,11 @@ static int coroutine_fn copy_sectors(BlockDriverState *bs,
BLKDBG_EVENT(bs->file, BLKDBG_COW_READ);
- /* Call .bdrv_co_readv() directly instead of using the public block-layer
+ /* Call .bdrv_co_rw_vector() directly instead of using the public block-layer
* interface. This avoids double I/O throttling and request tracking,
* which can lead to deadlock when block layer copy-on-read is enabled.
*/
- ret = bs->drv->bdrv_co_readv(bs, start_sect + n_start, n, &qiov);
+ ret = bs->drv->bdrv_co_rw_vector(bs, start_sect + n_start, n, &qiov, false);
if (ret < 0) {
goto out;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 3692b45..517c443 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -648,6 +648,16 @@ fail:
return ret;
}
+static coroutine_fn int qcow2_co_rw_vector(BlockDriverState *bs,
+ int64_t sector_num,
+ int nb_sectors,
+ QEMUIOVector *qiov, bool is_write)
+{
+ return is_write ? qcow2_co_writev(bs, sector_num, nb_sectors, qiov)
+ : qcow2_co_readv(bs, sector_num, nb_sectors, qiov);
+}
+
+
static void qcow2_close(BlockDriverState *bs)
{
BDRVQcowState *s = bs->opaque;
@@ -1359,8 +1369,7 @@ static BlockDriver bdrv_qcow2 = {
.bdrv_set_key = qcow2_set_key,
.bdrv_make_empty = qcow2_make_empty,
- .bdrv_co_readv = qcow2_co_readv,
- .bdrv_co_writev = qcow2_co_writev,
+ .bdrv_co_rw_vector = qcow2_co_rw_vector,
.bdrv_co_flush_to_os = qcow2_co_flush_to_os,
.bdrv_co_flush_to_disk = qcow2_co_flush_to_disk,
diff --git a/block/raw.c b/block/raw.c
index 1cdac0c..5348553 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -9,16 +9,10 @@ static int raw_open(BlockDriverState *bs, int flags)
return 0;
}
-static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, QEMUIOVector *qiov)
+static int coroutine_fn raw_co_rw_vector(BlockDriverState *bs, int64_t sector_num,
+ int nb_sectors, QEMUIOVector *qiov, bool is_write)
{
- return bdrv_co_readv(bs->file, sector_num, nb_sectors, qiov);
-}
-
-static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, QEMUIOVector *qiov)
-{
- return bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
+ return bdrv_co_rw_vector(bs->file, sector_num, nb_sectors, qiov, is_write);
}
static void raw_close(BlockDriverState *bs)
@@ -111,8 +105,7 @@ static BlockDriver bdrv_raw = {
.bdrv_open = raw_open,
.bdrv_close = raw_close,
- .bdrv_co_readv = raw_co_readv,
- .bdrv_co_writev = raw_co_writev,
+ .bdrv_co_rw_vector = raw_co_rw_vector,
.bdrv_co_flush_to_disk = raw_co_flush,
.bdrv_co_discard = raw_co_discard,
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 00276f6f..d768dbf 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1419,28 +1419,60 @@ out:
* Returns 1 when we need to wait a response, 0 when there is no sent
* request and -errno in error cases.
*/
-static int coroutine_fn sd_co_rw_vector(void *p)
+static coroutine_fn int sd_co_rw_vector(BlockDriverState *bs, int64_t sector_num,
+ int nb_sectors, QEMUIOVector *qiov, bool is_write)
{
- SheepdogAIOCB *acb = p;
+ SheepdogAIOCB *acb;
int ret = 0;
- unsigned long len, done = 0, total = acb->nb_sectors * SECTOR_SIZE;
- unsigned long idx = acb->sector_num * SECTOR_SIZE / SD_DATA_OBJ_SIZE;
+ unsigned long len, done = 0, total = nb_sectors * SECTOR_SIZE;
+ unsigned long idx = sector_num * SECTOR_SIZE / SD_DATA_OBJ_SIZE;
uint64_t oid;
- uint64_t offset = (acb->sector_num * SECTOR_SIZE) % SD_DATA_OBJ_SIZE;
- BDRVSheepdogState *s = acb->common.bs->opaque;
+ uint64_t offset = (sector_num * SECTOR_SIZE) % SD_DATA_OBJ_SIZE;
+ BDRVSheepdogState *s = bs->opaque;
SheepdogInode *inode = &s->inode;
AIOReq *aio_req;
- if (acb->aiocb_type == AIOCB_WRITE_UDATA && s->is_snapshot) {
+ if (is_write && bs->growable && sector_num + nb_sectors > bs->total_sectors) {
+ /* TODO: shouldn't block here */
+ if (sd_truncate(bs, (sector_num + nb_sectors) * SECTOR_SIZE) < 0) {
+ return -EIO;
+ }
+ bs->total_sectors = sector_num + nb_sectors;
+ }
+
+ acb = sd_aio_setup(bs, qiov, sector_num, nb_sectors, NULL, NULL);
+
+ if (is_write) {
+
+ acb->aio_done_func = sd_write_done;
+ acb->aiocb_type = AIOCB_WRITE_UDATA;
+
+ if (s->is_snapshot) {
+ /*
+ * In the case we open the snapshot VDI, Sheepdog creates the
+ * writable VDI when we do a write operation first.
+ */
+ ret = sd_create_branch(s);
+ if (ret) {
+ acb->ret = -EIO;
+ goto out;
+ }
+ }
+
+ } else { /* read */
+
+ int i;
+ acb->aiocb_type = AIOCB_READ_UDATA;
+ acb->aio_done_func = sd_finish_aiocb;
+
/*
- * In the case we open the snapshot VDI, Sheepdog creates the
- * writable VDI when we do a write operation first.
+ * TODO: we can do better; we don't need to initialize
+ * blindly.
*/
- ret = sd_create_branch(s);
- if (ret) {
- acb->ret = -EIO;
- goto out;
+ for (i = 0; i < qiov->niov; i++) {
+ memset(qiov->iov[i].iov_base, 0, qiov->iov[i].iov_len);
}
+
}
while (done != total) {
@@ -1453,12 +1485,12 @@ static int coroutine_fn sd_co_rw_vector(void *p)
len = MIN(total - done, SD_DATA_OBJ_SIZE - offset);
if (!inode->data_vdi_id[idx]) {
- if (acb->aiocb_type == AIOCB_READ_UDATA) {
+ if (!is_write) {
goto done;
}
create = 1;
- } else if (acb->aiocb_type == AIOCB_WRITE_UDATA
+ } else if (is_write
&& !is_data_obj_writable(inode, idx)) {
/* Copy-On-Write */
create = 1;
@@ -1511,63 +1543,13 @@ static int coroutine_fn sd_co_rw_vector(void *p)
done += len;
}
out:
- if (QLIST_EMPTY(&acb->aioreq_head)) {
- return acb->ret;
- }
- return 1;
-}
-
-static coroutine_fn int sd_co_writev(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, QEMUIOVector *qiov)
-{
- SheepdogAIOCB *acb;
- int ret;
- if (bs->growable && sector_num + nb_sectors > bs->total_sectors) {
- /* TODO: shouldn't block here */
- if (sd_truncate(bs, (sector_num + nb_sectors) * SECTOR_SIZE) < 0) {
- return -EIO;
- }
- bs->total_sectors = sector_num + nb_sectors;
- }
-
- acb = sd_aio_setup(bs, qiov, sector_num, nb_sectors, NULL, NULL);
- acb->aio_done_func = sd_write_done;
- acb->aiocb_type = AIOCB_WRITE_UDATA;
-
- ret = sd_co_rw_vector(acb);
- if (ret <= 0) {
- qemu_aio_release(acb);
- return ret;
- }
-
- qemu_coroutine_yield();
- return acb->ret;
-}
-
-static coroutine_fn int sd_co_readv(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, QEMUIOVector *qiov)
-{
- SheepdogAIOCB *acb;
- int i, ret;
-
- acb = sd_aio_setup(bs, qiov, sector_num, nb_sectors, NULL, NULL);
- acb->aiocb_type = AIOCB_READ_UDATA;
- acb->aio_done_func = sd_finish_aiocb;
-
- /*
- * TODO: we can do better; we don't need to initialize
- * blindly.
- */
- for (i = 0; i < qiov->niov; i++) {
- memset(qiov->iov[i].iov_base, 0, qiov->iov[i].iov_len);
- }
-
- ret = sd_co_rw_vector(acb);
- if (ret <= 0) {
- qemu_aio_release(acb);
- return ret;
+ if (QLIST_EMPTY(&acb->aioreq_head)) {
+ if (acb->ret <= 0) {
+ qemu_aio_release(acb);
+ return acb->ret;
+ }
}
qemu_coroutine_yield();
@@ -1902,8 +1884,7 @@ BlockDriver bdrv_sheepdog = {
.bdrv_getlength = sd_getlength,
.bdrv_truncate = sd_truncate,
- .bdrv_co_readv = sd_co_readv,
- .bdrv_co_writev = sd_co_writev,
+ .bdrv_co_rw_vector = sd_co_rw_vector,
.bdrv_snapshot_create = sd_snapshot_create,
.bdrv_snapshot_goto = sd_snapshot_goto,
diff --git a/block_int.h b/block_int.h
index aeafa2e..146d9b7 100644
--- a/block_int.h
+++ b/block_int.h
@@ -122,10 +122,8 @@ struct BlockDriver {
int64_t sector_num, int nb_sectors,
BlockDriverCompletionFunc *cb, void *opaque);
- int coroutine_fn (*bdrv_co_readv)(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
- int coroutine_fn (*bdrv_co_writev)(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
+ int coroutine_fn (*bdrv_co_rw_vector)(BlockDriverState *bs,
+ int64_t sector_num, int nb_sectors, QEMUIOVector *qiov, bool is_write);
/*
* Efficiently zero a region of the disk image. Typically an image format
* would use a compact metadata representation to implement this. This
--
1.7.9
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH 3/3] Combine bdrv_co_readv and bdrv_co_writev into bdrv_co_rw_vector
2012-02-28 23:54 ` [Qemu-devel] [PATCH 3/3] Combine bdrv_co_readv and bdrv_co_writev into bdrv_co_rw_vector Michael Tokarev
@ 2012-02-29 16:01 ` Paolo Bonzini
2012-02-29 16:12 ` Michael Tokarev
0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2012-02-29 16:01 UTC (permalink / raw)
To: Michael Tokarev; +Cc: kwolf, qemu-devel
Il 29/02/2012 00:54, Michael Tokarev ha scritto:
> BlockDriver *drv = bs->drv;
> BdrvTrackedRequest req;
> + bool is_write = flags & (BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE);
> int ret;
You can do BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE, but not
BDRV_REQ_READ|BDRV_REQ_COPY_ON_READ. That's ugly.
> +/* defines for is_write for bdrv_*_rw_vector */
> +#define BDRV_READ false
> +#define BDRV_WRITE true
> +
Please no, if you have to do this just change to bits. This would have
the advantage of passing all the flags, including COPY_ON_READ. In some
sense discard could be treated as a write too.
I don't oppose this change completely, in fact I think adding the flags
to co_readv/co_writev would be a good change. But I'm skeptical, the
actual amount of unification is not that large.
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] Combine bdrv_co_readv and bdrv_co_writev into bdrv_co_rw_vector
2012-02-29 16:01 ` Paolo Bonzini
@ 2012-02-29 16:12 ` Michael Tokarev
2012-02-29 16:24 ` Paolo Bonzini
0 siblings, 1 reply; 13+ messages in thread
From: Michael Tokarev @ 2012-02-29 16:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel
On 29.02.2012 20:01, Paolo Bonzini wrote:
> Il 29/02/2012 00:54, Michael Tokarev ha scritto:
>> BlockDriver *drv = bs->drv;
>> BdrvTrackedRequest req;
>> + bool is_write = flags & (BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE);
>> int ret;
>
> You can do BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE, but not
> BDRV_REQ_READ|BDRV_REQ_COPY_ON_READ. That's ugly.
BDRV_REQ_READ is zero. This is just mnemonic to avoid "magic
numbers" elsewhere in the code. This is an internal function
and the comment above it says just that, and it is always
called with just ONE value. It is not a bitmask, it is used
as such inside this very routine ONLY. The argument is declared
as enum too, -- this should tell something. In the function
prototype it should have been named "opcode" or "request",
not "flags". It is used as flags only inside this function.
This code isn't written by me, it was this way before.
I just added 2 more possible values for this parameter.
And as I mentioned, this place is the most questionable
due to its relative ugliness (but the result is much more
understandable, IMHO anyway). This ugliness comes from
the original code, I tried to not change it much.
>> +/* defines for is_write for bdrv_*_rw_vector */
>> +#define BDRV_READ false
>> +#define BDRV_WRITE true
>> +
>
> Please no, if you have to do this just change to bits. This would have
> the advantage of passing all the flags, including COPY_ON_READ. In some
> sense discard could be treated as a write too.
No block driver -- at least currently -- needs any other value
here except of read-or-write (or is_write). COPY_ON_READ is
not a business of the individual block drivers currently.
These defines are _only_ to make some code a bit more readable,
in a very few places where it necessary to call individual
read or write block driver method. So that the construct:
ret - s->bdrv_co_rw_vector(bs, ..., true)
becomes
ret - s->bdrv_co_rw_vector(bs, ..., BDRV_WRITE)
and it is immediately obvious that it is write. THe prototype
of the method has "bool is_write" here.
> I don't oppose this change completely, in fact I think adding the flags
> to co_readv/co_writev would be a good change.
The series is an RCF for the _current_ situation, which does not
pass any flags. Which other flags do you want to pass?
> But I'm skeptical, the
> actual amount of unification is not that large.
This is not about unification. This is, as described in the introduction
email, about removing complexity of a very twisted nature of read and
write code paths, for a start.
Thanks,
/mjt
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] Combine bdrv_co_readv and bdrv_co_writev into bdrv_co_rw_vector
2012-02-29 16:12 ` Michael Tokarev
@ 2012-02-29 16:24 ` Paolo Bonzini
2012-02-29 16:45 ` Michael Tokarev
0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2012-02-29 16:24 UTC (permalink / raw)
To: Michael Tokarev; +Cc: kwolf, qemu-devel
Il 29/02/2012 17:12, Michael Tokarev ha scritto:
> On 29.02.2012 20:01, Paolo Bonzini wrote:
>> Il 29/02/2012 00:54, Michael Tokarev ha scritto:
>>> BlockDriver *drv = bs->drv;
>>> BdrvTrackedRequest req;
>>> + bool is_write = flags & (BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE);
>>> int ret;
>>
>> You can do BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE, but not
>> BDRV_REQ_READ|BDRV_REQ_COPY_ON_READ. That's ugly.
>
> BDRV_REQ_READ is zero. This is just mnemonic to avoid "magic
> numbers" elsewhere in the code. This is an internal function
> and the comment above it says just that, and it is always
> called with just ONE value. It is not a bitmask, it is used
> as such inside this very routine ONLY. The argument is declared
> as enum too, -- this should tell something. In the function
> prototype it should have been named "opcode" or "request",
> not "flags". It is used as flags only inside this function.
>
> This code isn't written by me, it was this way before.
> I just added 2 more possible values for this parameter.
If you have 4 values, make them 1/2/4/8 or 0/1/2/3. Not 0/1/2/4.
> No block driver -- at least currently -- needs any other value
> here except of read-or-write (or is_write). COPY_ON_READ is
> not a business of the individual block drivers currently.
Sure, but ZERO_WRITES is (we have a separate callback).
> These defines are _only_ to make some code a bit more readable,
> in a very few places where it necessary to call individual
> read or write block driver method. So that the construct:
>
> ret - s->bdrv_co_rw_vector(bs, ..., true)
>
> becomes
>
> ret - s->bdrv_co_rw_vector(bs, ..., BDRV_WRITE)
>
> and it is immediately obvious that it is write. The prototype
> of the method has "bool is_write" here.
If you use an enum, the prototype shouldn't be bool.
>> But I'm skeptical, the
>> actual amount of unification is not that large.
>
> This is not about unification. This is, as described in the introduction
> email, about removing complexity of a very twisted nature of read and
> write code paths, for a start.
The patches are a balance of removing duplication and adding
conditionals, no? Removing duplication is unification.
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] Combine bdrv_co_readv and bdrv_co_writev into bdrv_co_rw_vector
2012-02-29 16:24 ` Paolo Bonzini
@ 2012-02-29 16:45 ` Michael Tokarev
0 siblings, 0 replies; 13+ messages in thread
From: Michael Tokarev @ 2012-02-29 16:45 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel
On 29.02.2012 20:24, Paolo Bonzini wrote:
> Il 29/02/2012 17:12, Michael Tokarev ha scritto:
>> On 29.02.2012 20:01, Paolo Bonzini wrote:
>>> Il 29/02/2012 00:54, Michael Tokarev ha scritto:
>>>> BlockDriver *drv = bs->drv;
>>>> BdrvTrackedRequest req;
>>>> + bool is_write = flags & (BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE);
>>>> int ret;
>>>
>>> You can do BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE, but not
>>> BDRV_REQ_READ|BDRV_REQ_COPY_ON_READ. That's ugly.
>>
>> BDRV_REQ_READ is zero. This is just mnemonic to avoid "magic
>> numbers" elsewhere in the code. This is an internal function
>> and the comment above it says just that, and it is always
>> called with just ONE value. It is not a bitmask, it is used
>> as such inside this very routine ONLY. The argument is declared
>> as enum too, -- this should tell something. In the function
>> prototype it should have been named "opcode" or "request",
>> not "flags". It is used as flags only inside this function.
>>
>> This code isn't written by me, it was this way before.
>> I just added 2 more possible values for this parameter.
>
> If you have 4 values, make them 1/2/4/8 or 0/1/2/3. Not 0/1/2/4.
You didn't read what I wrote, did you?
In the _original_ code, this very enum was ALREADY used as
a bitmask, but only inside this routine. I can change that,
but it has nothing to do with the patch in question. Making
it 0/1/2/3 will break that.
And yet again, I dislike this code myself, and I mentioned
this already, but that's why I put the "RFC" in the original
subject -- RFC for the general "idea".
>> No block driver -- at least currently -- needs any other value
>> here except of read-or-write (or is_write). COPY_ON_READ is
>> not a business of the individual block drivers currently.
>
> Sure, but ZERO_WRITES is (we have a separate callback).
I already replied about this. To me it looks like it is
better to keep it as separate, for several reasons:
very few drivers will actually implement it in a
reasonable way. By turning it into "request type"
of a common dispatcher method (like bdrv_rw) means
that each driver will have to recognize it and call
some common emulation routine, instead of letting
the upper layer to deal with it.
it matches much better the discard method instead of
rw method, so if we want to combine, lets' go there
instead.
it does not accept data, just like discard.
So we still end up with reads or writes. And using
bool instead of a enum for these is easier.
>
>> These defines are _only_ to make some code a bit more readable,
>> in a very few places where it necessary to call individual
>> read or write block driver method. So that the construct:
>>
>> ret - s->bdrv_co_rw_vector(bs, ..., true)
>>
>> becomes
>>
>> ret - s->bdrv_co_rw_vector(bs, ..., BDRV_WRITE)
>>
>> and it is immediately obvious that it is write. The prototype
>> of the method has "bool is_write" here.
>
> If you use an enum, the prototype shouldn't be bool.
I use a bool, not an enum. The parameter is called "is_write".
It is also used in lots of other places.
>>> But I'm skeptical, the
>>> actual amount of unification is not that large.
>>
>> This is not about unification. This is, as described in the introduction
>> email, about removing complexity of a very twisted nature of read and
>> write code paths, for a start.
>
> The patches are a balance of removing duplication and adding
> conditionals, no? Removing duplication is unification.
The unification is just a (good) side effect.
Thanks,
/mjt
^ permalink raw reply [flat|nested] 13+ messages in thread