Re: [Qemu-block] [PATCH v7 06/19] scsi-disk: Switch to byte-based aio block access

[Kevin Wolf]

Am 06.05.2016 um 18:26 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.
> 
> As part of the cleanup, scsi_init_iovec() no longer needs to return
> a value, and reword a comment.
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v7: remove more unused 'n', fix comment wording
> ---
>  hw/scsi/scsi-disk.c | 41 ++++++++++++++++++++---------------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 1335392..d6bb9ad 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -108,7 +108,7 @@ static void scsi_check_condition(SCSIDiskReq *r, 
> SCSISense sense)
>      scsi_req_complete(&r->req, CHECK_CONDITION);
>  }
> 
> -static uint32_t scsi_init_iovec(SCSIDiskReq *r, size_t size)
> +static void scsi_init_iovec(SCSIDiskReq *r, size_t size)
>  {
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> 
> @@ -118,7 +118,6 @@ static uint32_t scsi_init_iovec(SCSIDiskReq *r, size_t 
> size)
>      }
>      r->iov.iov_len = MIN(r->sector_count * 512, r->buflen);
>      qemu_iovec_init_external(&r->qiov, &r->iov, 1);
> -    return r->qiov.size / 512;

The return value was MIN(r->sector_count, SCSI_DMA_BUF_SIZE / 512).

>  }
> 
>  static void scsi_disk_save_request(QEMUFile *f, SCSIRequest *req)
> @@ -316,7 +315,6 @@ done:
>  static void scsi_do_read(SCSIDiskReq *r, int ret)
>  {
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> -    uint32_t n;
> 
>      assert (r->req.aiocb == NULL);
> 
> @@ -340,11 +338,12 @@ static void scsi_do_read(SCSIDiskReq *r, int ret)
>          r->req.aiocb = dma_blk_read(s->qdev.conf.blk, r->req.sg, r->sector,
>                                      scsi_dma_complete, r);
>      } else {
> -        n = scsi_init_iovec(r, SCSI_DMA_BUF_SIZE);
> +        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);
> -        r->req.aiocb = blk_aio_readv(s->qdev.conf.blk, r->sector, &r->qiov, 
> n,
> -                                     scsi_read_complete, r);
> +                         SCSI_DMA_BUF_SIZE, BLOCK_ACCT_READ);

But here you use SCSI_DMA_BUF_SIZE without considering r->sector_count.
Is this correct or are requests that are smaller than SCSI_DMA_BUF_SIZE
now accounted for more data than they actually read?

Would r->qiov.size be more obviously correct? You did that for writes.

> +        r->req.aiocb = blk_aio_preadv(s->qdev.conf.blk,
> +                                      r->sector << BDRV_SECTOR_BITS, 
> &r->qiov,
> +                                      0, scsi_read_complete, r);
>      }
> 
>  done:

Kevin