| Message ID | 1466721446-27737-18-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 06/23 16:37, Eric Blake wrote: > Sector-based limits are awkward to think about; in our on-going > quest to move to byte-based interfaces, convert max_discard and > discard_alignment. Rename them, using 'pdiscard' as an aid to > track which remaining discard interfaces need conversion, and so > that the compiler will help us catch the change in semantics > across any rebased code. The BlockLimits type is now completely > byte-based; and in iscsi.c, sector_limits_lun2qemu() is no > longer needed. > > pdiscard_alignment is made unsigned (we use power-of-2 alignments > as bitmasks, where unsigned is easier to think about) while > leaving max_pdiscard signed (since we still have an 'int' > interface); this is comparable to what commit cf081fc did for > write zeroes limits. We may later want to make everything an > unsigned 64-bit limit - but that requires a bigger code audit. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v3: split out write_zeroes wording tweaks, improve commit message > v2: rebase nbd and iscsi limits across earlier improvements > --- > include/block/block_int.h | 14 ++++++++++---- > block/io.c | 16 +++++++++------- > block/iscsi.c | 19 ++++++------------- > block/nbd.c | 2 +- > qemu-img.c | 3 ++- > 5 files changed, 28 insertions(+), 26 deletions(-) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 7a4a00f..388ef80 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -324,11 +324,17 @@ struct BlockDriver { > }; > > typedef struct BlockLimits { > - /* maximum number of sectors that can be discarded at once */ > - int max_discard; > + /* maximum number of bytes that can be discarded at once (since it > + * is signed, it must be < 2G, if set), should be multiple of > + * pdiscard_alignment, but need not be power of 2. May be 0 if no > + * inherent 32-bit limit */ > + int32_t max_pdiscard; > > - /* optimal alignment for discard requests in sectors */ > - int64_t discard_alignment; > + /* optimal alignment for discard requests in bytes, must be power > + * of 2, less than max_discard if that is set, and multiple of s/max_discard/max_pdiscard/ > + * bs->request_alignment. May be 0 if bs->request_alignment is > + * good enough */ > + uint32_t pdiscard_alignment; > > /* maximum number of bytes that can zeroized at once (since it is > * signed, it must be < 2G, if set), should be multiple of > diff --git a/block/io.c b/block/io.c > index 8ca9d43..0f15d05 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -2368,19 +2368,21 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, > goto out; > } > > - max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS); > + max_discard = MIN_NON_ZERO(bs->bl.max_pdiscard >> BDRV_SECTOR_BITS, > + BDRV_REQUEST_MAX_SECTORS); > while (nb_sectors > 0) { > int ret; > int num = nb_sectors; > + int discard_alignment = bs->bl.pdiscard_alignment >> BDRV_SECTOR_BITS; > > /* align request */ > - if (bs->bl.discard_alignment && > - num >= bs->bl.discard_alignment && > - sector_num % bs->bl.discard_alignment) { > - if (num > bs->bl.discard_alignment) { > - num = bs->bl.discard_alignment; > + if (discard_alignment && > + num >= discard_alignment && > + sector_num % discard_alignment) { > + if (num > discard_alignment) { > + num = discard_alignment; > } > - num -= sector_num % bs->bl.discard_alignment; > + num -= sector_num % discard_alignment; Or just num = discard_alignment - sector_num % discard_alignment; without the if. Otherwise looks good, Reviewed-by: Fam Zheng <famz@redhat.com> > } > > /* limit request size */ > diff --git a/block/iscsi.c b/block/iscsi.c > index 368687d..0d16c31 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -1696,13 +1696,6 @@ static void iscsi_close(BlockDriverState *bs) > memset(iscsilun, 0, sizeof(IscsiLun)); > } > > -static int sector_limits_lun2qemu(int64_t sector, IscsiLun *iscsilun) > -{ > - int limit = MIN(sector_lun2qemu(sector, iscsilun), INT_MAX / 2 + 1); > - > - return limit < BDRV_REQUEST_MAX_SECTORS ? limit : 0; > -} > - > static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) > { > /* We don't actually refresh here, but just return data queried in > @@ -1722,14 +1715,14 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) > } > > if (iscsilun->lbp.lbpu) { > - if (iscsilun->bl.max_unmap < 0xffffffff) { > - bs->bl.max_discard = > - sector_limits_lun2qemu(iscsilun->bl.max_unmap, iscsilun); > + if (iscsilun->bl.max_unmap < 0xffffffff / iscsilun->block_size) { > + bs->bl.max_pdiscard = > + iscsilun->bl.max_unmap * iscsilun->block_size; > } > - bs->bl.discard_alignment = > - sector_limits_lun2qemu(iscsilun->bl.opt_unmap_gran, iscsilun); > + bs->bl.pdiscard_alignment = > + iscsilun->bl.opt_unmap_gran * iscsilun->block_size; > } else { > - bs->bl.discard_alignment = iscsilun->block_size >> BDRV_SECTOR_BITS; > + bs->bl.pdiscard_alignment = iscsilun->block_size; > } > > if (iscsilun->bl.max_ws_len < 0xffffffff / iscsilun->block_size) {
On 06/24/2016 12:43 AM, Fam Zheng wrote: > On Thu, 06/23 16:37, Eric Blake wrote: >> Sector-based limits are awkward to think about; in our on-going >> quest to move to byte-based interfaces, convert max_discard and >> discard_alignment. Rename them, using 'pdiscard' as an aid to >> track which remaining discard interfaces need conversion, and so >> that the compiler will help us catch the change in semantics >> across any rebased code. The BlockLimits type is now completely >> byte-based; and in iscsi.c, sector_limits_lun2qemu() is no >> longer needed. >> >> pdiscard_alignment is made unsigned (we use power-of-2 alignments >> as bitmasks, where unsigned is easier to think about) while >> leaving max_pdiscard signed (since we still have an 'int' >> interface); this is comparable to what commit cf081fc did for >> write zeroes limits. We may later want to make everything an >> unsigned 64-bit limit - but that requires a bigger code audit. >> >> - /* optimal alignment for discard requests in sectors */ >> - int64_t discard_alignment; >> + /* optimal alignment for discard requests in bytes, must be power >> + * of 2, less than max_discard if that is set, and multiple of > > s/max_discard/max_pdiscard/ Maintainer could touch it up on pull request. >> /* align request */ >> - if (bs->bl.discard_alignment && >> - num >= bs->bl.discard_alignment && >> - sector_num % bs->bl.discard_alignment) { >> - if (num > bs->bl.discard_alignment) { >> - num = bs->bl.discard_alignment; >> + if (discard_alignment && >> + num >= discard_alignment && >> + sector_num % discard_alignment) { >> + if (num > discard_alignment) { >> + num = discard_alignment; >> } >> - num -= sector_num % bs->bl.discard_alignment; >> + num -= sector_num % discard_alignment; > > Or just > > num = discard_alignment - sector_num % discard_alignment; > > without the if. > Sure. It all gets simplified later when I switch to bdrv_co_pdiscard(). Up to the maintainer. > Otherwise looks good, > > Reviewed-by: Fam Zheng <famz@redhat.com> >
Am 24.06.2016 um 16:15 hat Eric Blake geschrieben: > On 06/24/2016 12:43 AM, Fam Zheng wrote: > > On Thu, 06/23 16:37, Eric Blake wrote: > >> Sector-based limits are awkward to think about; in our on-going > >> quest to move to byte-based interfaces, convert max_discard and > >> discard_alignment. Rename them, using 'pdiscard' as an aid to > >> track which remaining discard interfaces need conversion, and so > >> that the compiler will help us catch the change in semantics > >> across any rebased code. The BlockLimits type is now completely > >> byte-based; and in iscsi.c, sector_limits_lun2qemu() is no > >> longer needed. > >> > >> pdiscard_alignment is made unsigned (we use power-of-2 alignments > >> as bitmasks, where unsigned is easier to think about) while > >> leaving max_pdiscard signed (since we still have an 'int' > >> interface); this is comparable to what commit cf081fc did for > >> write zeroes limits. We may later want to make everything an > >> unsigned 64-bit limit - but that requires a bigger code audit. > >> > > >> - /* optimal alignment for discard requests in sectors */ > >> - int64_t discard_alignment; > >> + /* optimal alignment for discard requests in bytes, must be power > >> + * of 2, less than max_discard if that is set, and multiple of > > > > s/max_discard/max_pdiscard/ > > Maintainer could touch it up on pull request. Okay, no problem. > >> /* align request */ > >> - if (bs->bl.discard_alignment && > >> - num >= bs->bl.discard_alignment && > >> - sector_num % bs->bl.discard_alignment) { > >> - if (num > bs->bl.discard_alignment) { > >> - num = bs->bl.discard_alignment; > >> + if (discard_alignment && > >> + num >= discard_alignment && > >> + sector_num % discard_alignment) { > >> + if (num > discard_alignment) { > >> + num = discard_alignment; > >> } > >> - num -= sector_num % bs->bl.discard_alignment; > >> + num -= sector_num % discard_alignment; > > > > Or just > > > > num = discard_alignment - sector_num % discard_alignment; > > > > without the if. > > > > Sure. It all gets simplified later when I switch to bdrv_co_pdiscard(). > Up to the maintainer. This is an actual code change and not a bug fix, so I'll leave this one alone. We can always have a follow-up patch, but as you say your other work will simplify it anyway, I guess that's not necessary. Kevin
diff --git a/include/block/block_int.h b/include/block/block_int.h index 7a4a00f..388ef80 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -324,11 +324,17 @@ struct BlockDriver { }; typedef struct BlockLimits { - /* maximum number of sectors that can be discarded at once */ - int max_discard; + /* maximum number of bytes that can be discarded at once (since it + * is signed, it must be < 2G, if set), should be multiple of + * pdiscard_alignment, but need not be power of 2. May be 0 if no + * inherent 32-bit limit */ + int32_t max_pdiscard; - /* optimal alignment for discard requests in sectors */ - int64_t discard_alignment; + /* optimal alignment for discard requests in bytes, must be power + * of 2, less than max_discard if that is set, and multiple of + * bs->request_alignment. May be 0 if bs->request_alignment is + * good enough */ + uint32_t pdiscard_alignment; /* maximum number of bytes that can zeroized at once (since it is * signed, it must be < 2G, if set), should be multiple of diff --git a/block/io.c b/block/io.c index 8ca9d43..0f15d05 100644 --- a/block/io.c +++ b/block/io.c @@ -2368,19 +2368,21 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, goto out; } - max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS); + max_discard = MIN_NON_ZERO(bs->bl.max_pdiscard >> BDRV_SECTOR_BITS, + BDRV_REQUEST_MAX_SECTORS); while (nb_sectors > 0) { int ret; int num = nb_sectors; + int discard_alignment = bs->bl.pdiscard_alignment >> BDRV_SECTOR_BITS; /* align request */ - if (bs->bl.discard_alignment && - num >= bs->bl.discard_alignment && - sector_num % bs->bl.discard_alignment) { - if (num > bs->bl.discard_alignment) { - num = bs->bl.discard_alignment; + if (discard_alignment && + num >= discard_alignment && + sector_num % discard_alignment) { + if (num > discard_alignment) { + num = discard_alignment; } - num -= sector_num % bs->bl.discard_alignment; + num -= sector_num % discard_alignment; } /* limit request size */ diff --git a/block/iscsi.c b/block/iscsi.c index 368687d..0d16c31 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1696,13 +1696,6 @@ static void iscsi_close(BlockDriverState *bs) memset(iscsilun, 0, sizeof(IscsiLun)); } -static int sector_limits_lun2qemu(int64_t sector, IscsiLun *iscsilun) -{ - int limit = MIN(sector_lun2qemu(sector, iscsilun), INT_MAX / 2 + 1); - - return limit < BDRV_REQUEST_MAX_SECTORS ? limit : 0; -} - static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) { /* We don't actually refresh here, but just return data queried in @@ -1722,14 +1715,14 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) } if (iscsilun->lbp.lbpu) { - if (iscsilun->bl.max_unmap < 0xffffffff) { - bs->bl.max_discard = - sector_limits_lun2qemu(iscsilun->bl.max_unmap, iscsilun); + if (iscsilun->bl.max_unmap < 0xffffffff / iscsilun->block_size) { + bs->bl.max_pdiscard = + iscsilun->bl.max_unmap * iscsilun->block_size; } - bs->bl.discard_alignment = - sector_limits_lun2qemu(iscsilun->bl.opt_unmap_gran, iscsilun); + bs->bl.pdiscard_alignment = + iscsilun->bl.opt_unmap_gran * iscsilun->block_size; } else { - bs->bl.discard_alignment = iscsilun->block_size >> BDRV_SECTOR_BITS; + bs->bl.pdiscard_alignment = iscsilun->block_size; } if (iscsilun->bl.max_ws_len < 0xffffffff / iscsilun->block_size) { diff --git a/block/nbd.c b/block/nbd.c index f5511ea..08e5b67 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -362,7 +362,7 @@ static int nbd_co_flush(BlockDriverState *bs) static void nbd_refresh_limits(BlockDriverState *bs, Error **errp) { - bs->bl.max_discard = NBD_MAX_SECTORS; + bs->bl.max_pdiscard = NBD_MAX_BUFFER_SIZE; bs->bl.max_transfer = NBD_MAX_BUFFER_SIZE; } diff --git a/qemu-img.c b/qemu-img.c index cf9876d..1237d61 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2084,7 +2084,8 @@ static int img_convert(int argc, char **argv) bufsectors = MIN(32768, MAX(bufsectors, MAX(out_bs->bl.opt_transfer >> BDRV_SECTOR_BITS, - out_bs->bl.discard_alignment))); + out_bs->bl.pdiscard_alignment >> + BDRV_SECTOR_BITS))); if (skip_create) { int64_t output_sectors = blk_nb_sectors(out_blk);
Sector-based limits are awkward to think about; in our on-going quest to move to byte-based interfaces, convert max_discard and discard_alignment. Rename them, using 'pdiscard' as an aid to track which remaining discard interfaces need conversion, and so that the compiler will help us catch the change in semantics across any rebased code. The BlockLimits type is now completely byte-based; and in iscsi.c, sector_limits_lun2qemu() is no longer needed. pdiscard_alignment is made unsigned (we use power-of-2 alignments as bitmasks, where unsigned is easier to think about) while leaving max_pdiscard signed (since we still have an 'int' interface); this is comparable to what commit cf081fc did for write zeroes limits. We may later want to make everything an unsigned 64-bit limit - but that requires a bigger code audit. Signed-off-by: Eric Blake <eblake@redhat.com> --- v3: split out write_zeroes wording tweaks, improve commit message v2: rebase nbd and iscsi limits across earlier improvements --- include/block/block_int.h | 14 ++++++++++---- block/io.c | 16 +++++++++------- block/iscsi.c | 19 ++++++------------- block/nbd.c | 2 +- qemu-img.c | 3 ++- 5 files changed, 28 insertions(+), 26 deletions(-)