Re: [Qemu-devel] [PATCH 06/17] bochs: Implement .bdrv_co_preadv() interface

From: Stefan Hajnoczi <stefanha@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org, mreitz@redhat.com, famz@redhat.com,
	sw@weilnetz.de, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 06/17] bochs: Implement .bdrv_co_preadv() interface
Date: Wed, 27 Apr 2016 15:06:47 +0100	[thread overview]
Message-ID: <20160427140647.GF1985@stefanha-x1.localdomain> (raw)
In-Reply-To: <1461750767-23273-7-git-send-email-kwolf@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]