Re: [Qemu-devel] [PATCH v6 07/20] scsi-disk: Switch to byte-based aio block access

[Kevin Wolf <kwolf@redhat.com>]

Am 05.05.2016 um 01:55 hat Eric Blake geschrieben:
> Sector-based blk_aio_readv() and blk_aio_writev() should die; switch
> to byte-based blk_aio_preadv() and blk_aio_pwritev() instead.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  hw/scsi/scsi-disk.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 1335392..5d98f7b 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -343,8 +343,9 @@ static void scsi_do_read(SCSIDiskReq *r, int ret)
>          n = scsi_init_iovec(r, SCSI_DMA_BUF_SIZE);
>          block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
>                           n * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);

If you replace n * BDRV_SECTOR_SIZE here as well, n is unused and can go
away (you actually did that already for writes).

> -        r->req.aiocb = blk_aio_readv(s->qdev.conf.blk, r->sector, &r->qiov, n,
> -                                     scsi_read_complete, r);
> +        r->req.aiocb = blk_aio_preadv(s->qdev.conf.blk,
> +                                      r->sector << BDRV_SECTOR_BITS, &r->qiov,
> +                                      0, scsi_read_complete, r);
>      }
> 
>  done:
> @@ -504,7 +505,6 @@ static void scsi_write_data(SCSIRequest *req)
>  {
>      SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> -    uint32_t n;
> 
>      /* No data transfer may already be in progress */
>      assert(r->req.aiocb == NULL);
> @@ -544,11 +544,11 @@ static void scsi_write_data(SCSIRequest *req)
>          r->req.aiocb = dma_blk_write(s->qdev.conf.blk, r->req.sg, r->sector,
>                                       scsi_dma_complete, r);
>      } else {
> -        n = r->qiov.size / 512;
>          block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
> -                         n * BDRV_SECTOR_SIZE, BLOCK_ACCT_WRITE);
> -        r->req.aiocb = blk_aio_writev(s->qdev.conf.blk, r->sector, &r->qiov, n,
> -                                      scsi_write_complete, r);
> +                         r->qiov.size, BLOCK_ACCT_WRITE);
> +        r->req.aiocb = blk_aio_pwritev(s->qdev.conf.blk,
> +                                       r->sector << BDRV_SECTOR_BITS, &r->qiov,
> +                                       0, scsi_write_complete, r);
>      }
>  }
> 
> @@ -1730,13 +1730,11 @@ static void scsi_write_same_complete(void *opaque, int ret)
>      if (data->iov.iov_len) {
>          block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
>                           data->iov.iov_len, BLOCK_ACCT_WRITE);
> -        /* blk_aio_write doesn't like the qiov size being different from
> -         * nb_sectors, make sure they match.
> -         */

Wouldn't it be better to update the comment instead of deleting it? If I
understand correctly, this additional qemu_iovec_init_external() is for
the last part of an unaligned WRITE SAME request, where the qiov can
become shorter than in the previous iterations.

>          qemu_iovec_init_external(&data->qiov, &data->iov, 1);
> -        r->req.aiocb = blk_aio_writev(s->qdev.conf.blk, data->sector,
> -                                      &data->qiov, data->iov.iov_len / 512,
> -                                      scsi_write_same_complete, data);
> +        r->req.aiocb = blk_aio_pwritev(s->qdev.conf.blk,
> +                                       data->sector << BDRV_SECTOR_BITS,
> +                                       &data->qiov, 0,
> +                                       scsi_write_same_complete, data);
>          return;
>      }

Kevin