block: byte-based AIO read/write
cover block: byte-based AIO read/write
While we would prefer that block drivers use coroutines instead of aio callbacks, it is a fairly easy exercise to prove that all existing drivers with aio callbacks are merely scaling from bytes into sectors and back to bytes. So, even though I am not set up to completely run (or even compile-test) this full series, it seems pretty straightforward to change the signature to quit playing games with pointless scaling.
Note that except for the null-aio driver, I intentionally did NOT try and change the request_alignment from the block layer's default of 512 (it defaults to 1 only for drivers that use coroutines).
(And along the way, I got my docker-test-mingw@fedora working; thanks to the help I got on IRC)
Eric Blake (6):
block: Support byte-based aio callbacks
file-win32: Switch to byte-based callbacks
null: Switch to byte-based read/write
rbd: Switch to byte-based callbacks
vxhs: Switch to byte-based callbacks
block: Drop last of the sector-based aio callbacks
include/block/block_int.h | 8 +++--- include/block/raw-aio.h | 2 +- block/io.c | 26 ++++++++++++------- block/file-win32.c | 36 +++++++++++++------------- block/null.c | 66 ++++++++++++++++++++++++++--------------------- block/rbd.c | 36 ++++++++++++-------------- block/vxhs.c | 36 +++++++++++--------------- block/win32-aio.c | 5 ++-- 8 files changed, 109 insertions(+), 106 deletions(-)
Signature · 1 line
2.14.3
PATCH 1/6 block: Support byte-based aio callbacks
We are gradually moving away from sector-based interfaces, towards byte-based. Add new sector-based aio callbacks for read and write, to match the fact that bdrv_aio_pdiscard is already byte-based.
Ideally, drivers should be converted to use coroutine callbacks rather than aio; but that is not quite as trivial (if we do that conversion, the null-aio driver will disappear), so for the short term, converting the signature but keeping things with aio is easier. Once all drivers are converted, the sector-based aio callbacks will be removed.
--- include/block/block_int.h | 6 ++++++ block/io.c | 37 +++++++++++++++++++++++++++---------- 2 files changed, 33 insertions(+), 10 deletions(-)
include/block/block_int.h +6 -0
@@ -137,9 +137,15 @@ struct BlockDriver {BlockAIOCB *(*bdrv_aio_readv)(BlockDriverState *bs,int64_t sector_num,QEMUIOVector *qiov,int nb_sectors,BlockCompletionFunc *cb,void *opaque); +BlockAIOCB *(*bdrv_aio_preadv)(BlockDriverState *bs, +uint64_t offset,uint64_t bytes,QEMUIOVector *qiov,int flags, +BlockCompletionFunc *cb,void *opaque);BlockAIOCB *(*bdrv_aio_writev)(BlockDriverState *bs,int64_t sector_num,QEMUIOVector *qiov,int nb_sectors,BlockCompletionFunc *cb,void *opaque); +BlockAIOCB *(*bdrv_aio_pwritev)(BlockDriverState *bs, +uint64_t offset,uint64_t bytes,QEMUIOVector *qiov,int flags, +BlockCompletionFunc *cb,void *opaque); BlockAIOCB *(*bdrv_aio_flush)(BlockDriverState *bs, BlockCompletionFunc *cb, void *opaque); BlockAIOCB *(*bdrv_aio_pdiscard)(BlockDriverState *bs,
block/io.c +27 -10
@@ -934,9 +934,11 @@ static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs, sector_num = offset >>BDRV_SECTOR_BITS ; nb_sectors = bytes >>BDRV_SECTOR_BITS ; -assert ((offset & (BDRV_SECTOR_SIZE -1 )) ==0 ); -assert ((bytes & (BDRV_SECTOR_SIZE -1 )) ==0 ); -assert ((bytes >>BDRV_SECTOR_BITS ) <=BDRV_REQUEST_MAX_SECTORS ); +if (!drv->bdrv_aio_preadv) { +assert ((offset & (BDRV_SECTOR_SIZE -1 )) ==0 ); +assert ((bytes & (BDRV_SECTOR_SIZE -1 )) ==0 ); +assert ((bytes >>BDRV_SECTOR_BITS ) <=BDRV_REQUEST_MAX_SECTORS ); + }if (drv->bdrv_co_readv) {return drv->bdrv_co_readv (bs, sector_num, nb_sectors, qiov);
@@ -946,8 +948,13 @@ static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs, .coroutine = qemu_coroutine_self(), }; - acb = bs->drv->bdrv_aio_readv (bs, sector_num, qiov, nb_sectors, - bdrv_co_io_em_complete, &co); +if (drv->bdrv_aio_preadv) { + acb = bs->drv->bdrv_aio_preadv (bs, offset, bytes, qiov, flags, + bdrv_co_io_em_complete, &co); + }else { + acb = bs->drv->bdrv_aio_readv (bs, sector_num, qiov, nb_sectors, + bdrv_co_io_em_complete, &co); + }if (acb ==NULL ) {return -EIO ; }else {
@@ -982,9 +989,11 @@ static int coroutine_fn bdrv_driver_pwritev(BlockDriverState *bs, sector_num = offset >>BDRV_SECTOR_BITS ; nb_sectors = bytes >>BDRV_SECTOR_BITS ; -assert ((offset & (BDRV_SECTOR_SIZE -1 )) ==0 ); -assert ((bytes & (BDRV_SECTOR_SIZE -1 )) ==0 ); -assert ((bytes >>BDRV_SECTOR_BITS ) <=BDRV_REQUEST_MAX_SECTORS ); +if (!drv->bdrv_aio_pwritev) { +assert ((offset & (BDRV_SECTOR_SIZE -1 )) ==0 ); +assert ((bytes & (BDRV_SECTOR_SIZE -1 )) ==0 ); +assert ((bytes >>BDRV_SECTOR_BITS ) <=BDRV_REQUEST_MAX_SECTORS ); + }if (drv->bdrv_co_writev_flags) { ret = drv->bdrv_co_writev_flags(bs, sector_num, nb_sectors, qiov,
@@ -999,8 +1008,16 @@ static int coroutine_fn bdrv_driver_pwritev(BlockDriverState *bs, .coroutine = qemu_coroutine_self(), }; - acb = bs->drv->bdrv_aio_writev (bs, sector_num, qiov, nb_sectors, - bdrv_co_io_em_complete, &co); +if (drv->bdrv_aio_pwritev) { + acb = bs->drv->bdrv_aio_pwritev (bs, offset, bytes, qiov, + flags & bs->supported_write_flags, + bdrv_co_io_em_complete, &co); + flags &= ~bs->supported_write_flags; + }else { +assert (!bs->supported_write_flags); + acb = bs->drv->bdrv_aio_writev (bs, sector_num, qiov, nb_sectors, + bdrv_co_io_em_complete, &co); + }if (acb ==NULL ) { ret = -EIO ; }else {
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH 1/6] block: Support byte-based aio callbacks
Am 15.02.2018 um 20:28 hat Eric Blake geschrieben:
Quoted 43 lines
We are gradually moving away from sector-based interfaces, towards byte-based. Add new sector-based aio callbacks for read and write, to match the fact that
bdrv_aio_pdiscardis already byte-based.Ideally, drivers should be converted to use coroutine callbacks rather than aio; but that is not quite as trivial (if we do that conversion, the null-aio driver will disappear), so for the short term, converting the signature but keeping things with aio is easier. Once all drivers are converted, the sector-based aio callbacks will be removed.
Signed-off-by Eric Blake <[email protected]>--- include/block/block_int.h | 6 ++++++ block/io.c | 37 +++++++++++++++++++++++++++---------- 2 files changed, 33 insertions(+), 10 deletions(-)include/block/block_int.h +6
-0@@ -137,9 +137,15 @@ struct BlockDriver {BlockAIOCB *(*bdrv_aio_readv)(BlockDriverState *bs,int64_t sector_num,QEMUIOVector *qiov,int nb_sectors,BlockCompletionFunc *cb,void *opaque); +BlockAIOCB *(*bdrv_aio_preadv)(BlockDriverState *bs, +uint64_t offset,uint64_t bytes,QEMUIOVector *qiov,int flags, +BlockCompletionFunc *cb,void *opaque);BlockAIOCB *(*bdrv_aio_writev)(BlockDriverState *bs,int64_t sector_num,QEMUIOVector *qiov,int nb_sectors,BlockCompletionFunc *cb,void *opaque); +BlockAIOCB *(*bdrv_aio_pwritev)(BlockDriverState *bs, +uint64_t offset,uint64_t bytes,QEMUIOVector *qiov,int flags, +BlockCompletionFunc *cb,void *opaque); BlockAIOCB *(*bdrv_aio_flush)(BlockDriverState *bs, BlockCompletionFunc *cb, void *opaque); BlockAIOCB *(*bdrv_aio_pdiscard)(BlockDriverState *bs,block/io.c +0
-0@@ -934,9 +934,11 @@ static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs, sector_num = offset >>BDRV_SECTOR_BITS ;
block/io.c static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs, · line 941
Quoted 10 lines
block/io.c +5
-3nb_sectors = bytes >>BDRV_SECTOR_BITS ; -assert ((offset & (BDRV_SECTOR_SIZE -1 )) ==0 ); -assert ((bytes & (BDRV_SECTOR_SIZE -1 )) ==0 ); -assert ((bytes >>BDRV_SECTOR_BITS ) <=BDRV_REQUEST_MAX_SECTORS ); +if (!drv->bdrv_aio_preadv) { +assert ((offset & (BDRV_SECTOR_SIZE -1 )) ==0 ); +assert ((bytes & (BDRV_SECTOR_SIZE -1 )) ==0 ); +assert ((bytes >>BDRV_SECTOR_BITS ) <=BDRV_REQUEST_MAX_SECTORS ); + }
Hm, this is kind of ugly. Previously, we handled everything byte-aligned in the first section, now we mix both in the second section.
I can see that you do this so you don't have to duplicate the acb and coroutine yielding code below, but can we move things into the right place in the final patch at least? That is, calculate sector_num and nb_sectors only if all the byte-based interfaces weren't available.
Kevin
Re: [Qemu-devel] [PATCH 1/6] block: Support byte-based aio callbacks
Quoted 36 lines · Kevin Wolf
Am 15.02.2018 um 20:28 hat Eric Blake geschrieben:
Quoted 21 lines
We are gradually moving away from sector-based interfaces, towards byte-based. Add new sector-based aio callbacks for read and write, to match the fact that
bdrv_aio_pdiscardis already byte-based.Ideally, drivers should be converted to use coroutine callbacks rather than aio; but that is not quite as trivial (if we do that conversion, the null-aio driver will disappear), so for the short term, converting the signature but keeping things with aio is easier. Once all drivers are converted, the sector-based aio callbacks will be removed.
Signed-off-by Eric Blake <[email protected]>--- include/block/block_int.h | 6 ++++++ block/io.c | 37 +++++++++++++++++++++++++++---------- 2 files changed, 33 insertions(+), 10 deletions(-)+++
b/block/io.c(quoted diff) +0
-0@@ -934,9 +934,11 @@ static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs, sector_num = offset >> BDRV_SECTOR_BITS;Review static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs, · line 941
Quoted 10 lines
(quoted diff) +5
-3nb_sectors = bytes >> BDRV_SECTOR_BITS; - assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); - assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0); - assert((bytes >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS); + if (!drv->bdrv_aio_preadv) { + assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); + assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0); + assert((bytes >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS); + }Hm, this is kind of ugly. Previously, we handled everything byte-aligned in the first section, now we mix both in the second section.
Quoted 4 lines
I can see that you do this so you don't have to duplicate the acb and coroutine yielding code below, but can we move things into the right place in the final patch at least? That is, calculate
sector_numandnb_sectorsonly if all the byte-based interfaces weren't available.
Yeah, that's easy enough to squash into patch 6:
block/io.c +0 -0
@@ -924,16 +924,13 @@ static int coroutine_fn
bdrv_driver_preadv(BlockDriverState *bs,
return drv->bdrv_co_preadv(bs, offset, bytes, qiov, flags);
}- sector_num = offset >> BDRV_SECTOR_BITS;
- nb_sectors = bytes >> BDRV_SECTOR_BITS;
-
- if (!drv->bdrv_aio_preadv) {
- assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
- assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
- assert((bytes >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS);
- }
-
if (drv->bdrv_co_readv) {
+ sector_num = offset >> BDRV_SECTOR_BITS;
+ nb_sectors = bytes >> BDRV_SECTOR_BITS;
+
+ assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
+ assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+ assert((bytes >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS);
return drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
} else {
BlockAIOCB *acb;Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 1/6] block: Support byte-based aio callbacks
Am 24.04.2018 um 19:06 hat Eric Blake geschrieben:
Quoted 71 lines · Kevin Wolf
Quoted 36 lines
Am 15.02.2018 um 20:28 hat Eric Blake geschrieben:
Quoted 21 lines
We are gradually moving away from sector-based interfaces, towards byte-based. Add new sector-based aio callbacks for read and write, to match the fact that
bdrv_aio_pdiscardis already byte-based.Ideally, drivers should be converted to use coroutine callbacks rather than aio; but that is not quite as trivial (if we do that conversion, the null-aio driver will disappear), so for the short term, converting the signature but keeping things with aio is easier. Once all drivers are converted, the sector-based aio callbacks will be removed.
Signed-off-by Eric Blake <[email protected]>--- include/block/block_int.h | 6 ++++++ block/io.c | 37 +++++++++++++++++++++++++++---------- 2 files changed, 33 insertions(+), 10 deletions(-)+++
b/block/io.c(quoted diff) +0
-0@@ -934,9 +934,11 @@ static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs, sector_num = offset >> BDRV_SECTOR_BITS;Review static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs, · line 941
Quoted 10 lines
(quoted diff) +5
-3nb_sectors = bytes >> BDRV_SECTOR_BITS; - assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); - assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0); - assert((bytes >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS); + if (!drv->bdrv_aio_preadv) { + assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); + assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0); + assert((bytes >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS); + }Hm, this is kind of ugly. Previously, we handled everything byte-aligned in the first section, now we mix both in the second section.
Quoted 4 lines
I can see that you do this so you don't have to duplicate the acb and coroutine yielding code below, but can we move things into the right place in the final patch at least? That is, calculate
sector_numandnb_sectorsonly if all the byte-based interfaces weren't available.Yeah, that's easy enough to squash into patch 6:
block/io.c +6
-9@@ -924,16 +924,13 @@ static int coroutine_fnbdrv_driver_preadv (BlockDriverState *bs, return drv->bdrv_co_preadv(bs, offset, bytes, qiov, flags); } - sector_num = offset >>BDRV_SECTOR_BITS ; - nb_sectors = bytes >>BDRV_SECTOR_BITS ; - -if (!drv->bdrv_aio_preadv) { -assert ((offset & (BDRV_SECTOR_SIZE -1 )) ==0 ); -assert ((bytes & (BDRV_SECTOR_SIZE -1 )) ==0 ); -assert ((bytes >>BDRV_SECTOR_BITS ) <=BDRV_REQUEST_MAX_SECTORS ); - } -if (drv->bdrv_co_readv) { + sector_num = offset >>BDRV_SECTOR_BITS ; + nb_sectors = bytes >>BDRV_SECTOR_BITS ; + +assert ((offset & (BDRV_SECTOR_SIZE -1 )) ==0 ); +assert ((bytes & (BDRV_SECTOR_SIZE -1 )) ==0 ); +assert ((bytes >>BDRV_SECTOR_BITS ) <=BDRV_REQUEST_MAX_SECTORS );return drv->bdrv_co_readv (bs, sector_num, nb_sectors, qiov); }else {
Quoted 1 line
BlockAIOCB *acb;
Ah, yes. I thought of moving the code in the else block, but this works, too. Maybe it's even a bit nicer.
Kevin
Re: [Qemu-devel] [PATCH 1/6] block: Support byte-based aio callbacks
Quoted 8 lines · Kevin Wolf
Quoted 8 lines
Quoted 7 lines
Hm, this is kind of ugly. Previously, we handled everything byte-aligned in the first section, now we mix both in the second section.
I can see that you do this so you don't have to duplicate the acb and coroutine yielding code below, but can we move things into the right place in the final patch at least? That is, calculate
sector_numandnb_sectorsonly if all the byte-based interfaces weren't available.
Quoted 6 lines
Quoted 2 lines
Yeah, that's easy enough to squash into patch 6:
Ah, yes. I thought of moving the code in the else block, but this works, too. Maybe it's even a bit nicer.
Moving the code into the 'if' works for bdrv_co_readv, but not so nicely for the bdrv_co_writev vs. bdrv_co_writev_flags. So for v2, I'll just hoist the aio code first; and I guess I smell another round of cleanups coming...
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 2/6 file-win32: Switch to byte-based callbacks
In reply to Eric BlakeWe are gradually moving away from sector-based interfaces, towards byte-based. Make the change for the last few sector-based callbacks in the file-win32 driver.
Note that the driver was already using byte-based calls for performing actual I/O, so this just gets rid of a round trip of scaling; however, the block layer defaults this driver to use request_alignment of 512, and I did not change that, because I don't know if Windows is tolerant of non-sector AIO operations.
---
Compile-tested via 'make docker-test-mingw@fedora', but I don't have a sane way to test whether it actually works.
--- include/block/raw-aio.h | 2 +- block/file-win32.c | 36 ++++++++++++++++++------------------ block/win32-aio.c | 5 ++--- 3 files changed, 21 insertions(+), 22 deletions(-)
include/block/raw-aio.h +1 -1
@@ -57,7 +57,7 @@ void win32_aio_cleanup(QEMUWin32AIOState *aio);int win32_aio_attach (QEMUWin32AIOState *aio,HANDLE hfile);BlockAIOCB *win32_aio_submit (BlockDriverState *bs,QEMUWin32AIOState *aio,HANDLE hfile, -int64_t sector_num,QEMUIOVector *qiov,int nb_sectors, +uint64_t offset,uint64_t bytes,QEMUIOVector *qiov,BlockCompletionFunc *cb,void *opaque,int type);void win32_aio_detach_aio_context (QEMUWin32AIOState *aio,AioContext *old_context);
block/file-win32.c +18 -18
@@ -410,32 +410,32 @@ fail:return ret; } -static BlockAIOCB *raw_aio_readv (BlockDriverState *bs, -int64_t sector_num,QEMUIOVector *qiov,int nb_sectors, -BlockCompletionFunc *cb,void *opaque) +static BlockAIOCB *raw_aio_preadv (BlockDriverState *bs, +uint64_t offset,uint64_t bytes, +QEMUIOVector *qiov,int flags, +BlockCompletionFunc *cb,void *opaque) {BDRVRawState *s = bs->opaque;if (s->aio) { -return win32_aio_submit (bs, s->aio, s->hfile, sector_num, qiov, - nb_sectors, cb, opaque,QEMU_AIO_READ ); +return win32_aio_submit (bs, s->aio, s->hfile, offset, bytes, qiov, + cb, opaque,QEMU_AIO_READ ); }else { -return paio_submit (bs, s->hfile, sector_num <<BDRV_SECTOR_BITS , qiov, - nb_sectors <<BDRV_SECTOR_BITS , +return paio_submit (bs, s->hfile, offset, qiov, bytes, cb, opaque,QEMU_AIO_READ ); } } -static BlockAIOCB *raw_aio_writev (BlockDriverState *bs, -int64_t sector_num,QEMUIOVector *qiov,int nb_sectors, -BlockCompletionFunc *cb,void *opaque) +static BlockAIOCB *raw_aio_pwritev (BlockDriverState *bs, +uint64_t offset,uint64_t bytes, +QEMUIOVector *qiov,int flags, +BlockCompletionFunc *cb,void *opaque) {BDRVRawState *s = bs->opaque;if (s->aio) { -return win32_aio_submit (bs, s->aio, s->hfile, sector_num, qiov, - nb_sectors, cb, opaque,QEMU_AIO_WRITE ); +return win32_aio_submit (bs, s->aio, s->hfile, offset, bytes, qiov, + cb, opaque,QEMU_AIO_WRITE ); }else { -return paio_submit (bs, s->hfile, sector_num <<BDRV_SECTOR_BITS , qiov, - nb_sectors <<BDRV_SECTOR_BITS , +return paio_submit (bs, s->hfile, offset, qiov, bytes, cb, opaque,QEMU_AIO_WRITE ); } }
@@ -602,8 +602,8 @@ BlockDriver bdrv_file = { .bdrv_create = raw_create, .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_aio_readv = raw_aio_readv, - .bdrv_aio_writev = raw_aio_writev, + .bdrv_aio_preadv = raw_aio_preadv, + .bdrv_aio_pwritev = raw_aio_pwritev, .bdrv_aio_flush = raw_aio_flush, .bdrv_truncate = raw_truncate,
@@ -764,8 +764,8 @@ static BlockDriver bdrv_host_device = { .bdrv_file_open = hdev_open, .bdrv_close = raw_close, - .bdrv_aio_readv = raw_aio_readv, - .bdrv_aio_writev = raw_aio_writev, + .bdrv_aio_preadv = raw_aio_preadv, + .bdrv_aio_pwritev = raw_aio_pwritev, .bdrv_aio_flush = raw_aio_flush, .bdrv_detach_aio_context = raw_detach_aio_context,
block/win32-aio.c +2 -3
@@ -112,15 +112,14 @@ static const AIOCBInfo win32_aiocb_info = {BlockAIOCB *win32_aio_submit (BlockDriverState *bs,QEMUWin32AIOState *aio,HANDLE hfile, -int64_t sector_num,QEMUIOVector *qiov,int nb_sectors, +uint64_t offset,uint64_t bytes,QEMUIOVector *qiov,BlockCompletionFunc *cb,void *opaque,int type) {struct QEMUWin32AIOCB *waiocb; -uint64_t offset = sector_num *512 ;DWORD rc; waiocb =qemu_aio_get (&win32_aiocb_info, bs, cb, opaque); - waiocb->nbytes = nb_sectors *512 ; + waiocb->nbytes = bytes; waiocb->qiov = qiov; waiocb->is_read = (type ==QEMU_AIO_READ );
Signature · 1 line
2.14.3
PATCH 3/6 null: Switch to byte-based read/write
In reply to Eric BlakeWe are gradually moving away from sector-based interfaces, towards byte-based. Make the change for the last few sector-based callbacks in the null-co and null-aio drivers.
Note that since the null driver does nothing on writes, it trivially supports the BDRV_REQ_FUA flag (all writes have already landed to the same bit-bucket without needing an extra flush call). Furthermore, bdrv_refresh_limits() defaults the block size to 512 for any driver that does not support coroutines; while this is still correct for the other aio-based drivers, the null driver does just as well with byte-based requests, and being explicit means we can avoid cycles wasted on read-modify-write.
--- block/null.c | 66 ++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 37 insertions(+), 29 deletions(-)
block/null.c +37 -29
@@ -93,6 +93,7 @@ static int null_file_open(BlockDriverState *bs, QDict *options, int flags, } s->read_zeroes =qemu_opt_get_bool (opts,NULL_OPT_ZEROES , false);qemu_opts_del (opts); + bs->supported_write_flags =BDRV_REQ_FUA ;return ret; }
@@ -116,22 +117,22 @@ static coroutine_fn int null_co_common(BlockDriverState *bs)return 0 ; } -static coroutine_fn int null_co_readv(BlockDriverState *bs, -int64_t sector_num,int nb_sectors, -QEMUIOVector *qiov) +static coroutine_fn int null_co_preadv(BlockDriverState *bs, +uint64_t offset,uint64_t bytes, +QEMUIOVector *qiov,int flags) {BDRVNullState *s = bs->opaque;if (s->read_zeroes) { -qemu_iovec_memset (qiov,0 ,0 , nb_sectors *BDRV_SECTOR_SIZE ); +qemu_iovec_memset (qiov,0 ,0 , bytes); }return null_co_common (bs); } -static coroutine_fn int null_co_writev(BlockDriverState *bs, -int64_t sector_num,int nb_sectors, -QEMUIOVector *qiov) +static coroutine_fn int null_co_pwritev(BlockDriverState *bs, +uint64_t offset,uint64_t bytes, +QEMUIOVector *qiov,int flags) {return null_co_common (bs); }
@@ -186,26 +187,26 @@ static inline BlockAIOCB *null_aio_common(BlockDriverState *bs,return &acb->common; } -static BlockAIOCB *null_aio_readv (BlockDriverState *bs, -int64_t sector_num,QEMUIOVector *qiov, -int nb_sectors, -BlockCompletionFunc *cb, -void *opaque) -{ -BDRVNullState *s = bs->opaque; - -if (s->read_zeroes) { -qemu_iovec_memset (qiov,0 ,0 , nb_sectors *BDRV_SECTOR_SIZE ); - } - -return null_aio_common (bs, cb, opaque); -} - -static BlockAIOCB *null_aio_writev (BlockDriverState *bs, -int64_t sector_num,QEMUIOVector *qiov, -int nb_sectors, +static BlockAIOCB *null_aio_preadv (BlockDriverState *bs, +uint64_t offset,uint64_t bytes, +QEMUIOVector *qiov,int flags,BlockCompletionFunc *cb,void *opaque) +{ +BDRVNullState *s = bs->opaque; + +if (s->read_zeroes) { +qemu_iovec_memset (qiov,0 ,0 , bytes); + } + +return null_aio_common (bs, cb, opaque); +} + +static BlockAIOCB *null_aio_pwritev (BlockDriverState *bs, +uint64_t offset,uint64_t bytes, +QEMUIOVector *qiov,int flags, +BlockCompletionFunc *cb, +void *opaque) {return null_aio_common (bs, cb, opaque); }
@@ -256,6 +257,11 @@ static void null_refresh_filename(BlockDriverState *bs, QDict *opts) bs->full_open_options = opts; } +static void null_refresh_limits (BlockDriverState *bs,Error **errp) +{ + bs->bl.request_alignment =1 ; +} +static BlockDriver bdrv_null_co = { .format_name ="null-co" , .protocol_name ="null-co" ,
@@ -266,14 +272,15 @@ static BlockDriver bdrv_null_co = { .bdrv_close = null_close, .bdrv_getlength = null_getlength, - .bdrv_co_readv = null_co_readv, - .bdrv_co_writev = null_co_writev, + .bdrv_co_preadv = null_co_preadv, + .bdrv_co_pwritev = null_co_pwritev, .bdrv_co_flush_to_disk = null_co_flush, .bdrv_reopen_prepare = null_reopen_prepare, .bdrv_co_block_status = null_co_block_status, .bdrv_refresh_filename = null_refresh_filename, + .bdrv_refresh_limits = null_refresh_limits, };static BlockDriver bdrv_null_aio = {
@@ -286,14 +293,15 @@ static BlockDriver bdrv_null_aio = { .bdrv_close = null_close, .bdrv_getlength = null_getlength, - .bdrv_aio_readv = null_aio_readv, - .bdrv_aio_writev = null_aio_writev, + .bdrv_aio_preadv = null_aio_preadv, + .bdrv_aio_pwritev = null_aio_pwritev, .bdrv_aio_flush = null_aio_flush, .bdrv_reopen_prepare = null_reopen_prepare, .bdrv_co_block_status = null_co_block_status, .bdrv_refresh_filename = null_refresh_filename, + .bdrv_refresh_limits = null_refresh_limits, };static void bdrv_null_init (void )
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH 3/6] null: Switch to byte-based read/write
Am 15.02.2018 um 20:28 hat Eric Blake geschrieben:
Quoted 15 lines
We are gradually moving away from sector-based interfaces, towards byte-based. Make the change for the last few sector-based callbacks in the null-co and null-aio drivers.
Note that since the null driver does nothing on writes, it trivially supports the
BDRV_REQ_FUAflag (all writes have already landed to the same bit-bucket without needing an extra flush call). Furthermore,bdrv_refresh_limits()defaults the block size to 512 for any driver that does not support coroutines; while this is still correct for the other aio-based drivers, the null driver does just as well with byte-based requests, and being explicit means we can avoid cycles wasted on read-modify-write.Signed-off-by Eric Blake <[email protected]>
block/null.c static void null_refresh_filename(BlockDriverState *bs, QDict *opts) · line 263
Quoted 4 lines
block/null.c +4
-0+static void null_refresh_limits (BlockDriverState *bs,Error **errp) +{ + bs->bl.request_alignment =1 ; +}
I would rather modify bdrv_refresh_limits() so that it defaults to 1 for drivers supporting either .bdrv_co_preadv or .bdrv_aio_preadv.
Kevin
Re: [Qemu-devel] [PATCH 3/6] null: Switch to byte-based read/write
Quoted 16 lines · Kevin Wolf
Am 15.02.2018 um 20:28 hat Eric Blake geschrieben:
Quoted 14 lines
We are gradually moving away from sector-based interfaces, towards byte-based. Make the change for the last few sector-based callbacks in the null-co and null-aio drivers.
Note that since the null driver does nothing on writes, it trivially supports the
BDRV_REQ_FUAflag (all writes have already landed to the same bit-bucket without needing an extra flush call). Furthermore,bdrv_refresh_limits()defaults the block size to 512 for any driver that does not support coroutines; while this is still correct for the other aio-based drivers, the null driver does just as well with byte-based requests, and being explicit means we can avoid cycles wasted on read-modify-write.Signed-off-by Eric Blake <[email protected]>
Quoted 7 lines
block/null.c static void null_refresh_filename(BlockDriverState *bs, QDict *opts) · line 263
Quoted 4 lines
block/null.c +4
-0+static void null_refresh_limits (BlockDriverState *bs,Error **errp) +{ + bs->bl.request_alignment =1 ; +}I would rather modify
bdrv_refresh_limits()so that it defaults to 1 for drivers supporting either .bdrv_co_preadv or .bdrv_aio_preadv.
Sure, I can do that (although then I may have to provide a refresh_limits callback for each of the other drivers that I specifically left at 512 alignment).
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 3/6] null: Switch to byte-based read/write
Am 24.04.2018 um 19:00 hat Eric Blake geschrieben:
Quoted 25 lines · Kevin Wolf
Quoted 16 lines
Am 15.02.2018 um 20:28 hat Eric Blake geschrieben:
Quoted 14 lines
We are gradually moving away from sector-based interfaces, towards byte-based. Make the change for the last few sector-based callbacks in the null-co and null-aio drivers.
Note that since the null driver does nothing on writes, it trivially supports the
BDRV_REQ_FUAflag (all writes have already landed to the same bit-bucket without needing an extra flush call). Furthermore,bdrv_refresh_limits()defaults the block size to 512 for any driver that does not support coroutines; while this is still correct for the other aio-based drivers, the null driver does just as well with byte-based requests, and being explicit means we can avoid cycles wasted on read-modify-write.Signed-off-by Eric Blake <[email protected]>Quoted 7 lines
block/null.c static void null_refresh_filename(BlockDriverState *bs, QDict *opts) · line 263
Quoted 4 lines
block/null.c +4
-0+static void null_refresh_limits (BlockDriverState *bs,Error **errp) +{ + bs->bl.request_alignment =1 ; +}I would rather modify
bdrv_refresh_limits()so that it defaults to 1 for drivers supporting either .bdrv_co_preadv or .bdrv_aio_preadv.
Quoted 3 lines
Sure, I can do that (although then I may have to provide a
refresh_limitscallback for each of the other drivers that I specifically left at 512 alignment).
Do we know that the other drivers need 512, or do we expect than they can handle byte granularity, but we try to err on the safe side?
If it's the latter (which would be my expectation), then it's probably better to set request_alignment = 512 with a comment in those drivers and hope that someone will lift the restriction in the future.
Kevin
Re: [Qemu-devel] [PATCH 3/6] null: Switch to byte-based read/write
Quoted 8 lines · Kevin Wolf
Quoted 8 lines
Quoted 7 lines
block/null.c static void null_refresh_filename(BlockDriverState *bs, QDict *opts) · line 263
Quoted 4 lines
block/null.c +4
-0+static void null_refresh_limits (BlockDriverState *bs,Error **errp) +{ + bs->bl.request_alignment =1 ; +}I would rather modify
bdrv_refresh_limits()so that it defaults to 1 for drivers supporting either .bdrv_co_preadv or .bdrv_aio_preadv.
Quoted 6 lines
Quoted 3 lines
Sure, I can do that (although then I may have to provide a
refresh_limitscallback for each of the other drivers that I specifically left at 512 alignment).Do we know that the other drivers need 512, or do we expect than they can handle byte granularity, but we try to err on the safe side?
Done to err on the safe side and avoid the difficulty of a deep audit in code I'm unfamiliar with; in fact, for vxhs, I wasn't even able to compile test things, which means I certainly wasn't able to reproduce a test to see if dropping the alignment would break anything under qemu-io.
Quoted 4 lines
If it's the latter (which would be my expectation), then it's probably better to set
request_alignment= 512 with a comment in those drivers and hope that someone will lift the restriction in the future.
Indeed, leaving a good comment for a future reader is worth the effort. I'll send a v2 later today.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 4/6 rbd: Switch to byte-based callbacks
In reply to Eric BlakeWe are gradually moving away from sector-based interfaces, towards byte-based. Make the change for the last few sector-based callbacks in the rbd driver.
Note that the driver was already using byte-based calls for performing actual I/O, so this just gets rid of a round trip of scaling; however, the block layer defaults this driver to use request_alignment of 512, and I did not change that.
--- block/rbd.c | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-)
block/rbd.c +16 -20
@@ -870,27 +870,23 @@ failed:return NULL ; } -static BlockAIOCB *qemu_rbd_aio_readv (BlockDriverState *bs, -int64_t sector_num, -QEMUIOVector *qiov, -int nb_sectors, -BlockCompletionFunc *cb, -void *opaque) -{ -return rbd_start_aio (bs, sector_num <<BDRV_SECTOR_BITS , qiov, - (int64_t ) nb_sectors <<BDRV_SECTOR_BITS , cb, opaque, -RBD_AIO_READ ); -} - -static BlockAIOCB *qemu_rbd_aio_writev (BlockDriverState *bs, -int64_t sector_num, -QEMUIOVector *qiov, -int nb_sectors, +static BlockAIOCB *qemu_rbd_aio_preadv (BlockDriverState *bs, +uint64_t offset,uint64_t bytes, +QEMUIOVector *qiov,int flags,BlockCompletionFunc *cb,void *opaque) { -return rbd_start_aio (bs, sector_num <<BDRV_SECTOR_BITS , qiov, - (int64_t ) nb_sectors <<BDRV_SECTOR_BITS , cb, opaque, +return rbd_start_aio (bs, offset, qiov, bytes, cb, opaque, +RBD_AIO_READ ); +} + +static BlockAIOCB *qemu_rbd_aio_pwritev (BlockDriverState *bs, +uint64_t offset,uint64_t bytes, +QEMUIOVector *qiov,int flags, +BlockCompletionFunc *cb, +void *opaque) +{ +return rbd_start_aio (bs, offset, qiov, bytes, cb, opaque,RBD_AIO_WRITE ); }
@@ -1140,8 +1136,8 @@ static BlockDriver bdrv_rbd = { .bdrv_truncate = qemu_rbd_truncate, .protocol_name = "rbd", - .bdrv_aio_readv = qemu_rbd_aio_readv, - .bdrv_aio_writev = qemu_rbd_aio_writev, + .bdrv_aio_preadv = qemu_rbd_aio_preadv, + .bdrv_aio_pwritev = qemu_rbd_aio_pwritev, #ifdef LIBRBD_SUPPORTS_AIO_FLUSH .bdrv_aio_flush = qemu_rbd_aio_flush,
Signature · 1 line
2.14.3
PATCH 5/6 vxhs: Switch to byte-based callbacks
In reply to Eric BlakeWe are gradually moving away from sector-based interfaces, towards byte-based. Make the change for the last few sector-based callbacks in the vxhs driver.
Note that the driver was already using byte-based calls for performing actual I/O, so this just gets rid of a round trip of scaling; however, the block layer defaults this driver to use request_alignment of 512, and I did not change that.
---
I was unable to get this to compile, as I do not have the vxhs devel headers installed. By inspection, it should work, but a careful review is appreciated.
--- block/vxhs.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-)
block/vxhs.c +15 -21
@@ -424,21 +424,17 @@ static const AIOCBInfo vxhs_aiocb_info = {* and is passed to QNIO. When QNIO completes the work, * it will be passed back through the callback. */ -static BlockAIOCB *vxhs_aio_rw (BlockDriverState *bs,int64_t sector_num, -QEMUIOVector *qiov,int nb_sectors, +static BlockAIOCB *vxhs_aio_rw (BlockDriverState *bs,uint64_t offset, +QEMUIOVector *qiov,uint64_t size,BlockCompletionFunc *cb,void *opaque,VDISKAIOCmd iodir) {VXHSAIOCB *acb =NULL ;BDRVVXHSState *s = bs->opaque; -size_t size; -uint64_t offset;int iio_flags =0 ;int ret =0 ;void *dev_handle = s->vdisk_hostinfo.dev_handle; - offset = sector_num *BDRV_SECTOR_SIZE ; - size = nb_sectors *BDRV_SECTOR_SIZE ; acb =qemu_aio_get (&vxhs_aiocb_info, bs, cb, opaque);/*
@@ -451,11 +447,11 @@ static BlockAIOCB *vxhs_aio_rw(BlockDriverState *bs, int64_t sector_num,switch (iodir) {case VDISK_AIO_WRITE : ret =iio_writev (dev_handle, acb, qiov->iov, qiov->niov, - offset, (uint64_t )size, iio_flags); + offset, size, iio_flags);break ;case VDISK_AIO_READ : ret =iio_readv (dev_handle, acb, qiov->iov, qiov->niov, - offset, (uint64_t )size, iio_flags); + offset, size, iio_flags);break ;default :trace_vxhs_aio_rw_invalid (iodir);
@@ -474,22 +470,20 @@ errout:return NULL ; } -static BlockAIOCB *vxhs_aio_readv (BlockDriverState *bs, -int64_t sector_num,QEMUIOVector *qiov, -int nb_sectors, +static BlockAIOCB *vxhs_aio_preadv (BlockDriverState *bs, +uint64_t offset,uint64_t bytes, +QEMUIOVector *qiov,int flags,BlockCompletionFunc *cb,void *opaque) { -return vxhs_aio_rw (bs, sector_num, qiov, nb_sectors, cb, - opaque,VDISK_AIO_READ ); +return vxhs_aio_rw (bs, offset, qiov, bytes, cb, opaque,VDISK_AIO_READ ); } -static BlockAIOCB *vxhs_aio_writev (BlockDriverState *bs, -int64_t sector_num,QEMUIOVector *qiov, -int nb_sectors, -BlockCompletionFunc *cb,void *opaque) +static BlockAIOCB *vxhs_aio_pwritev (BlockDriverState *bs, +uint64_t offset,uint64_t bytes, +QEMUIOVector *qiov,int flags, +BlockCompletionFunc *cb,void *opaque) { -return vxhs_aio_rw (bs, sector_num, qiov, nb_sectors, - cb, opaque,VDISK_AIO_WRITE ); +return vxhs_aio_rw (bs, offset, qiov, bytes, cb, opaque,VDISK_AIO_WRITE ); }static void vxhs_close (BlockDriverState *bs)
@@ -563,8 +557,8 @@ static BlockDriver bdrv_vxhs = { .bdrv_parse_filename = vxhs_parse_filename, .bdrv_close = vxhs_close, .bdrv_getlength = vxhs_getlength, - .bdrv_aio_readv = vxhs_aio_readv, - .bdrv_aio_writev = vxhs_aio_writev, + .bdrv_aio_preadv = vxhs_aio_preadv, + .bdrv_aio_pwritev = vxhs_aio_pwritev, };static void bdrv_vxhs_init (void )
Signature · 1 line
2.14.3
PATCH 6/6 block: Drop last of the sector-based aio callbacks
In reply to Eric BlakeWe are gradually moving away from sector-based interfaces, towards byte-based. Now that all drivers with aio callbacks are using the byte-based interfaces, we can remove the sector-based versions.
--- include/block/block_int.h | 6 ------ block/io.c | 23 ++++++----------------- 2 files changed, 6 insertions(+), 23 deletions(-)
include/block/block_int.h +0 -6
@@ -134,15 +134,9 @@ struct BlockDriver {void (*bdrv_refresh_filename)(BlockDriverState *bs,QDict *options);/* aio */ -BlockAIOCB *(*bdrv_aio_readv)(BlockDriverState *bs, -int64_t sector_num,QEMUIOVector *qiov,int nb_sectors, -BlockCompletionFunc *cb,void *opaque);BlockAIOCB *(*bdrv_aio_preadv)(BlockDriverState *bs,uint64_t offset,uint64_t bytes,QEMUIOVector *qiov,int flags,BlockCompletionFunc *cb,void *opaque); -BlockAIOCB *(*bdrv_aio_writev)(BlockDriverState *bs, -int64_t sector_num,QEMUIOVector *qiov,int nb_sectors, -BlockCompletionFunc *cb,void *opaque);BlockAIOCB *(*bdrv_aio_pwritev)(BlockDriverState *bs,uint64_t offset,uint64_t bytes,QEMUIOVector *qiov,int flags,BlockCompletionFunc *cb,void *opaque);
block/io.c +6 -17
@@ -948,13 +948,8 @@ static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs, .coroutine = qemu_coroutine_self(), }; -if (drv->bdrv_aio_preadv) { - acb = bs->drv->bdrv_aio_preadv (bs, offset, bytes, qiov, flags, - bdrv_co_io_em_complete, &co); - }else { - acb = bs->drv->bdrv_aio_readv (bs, sector_num, qiov, nb_sectors, - bdrv_co_io_em_complete, &co); - } + acb = bs->drv->bdrv_aio_preadv (bs, offset, bytes, qiov, flags, + bdrv_co_io_em_complete, &co);if (acb ==NULL ) {return -EIO ; }else {
@@ -1008,16 +1003,10 @@ static int coroutine_fn bdrv_driver_pwritev(BlockDriverState *bs, .coroutine = qemu_coroutine_self(), }; -if (drv->bdrv_aio_pwritev) { - acb = bs->drv->bdrv_aio_pwritev (bs, offset, bytes, qiov, - flags & bs->supported_write_flags, - bdrv_co_io_em_complete, &co); - flags &= ~bs->supported_write_flags; - }else { -assert (!bs->supported_write_flags); - acb = bs->drv->bdrv_aio_writev (bs, sector_num, qiov, nb_sectors, - bdrv_co_io_em_complete, &co); - } + acb = bs->drv->bdrv_aio_pwritev (bs, offset, bytes, qiov, + flags & bs->supported_write_flags, + bdrv_co_io_em_complete, &co); + flags &= ~bs->supported_write_flags;if (acb ==NULL ) { ret = -EIO ; }else {
Signature · 1 line
2.14.3
Quoted 8 lines · Eric Blake
While we would prefer that block drivers use coroutines instead of aio callbacks, it is a fairly easy exercise to prove that all existing drivers with aio callbacks are merely scaling from bytes into sectors and back to bytes. So, even though I am not set up to completely run (or even compile-test) this full series, it seems pretty straightforward to change the signature to quit playing games with pointless scaling.
Quoted 7 lines
Note that except for the null-aio driver, I intentionally did NOT try and change the
request_alignmentfrom the block layer's default of 512 (it defaults to 1 only for drivers that use coroutines).(And along the way, I got my docker-test-mingw@fedora working; thanks to the help I got on IRC)
ping
Quoted 19 lines
Eric Blake (6):
block: Support byte-based aio callbacks
file-win32: Switch to byte-based callbacks
null: Switch to byte-based read/write
rbd: Switch to byte-based callbacks
vxhs: Switch to byte-based callbacks
block: Drop last of the sector-based aio callbacksinclude/block/block_int.h | 8 +++--- include/block/raw-aio.h | 2 +- block/io.c | 26 ++++++++++++------- block/file-win32.c | 36 +++++++++++++------------- block/null.c | 66 ++++++++++++++++++++++++++--------------------- block/rbd.c | 36 ++++++++++++-------------- block/vxhs.c | 36 +++++++++++--------------- block/win32-aio.c | 5 ++-- 8 files changed, 109 insertions(+), 106 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 14 lines · Eric Blake
Quoted 13 lines · Eric Blake
While we would prefer that block drivers use coroutines instead of aio callbacks, it is a fairly easy exercise to prove that all existing drivers with aio callbacks are merely scaling from bytes into sectors and back to bytes. So, even though I am not set up to completely run (or even compile-test) this full series, it seems pretty straightforward to change the signature to quit playing games with pointless scaling.
Note that except for the null-aio driver, I intentionally did NOT try and change the
request_alignmentfrom the block layer's default of 512 (it defaults to 1 only for drivers that use coroutines).
Quoted 5 lines
Quoted 2 lines
(And along the way, I got my docker-test-mingw@fedora working; thanks to the help I got on IRC)
ping
One of your replies to patch 3 suggests you were going to send a v2 -- is that not the case? Do you want deeper review of v1 first?
--js