| Message ID | 1464128732-12667-3-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 25.05.2016 um 00:25 hat Eric Blake geschrieben: > Another step towards removing sector-based interfaces: convert > the maximum write and minimum alignment values from sectorss to s/sectorss/sectors/ > bytes. Alignment is changed to 'int', since it makes no sense > to have an alignment larger than the maximum write. Add an > assert that no one was trying to use sectors to get a write > zeroes larger than 2G. Rename the variables to let the compiler > check that all users are converted to the new semantics. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -1706,12 +1706,10 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) > } > > if (iscsilun->bl.max_ws_len < 0xffffffff) { > - bs->bl.max_write_zeroes = > - sector_limits_lun2qemu(iscsilun->bl.max_ws_len, iscsilun); > + bs->bl.max_pwrite_zeroes = iscsilun->bl.max_ws_len; Wrong unit, I think. You need to multiply by iscsi_lun->block_size. > } > if (iscsilun->lbp.lbpws) { > - bs->bl.write_zeroes_alignment = > - sector_limits_lun2qemu(iscsilun->bl.opt_unmap_gran, iscsilun); > + bs->bl.pwrite_zeroes_alignment = iscsilun->bl.opt_unmap_gran; Same here. > } > bs->bl.opt_transfer_length = > sector_limits_lun2qemu(iscsilun->bl.opt_xfer_len, iscsilun); > diff --git a/block/qcow2.c b/block/qcow2.c > index c9306a7..745b66f 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1193,7 +1193,7 @@ static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp) > { > BDRVQcow2State *s = bs->opaque; > > - bs->bl.write_zeroes_alignment = s->cluster_sectors; > + bs->bl.pwrite_zeroes_alignment = s->cluster_sectors << BDRV_SECTOR_BITS; This is s->cluster_size. I hope to get rid of s->cluster_sectors eventually. :-) Kevin
On 05/25/2016 04:30 AM, Kevin Wolf wrote: > Am 25.05.2016 um 00:25 hat Eric Blake geschrieben: >> Another step towards removing sector-based interfaces: convert >> the maximum write and minimum alignment values from sectorss to > > s/sectorss/sectors/ > >> bytes. Alignment is changed to 'int', since it makes no sense >> to have an alignment larger than the maximum write. Add an >> assert that no one was trying to use sectors to get a write >> zeroes larger than 2G. Rename the variables to let the compiler >> check that all users are converted to the new semantics. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > >> --- a/block/iscsi.c >> +++ b/block/iscsi.c >> @@ -1706,12 +1706,10 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) >> } >> >> if (iscsilun->bl.max_ws_len < 0xffffffff) { >> - bs->bl.max_write_zeroes = >> - sector_limits_lun2qemu(iscsilun->bl.max_ws_len, iscsilun); >> + bs->bl.max_pwrite_zeroes = iscsilun->bl.max_ws_len; > > Wrong unit, I think. You need to multiply by iscsi_lun->block_size. Hmm, I think you're right. What's more, I need to make sure the result doesn't wrap around INT_MAX (a device with 4k block size that supports 8G limits via 2M max blocks should still allow up to 2G transactions from qemu). I'm also thinking that in v2, it may be easier to reason about alignment limits if I convert alignment numbers to uint32_t, although we are still capped by INT_MAX in our various blk_* interfaces (worrying about signed overflow is a pain). >> +++ b/block/qcow2.c >> @@ -1193,7 +1193,7 @@ static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp) >> { >> BDRVQcow2State *s = bs->opaque; >> >> - bs->bl.write_zeroes_alignment = s->cluster_sectors; >> + bs->bl.pwrite_zeroes_alignment = s->cluster_sectors << BDRV_SECTOR_BITS; > > This is s->cluster_size. I hope to get rid of s->cluster_sectors > eventually. :-) Should I go ahead and convert ALL of BlockLimits to be byte-based limits, rather than an odd mix of sector vs. byte limits? Should I add any assertions for power-of-2 limits? Do we want to allow 0x80000000 as a valid length limit? [/me Should I be regretting touching this can of worms in the first place? :) ]
diff --git a/include/block/block_int.h b/include/block/block_int.h index b6f4755..4282ffd 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -328,11 +328,11 @@ typedef struct BlockLimits { /* optimal alignment for discard requests in sectors */ int64_t discard_alignment; - /* maximum number of sectors that can zeroized at once */ - int max_write_zeroes; + /* maximum number of bytes that can zeroized at once */ + int max_pwrite_zeroes; - /* optimal alignment for write zeroes requests in sectors */ - int64_t write_zeroes_alignment; + /* optimal alignment for write zeroes requests in bytes */ + int pwrite_zeroes_alignment; /* optimal transfer length in sectors */ int opt_transfer_length; diff --git a/block/io.c b/block/io.c index 2a2ff84..41b4e9d 100644 --- a/block/io.c +++ b/block/io.c @@ -1120,32 +1120,35 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs, int ret = 0; bool need_flush = false; - int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_write_zeroes, - BDRV_REQUEST_MAX_SECTORS); + int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX); + int max_write_zeroes_sectors = max_write_zeroes >> BDRV_SECTOR_BITS; + int write_zeroes_sector_align = + bs->bl.pwrite_zeroes_alignment >> BDRV_SECTOR_BITS; + assert(nb_sectors <= BDRV_REQUEST_MAX_SECTORS); while (nb_sectors > 0 && !ret) { int num = nb_sectors; /* Align request. Block drivers can expect the "bulk" of the request * to be aligned. */ - if (bs->bl.write_zeroes_alignment - && num > bs->bl.write_zeroes_alignment) { - if (sector_num % bs->bl.write_zeroes_alignment != 0) { + if (write_zeroes_sector_align + && num > write_zeroes_sector_align) { + if (sector_num % write_zeroes_sector_align != 0) { /* Make a small request up to the first aligned sector. */ - num = bs->bl.write_zeroes_alignment; - num -= sector_num % bs->bl.write_zeroes_alignment; - } else if ((sector_num + num) % bs->bl.write_zeroes_alignment != 0) { + num = write_zeroes_sector_align; + num -= sector_num % write_zeroes_sector_align; + } else if ((sector_num + num) % write_zeroes_sector_align != 0) { /* Shorten the request to the last aligned sector. num cannot - * underflow because num > bs->bl.write_zeroes_alignment. + * underflow because num > write_zeroes_sector_align. */ - num -= (sector_num + num) % bs->bl.write_zeroes_alignment; + num -= (sector_num + num) % write_zeroes_sector_align; } } /* limit request size */ - if (num > max_write_zeroes) { - num = max_write_zeroes; + if (num > max_write_zeroes_sectors) { + num = max_write_zeroes_sectors; } ret = -ENOTSUP; diff --git a/block/iscsi.c b/block/iscsi.c index 10f3906..0acc3dc 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1706,12 +1706,10 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) } if (iscsilun->bl.max_ws_len < 0xffffffff) { - bs->bl.max_write_zeroes = - sector_limits_lun2qemu(iscsilun->bl.max_ws_len, iscsilun); + bs->bl.max_pwrite_zeroes = iscsilun->bl.max_ws_len; } if (iscsilun->lbp.lbpws) { - bs->bl.write_zeroes_alignment = - sector_limits_lun2qemu(iscsilun->bl.opt_unmap_gran, iscsilun); + bs->bl.pwrite_zeroes_alignment = iscsilun->bl.opt_unmap_gran; } bs->bl.opt_transfer_length = sector_limits_lun2qemu(iscsilun->bl.opt_xfer_len, iscsilun); diff --git a/block/qcow2.c b/block/qcow2.c index c9306a7..745b66f 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1193,7 +1193,7 @@ static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp) { BDRVQcow2State *s = bs->opaque; - bs->bl.write_zeroes_alignment = s->cluster_sectors; + bs->bl.pwrite_zeroes_alignment = s->cluster_sectors << BDRV_SECTOR_BITS; } static int qcow2_set_key(BlockDriverState *bs, const char *key) diff --git a/block/qed.c b/block/qed.c index b591d4a..0ab5b40 100644 --- a/block/qed.c +++ b/block/qed.c @@ -518,7 +518,7 @@ static void bdrv_qed_refresh_limits(BlockDriverState *bs, Error **errp) { BDRVQEDState *s = bs->opaque; - bs->bl.write_zeroes_alignment = s->header.cluster_size >> BDRV_SECTOR_BITS; + bs->bl.pwrite_zeroes_alignment = s->header.cluster_size; } /* We have nothing to do for QED reopen, stubs just return diff --git a/block/vmdk.c b/block/vmdk.c index 372e5ed..8494d63 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -998,9 +998,9 @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp) for (i = 0; i < s->num_extents; i++) { if (!s->extents[i].flat) { - bs->bl.write_zeroes_alignment = - MAX(bs->bl.write_zeroes_alignment, - s->extents[i].cluster_sectors); + bs->bl.pwrite_zeroes_alignment = + MAX(bs->bl.pwrite_zeroes_alignment, + s->extents[i].cluster_sectors << BDRV_SECTOR_BITS); } } }
Another step towards removing sector-based interfaces: convert the maximum write and minimum alignment values from sectorss to bytes. Alignment is changed to 'int', since it makes no sense to have an alignment larger than the maximum write. Add an assert that no one was trying to use sectors to get a write zeroes larger than 2G. Rename the variables to let the compiler check that all users are converted to the new semantics. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/block/block_int.h | 8 ++++---- block/io.c | 27 +++++++++++++++------------ block/iscsi.c | 6 ++---- block/qcow2.c | 2 +- block/qed.c | 2 +- block/vmdk.c | 6 +++--- 6 files changed, 26 insertions(+), 25 deletions(-)