| Message ID | 1465939839-30097-16-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 06/14 15:30, 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. In iscsi.c, sector_limits_lun2qemu() > is no longer needed; and the BlockLimits type is now completely > byte-based. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: rebase nbd and iscsi limits across earlier improvements > --- > include/block/block_int.h | 21 +++++++++++++++------ > block/io.c | 16 +++++++++------- > block/iscsi.c | 19 ++++++------------- > block/nbd.c | 2 +- > qemu-img.c | 3 ++- > 5 files changed, 33 insertions(+), 28 deletions(-) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 2b45d57..0169019 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -324,18 +324,27 @@ 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; Why not use uint32_t? > > - /* 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) */ > + * signed, it must be < 2G, if set), should be multiple of > + * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */ > int32_t max_pwrite_zeroes; > > /* optimal alignment for write zeroes requests in bytes, must be > - * power of 2, and less than max_pwrite_zeroes if that is set */ > + * power of 2, less than max_pwrite_zeroes if that is set, and > + * multiple of bs->request_alignment. May be 0 if > + * bs->request_alignment is good enough */ > uint32_t pwrite_zeroes_alignment; > > /* optimal transfer length in bytes (must be power of 2, and > diff --git a/block/io.c b/block/io.c > index 5e38ab4..0b5c40d 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -2352,19 +2352,21 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, > BDRV_TRACKED_DISCARD); > bdrv_set_dirty(bs, sector_num, nb_sectors); > > - 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 739d5ca..af9babf 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); > -- > 2.5.5 >
On 06/15/2016 11:46 PM, Fam Zheng wrote: > On Tue, 06/14 15:30, 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. In iscsi.c, sector_limits_lun2qemu() >> is no longer needed; and the BlockLimits type is now completely >> byte-based. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> >> 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; > > Why not use uint32_t? > >> >> - /* 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) */ >> + * signed, it must be < 2G, if set), should be multiple of >> + * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */ >> int32_t max_pwrite_zeroes; Because max_pwrite_zeroes didn't. And because we're still limited by INT_MAX (or it's alternative spelling BDRV_REQUEST_MAX_SECTORS). Maybe we should switch both to uint32_t, but that can be a followup.
On Thu, 06/16 08:21, Eric Blake wrote: > On 06/15/2016 11:46 PM, Fam Zheng wrote: > > On Tue, 06/14 15:30, 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. In iscsi.c, sector_limits_lun2qemu() > >> is no longer needed; and the BlockLimits type is now completely > >> byte-based. > >> > >> Signed-off-by: Eric Blake <eblake@redhat.com> > >> > > >> > >> 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; > > > > Why not use uint32_t? > > > >> > >> - /* 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) */ > >> + * signed, it must be < 2G, if set), should be multiple of > >> + * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */ > >> int32_t max_pwrite_zeroes; > > Because max_pwrite_zeroes didn't. > > And because we're still limited by INT_MAX (or it's alternative spelling > BDRV_REQUEST_MAX_SECTORS). > > Maybe we should switch both to uint32_t, but that can be a followup. > OK, thanks! Reviewed-by: Fam Zheng <famz@redhat.com>
Am 14.06.2016 um 23:30 hat Eric Blake geschrieben: > 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. In iscsi.c, sector_limits_lun2qemu() > is no longer needed; and the BlockLimits type is now completely > byte-based. > > Signed-off-by: Eric Blake <eblake@redhat.com> > /* maximum number of bytes that can zeroized at once (since it is > - * signed, it must be < 2G, if set) */ > + * signed, it must be < 2G, if set), should be multiple of > + * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */ > int32_t max_pwrite_zeroes; > > /* optimal alignment for write zeroes requests in bytes, must be > - * power of 2, and less than max_pwrite_zeroes if that is set */ > + * power of 2, less than max_pwrite_zeroes if that is set, and > + * multiple of bs->request_alignment. May be 0 if > + * bs->request_alignment is good enough */ > uint32_t pwrite_zeroes_alignment; I think you intended to have these as part of some earlier patch, they are not related to discard. Kevin
On 06/21/2016 08:05 AM, Kevin Wolf wrote: > Am 14.06.2016 um 23:30 hat Eric Blake geschrieben: >> 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. In iscsi.c, sector_limits_lun2qemu() >> is no longer needed; and the BlockLimits type is now completely >> byte-based. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > >> /* maximum number of bytes that can zeroized at once (since it is >> - * signed, it must be < 2G, if set) */ >> + * signed, it must be < 2G, if set), should be multiple of >> + * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */ >> int32_t max_pwrite_zeroes; >> >> /* optimal alignment for write zeroes requests in bytes, must be >> - * power of 2, and less than max_pwrite_zeroes if that is set */ >> + * power of 2, less than max_pwrite_zeroes if that is set, and >> + * multiple of bs->request_alignment. May be 0 if >> + * bs->request_alignment is good enough */ >> uint32_t pwrite_zeroes_alignment; > > I think you intended to have these as part of some earlier patch, they > are not related to discard. Well, the patch they should have been in is already part of master (cf081fca), so I'll just split out the changes to a trivial prereq patch.
diff --git a/include/block/block_int.h b/include/block/block_int.h index 2b45d57..0169019 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -324,18 +324,27 @@ 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) */ + * signed, it must be < 2G, if set), should be multiple of + * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */ int32_t max_pwrite_zeroes; /* optimal alignment for write zeroes requests in bytes, must be - * power of 2, and less than max_pwrite_zeroes if that is set */ + * power of 2, less than max_pwrite_zeroes if that is set, and + * multiple of bs->request_alignment. May be 0 if + * bs->request_alignment is good enough */ uint32_t pwrite_zeroes_alignment; /* optimal transfer length in bytes (must be power of 2, and diff --git a/block/io.c b/block/io.c index 5e38ab4..0b5c40d 100644 --- a/block/io.c +++ b/block/io.c @@ -2352,19 +2352,21 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, BDRV_TRACKED_DISCARD); bdrv_set_dirty(bs, sector_num, nb_sectors); - 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 739d5ca..af9babf 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. In iscsi.c, sector_limits_lun2qemu() is no longer needed; and the BlockLimits type is now completely byte-based. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: rebase nbd and iscsi limits across earlier improvements --- include/block/block_int.h | 21 +++++++++++++++------ block/io.c | 16 +++++++++------- block/iscsi.c | 19 ++++++------------- block/nbd.c | 2 +- qemu-img.c | 3 ++- 5 files changed, 33 insertions(+), 28 deletions(-)