block: byte-based blocking read/write
cover block: byte-based blocking read/write
A logical followup to my other recent cleanups. The only remaining mention of sectors in block.h APIs is in bdrv_nb_sectors() (converting those callers to bdrv_getlength() not only requires more work, but would be our opportunity to see if we could quit rounding block sizes to sector boundaries for byte-based protocols that can actually report an exact unaligned size) and for computing geometries (where it actually makes sense).
Based-on: <[email protected]> ([PATCH v2 0/6] block: byte-based AIO read/write)
Based-on: <[email protected]> ([PATCH v6 0/6] minor qcow2 compression improvements)
Eric Blake (3):
vdi: Switch to byte-based calls
vvfat: Switch to byte-based calls
block: Removed unused sector-based blocking I/O
include/block/block.h | 4 ---- block/io.c | 48 ++++++++---------------------------------------- block/vdi.c | 14 +++++++------- block/vvfat.c | 20 +++++++++++--------- 4 files changed, 26 insertions(+), 60 deletions(-)
Signature · 1 line
2.14.3
PATCH 1/3 vdi: Switch to byte-based calls
We are gradually moving away from sector-based interfaces, towards byte-based. Make the change for the last few sector-based calls into the block layer from the vdi driver.
Ideally, the vdi driver should switch to doing everything byte-based, but that's a more invasive change that requires a bit more auditing.
--- block/vdi.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
block/vdi.c +7 -7
@@ -379,7 +379,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,logout ("\n" ); - ret =bdrv_read (bs->file,0 , (uint8_t *)&header,1 ); + ret =bdrv_pread (bs->file,0 , &header,sizeof (header));if (ret <0 ) { goto fail; }
@@ -468,15 +468,14 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, s->header = header; bmap_size = header.blocks_in_image *sizeof (uint32_t ); - bmap_size =DIV_ROUND_UP (bmap_size,SECTOR_SIZE ); - s->bmap =qemu_try_blockalign (bs->file->bs, bmap_size *SECTOR_SIZE ); + s->bmap =qemu_try_blockalign (bs->file->bs, bmap_size);if (s->bmap ==NULL ) { ret = -ENOMEM ; goto fail; } - ret =bdrv_read (bs->file, s->bmap_sector, (uint8_t *)s->bmap, - bmap_size); + ret =bdrv_pread (bs->file, s->bmap_sector *SECTOR_SIZE , s->bmap, + bmap_size);if (ret <0 ) { goto fail_free_bmap; }
@@ -695,7 +694,7 @@ nonallocating_write:assert (VDI_IS_ALLOCATED (bmap_first)); *header = s->header;vdi_header_to_le (header); - ret =bdrv_write (bs->file,0 , block,1 ); + ret =bdrv_pwrite (bs->file,0 , block,SECTOR_SIZE );g_free (block); block =NULL ;
@@ -713,7 +712,8 @@ nonallocating_write: base = ((uint8_t *)&s->bmap[0 ]) + bmap_first *SECTOR_SIZE ;logout ("will write %u block map sectors starting from entry %u\n" , n_sectors, bmap_first); - ret =bdrv_write (bs->file, offset, base, n_sectors); + ret =bdrv_pwrite (bs->file, offset *SECTOR_SIZE , base, + n_sectors *SECTOR_SIZE ); }return ret;
Signature · 1 line
2.14.3
Re: [Qemu-devel] [Qemu-block] [PATCH 1/3] vdi: Switch to byte-based calls
Quoted 9 lines · Eric Blake
We are gradually moving away from sector-based interfaces, towards byte-based. Make the change for the last few sector-based calls into the block layer from the vdi driver.
Ideally, the vdi driver should switch to doing everything byte-based, but that's a more invasive change that requires a bit more auditing.
Signed-off-by Eric Blake <[email protected]>
Berto
PATCH 2/3 vvfat: Switch to byte-based calls
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 calls into the block layer from the vvfat driver.
Ideally, the vvfat driver should switch to doing everything byte-based, but that's a more invasive change that requires a bit more auditing.
--- block/vvfat.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
block/vvfat.c +11 -9
@@ -1503,8 +1503,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,DLOG (fprintf (stderr,"sectors %" PRId64"+%" PRId64" allocated\n" , sector_num, n >>BDRV_SECTOR_BITS )); -if (bdrv_read (s->qcow, sector_num, buf + i *0x200 , - n >>BDRV_SECTOR_BITS )) { +if (bdrv_pread (s->qcow, sector_num *BDRV_SECTOR_BITS , + buf + i *0x200 , n) <0 ) {return -1 ; } i += (n >>BDRV_SECTOR_BITS ) -1 ;
@@ -1992,7 +1992,8 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,if (res) {return -1 ; } - res =bdrv_write (s->qcow, offset, s->cluster_buffer,1 ); + res =bdrv_pwrite (s->qcow, offset *BDRV_SECTOR_SIZE , + s->cluster_buffer,BDRV_SECTOR_SIZE );if (res) {return -2 ; }
@@ -2155,7 +2156,7 @@ DLOG(checkpoint());* - get modified FAT * - compare the two FATs (TODO) * - get buffer for marking used clusters -* - recurse direntries from root (using bs->bdrv_read to make +* - recurse direntries from root (using bs->bdrv_pread to make * sure to get the new data) * - check that the FAT agrees with the size * - count the number of clusters occupied by this directory and
@@ -2902,9 +2903,9 @@ static int handle_deletes(BDRVVVFATState* s)/* * synchronize mapping with new state: * -* - copy FAT (with bdrv_read) +* - copy FAT (with bdrv_pread) * - mark all filenames corresponding to mappings as deleted -* - recurse direntries from root (using bs->bdrv_read) +* - recurse direntries from root (using bs->bdrv_pread) * - delete files corresponding to mappings marked as deleted */ static int do_commit (BDRVVVFATState * s)
@@ -2924,10 +2925,10 @@ static int do_commit(BDRVVVFATState* s)return ret; } -/* copy FAT (with bdrv_read) */ +/* copy FAT (with bdrv_pread) */ memcpy (s->fat.pointer, s->fat2,0x200 * s->sectors_per_fat); -/* recurse direntries from root (using bs->bdrv_read) */ +/* recurse direntries from root (using bs->bdrv_pread) */ ret =commit_direntries (s,0 ,-1 );if (ret) {fprintf (stderr,"Fatal: error while committing (%d)\n" , ret);
@@ -3041,7 +3042,8 @@ DLOG(checkpoint());* Use qcow backend. Commit later. */ DLOG (fprintf (stderr,"Write to qcow backend: %d + %d\n" , (int )sector_num, nb_sectors)); - ret =bdrv_write (s->qcow, sector_num, buf, nb_sectors); + ret =bdrv_pwrite (s->qcow, sector_num *BDRV_SECTOR_SIZE , buf, + nb_sectors *BDRV_SECTOR_SIZE );if (ret <0 ) {fprintf (stderr,"Error writing to qcow backend\n" );return ret;
Signature · 1 line
2.14.3
Re: [Qemu-devel] [Qemu-block] [PATCH 2/3] vvfat: Switch to byte-based calls
Quoted 9 lines · Eric Blake
We are gradually moving away from sector-based interfaces, towards byte-based. Make the change for the last few sector-based calls into the block layer from the vvfat driver.
Ideally, the vvfat driver should switch to doing everything byte-based, but that's a more invasive change that requires a bit more auditing.
Signed-off-by Eric Blake <[email protected]>
Berto
PATCH 3/3 block: Removed unused sector-based blocking I/O
In reply to Eric BlakeWe are gradually moving away from sector-based interfaces, towards byte-based. Now that all callers of blocking I/O have been converted to use our preferred byte-based bdrv_p{read,write}(), we can delete the unused bdrv_{read,write}().
--- include/block/block.h | 4 ---- block/io.c | 48 ++++++++---------------------------------------- 2 files changed, 8 insertions(+), 44 deletions(-)
include/block/block.h +0 -4
@@ -265,10 +265,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, Error **errp);void bdrv_reopen_commit (BDRVReopenState *reopen_state);void bdrv_reopen_abort (BDRVReopenState *reopen_state); -int bdrv_read (BdrvChild *child,int64_t sector_num, -uint8_t *buf,int nb_sectors); -int bdrv_write (BdrvChild *child,int64_t sector_num, -const uint8_t *buf,int nb_sectors);int bdrv_pwrite_zeroes (BdrvChild *child,int64_t offset,int bytes,BdrvRequestFlags flags);int bdrv_make_zero (BdrvChild *child,BdrvRequestFlags flags);
block/io.c +8 -40
72 lines not on this page · raw message
Signature · 1 line
2.14.3
Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] block: Removed unused sector-based blocking I/O
Quoted 6 lines · Eric Blake
We are gradually moving away from sector-based interfaces, towards byte-based. Now that all callers of blocking I/O have been converted to use our preferred byte-based bdrv_p{read,write}(), we can delete the unused bdrv_{read,write}().
Signed-off-by Eric Blake <[email protected]>
block/io.c static int bdrv_prwv_co(BdrvChild *child, int64_t offset, · line 732
Quoted 12 lines
block/io.c +0
-12-static int bdrv_rw_co (BdrvChild *child,int64_t sector_num,uint8_t *buf, -int nb_sectors,bool is_write,BdrvRequestFlags flags) -{ -QEMUIOVector qiov; -struct iovec iov = { - .iov_base = (void *)buf, - .iov_len = nb_sectors *BDRV_SECTOR_SIZE , - }; - -if (nb_sectors <0 || nb_sectors >BDRV_REQUEST_MAX_SECTORS ) { -return -EINVAL ; - }
Do we have a check for BDRV_REQUEST_MAX_BYTES in the byte-based API?
Berto
Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] block: Removed unused sector-based blocking I/O
Quoted 10 lines · Alberto Garcia
Quoted 6 lines · Eric Blake
We are gradually moving away from sector-based interfaces, towards byte-based. Now that all callers of blocking I/O have been converted to use our preferred byte-based bdrv_p{read,write}(), we can delete the unused bdrv_{read,write}().
Signed-off-by Eric Blake <[email protected]>Reviewed-by Alberto Garcia <[email protected]>
Quoted 14 lines
block/io.c static int bdrv_prwv_co(BdrvChild *child, int64_t offset, · line 732
Quoted 12 lines
block/io.c +0
-12-static int bdrv_rw_co (BdrvChild *child,int64_t sector_num,uint8_t *buf, -int nb_sectors,bool is_write,BdrvRequestFlags flags) -{ -QEMUIOVector qiov; -struct iovec iov = { - .iov_base = (void *)buf, - .iov_len = nb_sectors *BDRV_SECTOR_SIZE , - }; - -if (nb_sectors <0 || nb_sectors >BDRV_REQUEST_MAX_SECTORS ) { -return -EINVAL ; - }Do we have a check for
BDRV_REQUEST_MAX_BYTESin the byte-based API?
No, but we don't need one. First, note that bs->bl.max_transfer is currently uint32_t, so right now, no driver can set it any larger than 4G; although they can certainly set it smaller (for example, NBD sets it to 32M). Then note that bdrv_co_pwritev() and friends take care of fragmenting any >4G request down to max_transfer. The old check was to ensure that a sector count couldn't overflow larger than a 4G byte count (since the sector API had a 32-bit intrinsic limit on byte count, even though the parameter was in sectors); but the public byte-based API doesn't have an intrinsic limit on byte count, and takes care of fragmenting down to any intrinsic limit for the driver callbacks.
But, having said that, you've made me wonder if it's time to increase max_transfer to [u]int64_t, then audit ALL drivers to ensure that they either properly handle requests >=4G or else set max_transfer <4G (ideally, the block layer will default max_transfer to the value of BDRV_REQUEST_MAX_BYTES, even if the audit means we no longer need that macro). Should be a separate series, though.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] block: Removed unused sector-based blocking I/O
Quoted 15 lines · Eric Blake
Quoted 6 lines
Quoted 6 lines
block/io.c +0
-6-static int bdrv_rw_co (BdrvChild *child,int64_t sector_num,uint8_t *buf, -int nb_sectors,bool is_write,BdrvRequestFlags flags) -{ -QEMUIOVector qiov; -struct iovec iov = { - .iov_base = (void *)buf,Quoted 8 lines
block/io.c static int bdrv_prwv_co(BdrvChild *child, int64_t offset, · line 732
Quoted 6 lines
block/io.c +0
-6- .iov_len = nb_sectors * BDRV_SECTOR_SIZE, - }; - -if (nb_sectors <0 || nb_sectors >BDRV_REQUEST_MAX_SECTORS ) { -return -EINVAL ; - }Do we have a check for
BDRV_REQUEST_MAX_BYTESin the byte-based API?
Quoted 3 lines
No, but we don't need one. First, note that
bs->bl.max_transferis currentlyuint32_t, so right now, no driver can set it any larger than 4G
But note that the old limit was based on a signed integer.
BDRV_REQUEST_MAX_SECTORS is 2147483136 bytes (a bit less than 2GB).
For 32-bit integers, (INT_MAX - 1) & ~511 = 2147483136
Berto
Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] block: Removed unused sector-based blocking I/O
[Reviving an old series]
Quoted 20 lines · Alberto Garcia
Quoted 15 lines · Eric Blake
Quoted 6 lines
Quoted 6 lines
block/io.c +0
-6-static int bdrv_rw_co (BdrvChild *child,int64_t sector_num,uint8_t *buf, -int nb_sectors,bool is_write,BdrvRequestFlags flags) -{ -QEMUIOVector qiov; -struct iovec iov = { - .iov_base = (void *)buf,Quoted 8 lines
block/io.c static int bdrv_prwv_co(BdrvChild *child, int64_t offset, · line 732
Quoted 6 lines
block/io.c +0
-6- .iov_len = nb_sectors * BDRV_SECTOR_SIZE, - }; - -if (nb_sectors <0 || nb_sectors >BDRV_REQUEST_MAX_SECTORS ) { -return -EINVAL ; - }Do we have a check for
BDRV_REQUEST_MAX_BYTESin the byte-based API?Quoted 3 lines
No, but we don't need one. First, note that
bs->bl.max_transferis currentlyuint32_t, so right now, no driver can set it any larger than 4G
Quoted 5 lines
But note that the old limit was based on a signed integer.
BDRV_REQUEST_MAX_SECTORSis 2147483136 bytes (a bit less than 2GB).For 32-bit integers, (
INT_MAX- 1) & ~511 = 2147483136
The whole point of the byte-based callbacks is that they pass a 64-bit length BUT also honor the driver's max_transfer. As long as drivers default to (or explicitly set) a max_transfer of BDRV_REQUEST_MAX_SECTORS or smaller, then the block layer has already taken care of fragmenting things so that the callers no longer have to worry about artificially fragmenting things themselves before handing something to the block layer that might overflow. And you snipped the rest of my mail, where I argued:
Quoted 7 lines
But, having said that, you've made me wonder if it's time to increase
max_transferto [u]int64_t, then audit ALL drivers to ensure that they either properly handle requests >=4G or else setmax_transfer<4G (ideally, the block layer will defaultmax_transferto the value ofBDRV_REQUEST_MAX_BYTES, even if the audit means we no longer need that macro). Should be a separate series, though.
I will concede that you are right that because we previously used a signed type, I should amend my argument to state that we should audit ALL drivers to ensure that they either properly handle requests >= 2G or else set max_transfer <2G, even though max_transfer is unsigned. Maybe it's time that I attempt that audit, before posting a v2 of this series for 4.0. (Fingers crossed that it will be a quick audit)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] block: Removed unused sector-based blocking I/O
In reply to Eric BlakeQuoted 10 lines · Eric Blake
We are gradually moving away from sector-based interfaces, towards byte-based. Now that all callers of blocking I/O have been converted to use our preferred byte-based bdrv_p{read,write}(), we can delete the unused bdrv_{read,write}().
Signed-off-by Eric Blake <[email protected]>--- include/block/block.h | 4 ---- block/io.c | 48 ++++++++---------------------------------------- 2 files changed, 8 insertions(+), 44 deletions(-)