diff mbox

[06/17] bochs: Implement .bdrv_co_preadv() interface

Message ID 1461750767-23273-7-git-send-email-kwolf@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kevin Wolf April 27, 2016, 9:52 a.m. UTC
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/bochs.c | 46 +++++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 17 deletions(-)

Comments

Stefan Hajnoczi April 27, 2016, 2:06 p.m. UTC | #1
On Wed, Apr 27, 2016 at 11:52:36AM +0200, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/bochs.c | 46 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 17 deletions(-)
> 
> diff --git a/block/bochs.c b/block/bochs.c
> index af8b7ab..d148454 100644
> --- a/block/bochs.c
> +++ b/block/bochs.c
> @@ -104,6 +104,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
>      int ret;
>  
>      bs->read_only = 1; // no write support yet
> +    bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */

Can this be the default in block.c?  Drivers that have other alignment
characteristics can set it explicitly but most drivers will want
BDRV_SECTOR_SIZE so it's nice to make it common code.

>  
>      ret = bdrv_pread(bs->file->bs, 0, &bochs, sizeof(bochs));
>      if (ret < 0) {
> @@ -221,39 +222,50 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
>      return bitmap_offset + (512 * (s->bitmap_blocks + extent_offset));
>  }
>  
> -static int bochs_read(BlockDriverState *bs, int64_t sector_num,
> -                    uint8_t *buf, int nb_sectors)
> +static int coroutine_fn
> +bochs_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> +                QEMUIOVector *qiov, int flags)
>  {
> +    BDRVBochsState *s = bs->opaque;
> +    uint64_t sector_num = offset >> BDRV_SECTOR_BITS;
> +    int nb_sectors = bytes >> BDRV_SECTOR_BITS;
> +    uint64_t bytes_done = 0;
> +    QEMUIOVector local_qiov;
>      int ret;
>  
> +    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> +    assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> +
> +    qemu_iovec_init(&local_qiov, qiov->niov);
> +    qemu_co_mutex_lock(&s->lock);
> +
>      while (nb_sectors > 0) {
>          int64_t block_offset = seek_to_sector(bs, sector_num);
>          if (block_offset < 0) {
>              return block_offset;

s->lock must be unlocked.

> -        } else if (block_offset > 0) {
> -            ret = bdrv_pread(bs->file->bs, block_offset, buf, 512);
> +        }
> +
> +        qemu_iovec_reset(&local_qiov);
> +        qemu_iovec_concat(&local_qiov, qiov, bytes_done, 512);
> +
> +        if (block_offset > 0) {
> +            ret = bdrv_co_preadv(bs->file->bs, block_offset, 512,
> +                                 &local_qiov, 0);
>              if (ret < 0) {
>                  return ret;

Same here.
Kevin Wolf April 27, 2016, 2:33 p.m. UTC | #2
Am 27.04.2016 um 16:06 hat Stefan Hajnoczi geschrieben:
> On Wed, Apr 27, 2016 at 11:52:36AM +0200, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/bochs.c | 46 +++++++++++++++++++++++++++++-----------------
> >  1 file changed, 29 insertions(+), 17 deletions(-)
> > 
> > diff --git a/block/bochs.c b/block/bochs.c
> > index af8b7ab..d148454 100644
> > --- a/block/bochs.c
> > +++ b/block/bochs.c
> > @@ -104,6 +104,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
> >      int ret;
> >  
> >      bs->read_only = 1; // no write support yet
> > +    bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */
> 
> Can this be the default in block.c?  Drivers that have other alignment
> characteristics can set it explicitly but most drivers will want
> BDRV_SECTOR_SIZE so it's nice to make it common code.

I absolutely don't expect BDRV_SECTOR_SIZE to be what most drivers will
want. It's basically a sign that a driver is either a protocol that
imposes restrictions (like O_DIRECT on raw-posix) or it is broken and we
don't care enough about the emulation overhead to fix it (like this one).

The expected value for most block drivers is 1.

> >      while (nb_sectors > 0) {
> >          int64_t block_offset = seek_to_sector(bs, sector_num);
> >          if (block_offset < 0) {
> >              return block_offset;
> 
> s->lock must be unlocked.

I can't believe I messed this up in so many places. Thanks for catching
this, I'll have to go through all patches and check the locking.

Kevin
Eric Blake April 27, 2016, 3:51 p.m. UTC | #3
On 04/27/2016 03:52 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/bochs.c | 46 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 17 deletions(-)
> 

>  static void bochs_close(BlockDriverState *bs)
> @@ -267,7 +279,7 @@ static BlockDriver bdrv_bochs = {
>      .instance_size	= sizeof(BDRVBochsState),
>      .bdrv_probe		= bochs_probe,
>      .bdrv_open		= bochs_open,
> -    .bdrv_read          = bochs_co_read,
> +    .bdrv_co_preadv = bochs_co_preadv,
>      .bdrv_close		= bochs_close,
>  };

Alignment is funky here.  I'd rather just get rid of all the extra
spaces, if that's easier than having half but not all of the = aligned.
Kevin Wolf April 28, 2016, 8:21 a.m. UTC | #4
Am 27.04.2016 um 17:51 hat Eric Blake geschrieben:
> On 04/27/2016 03:52 AM, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/bochs.c | 46 +++++++++++++++++++++++++++++-----------------
> >  1 file changed, 29 insertions(+), 17 deletions(-)
> > 
> 
> >  static void bochs_close(BlockDriverState *bs)
> > @@ -267,7 +279,7 @@ static BlockDriver bdrv_bochs = {
> >      .instance_size	= sizeof(BDRVBochsState),
> >      .bdrv_probe		= bochs_probe,
> >      .bdrv_open		= bochs_open,
> > -    .bdrv_read          = bochs_co_read,
> > +    .bdrv_co_preadv = bochs_co_preadv,
> >      .bdrv_close		= bochs_close,
> >  };
> 
> Alignment is funky here.  I'd rather just get rid of all the extra
> spaces, if that's easier than having half but not all of the = aligned.

Alignment is funky by definition when there are tabs involved and new
code follows the coding style and uses spaces instead. With a tab stop
of 4, this is aligned correctly.

The other option would be to just convert the whole struct to spaces,
but that's not really related to this patch and I think we're avoiding
pure style cleanup patches to keep things like 'git blame' useful.

Kevin
Markus Armbruster April 28, 2016, 8:42 a.m. UTC | #5
Kevin Wolf <kwolf@redhat.com> writes:

> Am 27.04.2016 um 17:51 hat Eric Blake geschrieben:
>> On 04/27/2016 03:52 AM, Kevin Wolf wrote:
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> >  block/bochs.c | 46 +++++++++++++++++++++++++++++-----------------
>> >  1 file changed, 29 insertions(+), 17 deletions(-)
>> > 
>> 
>> >  static void bochs_close(BlockDriverState *bs)
>> > @@ -267,7 +279,7 @@ static BlockDriver bdrv_bochs = {
>> >      .instance_size	= sizeof(BDRVBochsState),
>> >      .bdrv_probe		= bochs_probe,
>> >      .bdrv_open		= bochs_open,
>> > -    .bdrv_read          = bochs_co_read,
>> > +    .bdrv_co_preadv = bochs_co_preadv,
>> >      .bdrv_close		= bochs_close,
>> >  };
>> 
>> Alignment is funky here.  I'd rather just get rid of all the extra
>> spaces, if that's easier than having half but not all of the = aligned.
>
> Alignment is funky by definition when there are tabs involved and new
> code follows the coding style and uses spaces instead. With a tab stop
> of 4, this is aligned correctly.

Four?  It comes out correctly with tab-width 8 for me.

You can have any tab stops you want as long as it's multiples of eight.

> The other option would be to just convert the whole struct to spaces,
> but that's not really related to this patch and I think we're avoiding
> pure style cleanup patches to keep things like 'git blame' useful.

We normally untabify the lines we touch anyway, and leave the rest
alone.
diff mbox

Patch

diff --git a/block/bochs.c b/block/bochs.c
index af8b7ab..d148454 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -104,6 +104,7 @@  static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;
 
     bs->read_only = 1; // no write support yet
+    bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */
 
     ret = bdrv_pread(bs->file->bs, 0, &bochs, sizeof(bochs));
     if (ret < 0) {
@@ -221,39 +222,50 @@  static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
     return bitmap_offset + (512 * (s->bitmap_blocks + extent_offset));
 }
 
-static int bochs_read(BlockDriverState *bs, int64_t sector_num,
-                    uint8_t *buf, int nb_sectors)
+static int coroutine_fn
+bochs_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
+                QEMUIOVector *qiov, int flags)
 {
+    BDRVBochsState *s = bs->opaque;
+    uint64_t sector_num = offset >> BDRV_SECTOR_BITS;
+    int nb_sectors = bytes >> BDRV_SECTOR_BITS;
+    uint64_t bytes_done = 0;
+    QEMUIOVector local_qiov;
     int ret;
 
+    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+
+    qemu_iovec_init(&local_qiov, qiov->niov);
+    qemu_co_mutex_lock(&s->lock);
+
     while (nb_sectors > 0) {
         int64_t block_offset = seek_to_sector(bs, sector_num);
         if (block_offset < 0) {
             return block_offset;
-        } else if (block_offset > 0) {
-            ret = bdrv_pread(bs->file->bs, block_offset, buf, 512);
+        }
+
+        qemu_iovec_reset(&local_qiov);
+        qemu_iovec_concat(&local_qiov, qiov, bytes_done, 512);
+
+        if (block_offset > 0) {
+            ret = bdrv_co_preadv(bs->file->bs, block_offset, 512,
+                                 &local_qiov, 0);
             if (ret < 0) {
                 return ret;
             }
         } else {
-            memset(buf, 0, 512);
+            qemu_iovec_memset(&local_qiov, 0, 0, 512);
         }
         nb_sectors--;
         sector_num++;
-        buf += 512;
+        bytes_done += 512;
     }
-    return 0;
-}
 
-static coroutine_fn int bochs_co_read(BlockDriverState *bs, int64_t sector_num,
-                                      uint8_t *buf, int nb_sectors)
-{
-    int ret;
-    BDRVBochsState *s = bs->opaque;
-    qemu_co_mutex_lock(&s->lock);
-    ret = bochs_read(bs, sector_num, buf, nb_sectors);
     qemu_co_mutex_unlock(&s->lock);
-    return ret;
+    qemu_iovec_destroy(&local_qiov);
+
+    return 0;
 }
 
 static void bochs_close(BlockDriverState *bs)
@@ -267,7 +279,7 @@  static BlockDriver bdrv_bochs = {
     .instance_size	= sizeof(BDRVBochsState),
     .bdrv_probe		= bochs_probe,
     .bdrv_open		= bochs_open,
-    .bdrv_read          = bochs_co_read,
+    .bdrv_co_preadv = bochs_co_preadv,
     .bdrv_close		= bochs_close,
 };