| Message ID | 1462406126-22946-8-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
On 05/06/2016 06:50 AM, Kevin Wolf wrote: > 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. >> >> @@ -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). Sure, I can add that in. >> @@ -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. The comment mattered when you were passing two sizes to blk_aio_writev (one embedded in data->qiov, and one in terms of sectors as a direct argument); the block layer asserted the two were equal. But now that we are handling bytes, we pass in a single size (that of data->qiov), so the comment no longer makes sense. > >> 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, It caught me more than once while writing the patch that my new function replaced nb_sectors (the duplicated size) by flags, rather than adding a parameter (as it was harder to get the compiler to gripe about incomplete conversions, such as forgetting to s/write/pwrite/). But by the end of the series, when the old interface is gone, everything still compiles under the new name, so at least we're assured that the series worked on that front.
Am 06.05.2016 um 16:18 hat Eric Blake geschrieben: > On 05/06/2016 06:50 AM, Kevin Wolf wrote: > > Am 05.05.2016 um 01:55 hat Eric Blake geschrieben: > >> @@ -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. > > The comment mattered when you were passing two sizes to blk_aio_writev > (one embedded in data->qiov, and one in terms of sectors as a direct > argument); the block layer asserted the two were equal. But now that we > are handling bytes, we pass in a single size (that of data->qiov), so > the comment no longer makes sense. Yes, with the current text it doesn't make sense any more. But it explains why we have another qemu_iovec_init_external() here, and as that call remains, it would still be good to have an explanation. Or maybe it's obvious, don't know... > > > >> 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, > > It caught me more than once while writing the patch that my new function > replaced nb_sectors (the duplicated size) by flags, rather than adding a > parameter (as it was harder to get the compiler to gripe about > incomplete conversions, such as forgetting to s/write/pwrite/). But by > the end of the series, when the old interface is gone, everything still > compiles under the new name, so at least we're assured that the series > worked on that front. Yes, I was a bit worried that I might miss possible cases where you still pass bytes instead of flags when the compiler can't complain about them. I didn't see any, so hopefully that means it's good. Kevin
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); - 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. - */ 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; } @@ -1803,9 +1801,10 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf) scsi_req_ref(&r->req); block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, data->iov.iov_len, BLOCK_ACCT_WRITE); - 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); } static void scsi_disk_emulate_write_data(SCSIRequest *req)
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(-)