diff mbox

[1/3] Combine bdrv_read and bdrv_write to bdrv_rw

Message ID 1330473276-8975-2-git-send-email-mjt@msgid.tls.msk.ru
State New
Headers show

Commit Message

Michael Tokarev Feb. 28, 2012, 11:54 p.m. UTC 
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(-)

Comments

Paolo Bonzini Feb. 29, 2012, 3:53 p.m. UTC | #1 
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
Michael Tokarev Feb. 29, 2012, 4 p.m. UTC | #2 
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
Paolo Bonzini Feb. 29, 2012, 4:07 p.m. UTC | #3 
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
Michael Tokarev Feb. 29, 2012, 4:36 p.m. UTC | #4 
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
diff mbox

Patch

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