Message ID | 20170411222945.11741-2-eblake@redhat.com |
---|---|
State | New |
Headers | show |
On 04/11/2017 06:29 PM, Eric Blake wrote: > The user interface specifies job rate limits in bytes/second. > It's pointless to have our internal representation track things > in sectors/second, particularly since we want to move away from > sector-based interfaces. > > Fix up a doc typo found while verifying that the ratelimit > code handles the scaling difference. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > include/qemu/ratelimit.h | 3 ++- > block/backup.c | 5 +++-- > block/commit.c | 5 +++-- > block/mirror.c | 13 +++++++------ > block/stream.c | 5 +++-- > 5 files changed, 18 insertions(+), 13 deletions(-) > > diff --git a/include/qemu/ratelimit.h b/include/qemu/ratelimit.h > index 8da1232..8dece48 100644 > --- a/include/qemu/ratelimit.h > +++ b/include/qemu/ratelimit.h > @@ -24,7 +24,8 @@ typedef struct { > > /** Calculate and return delay for next request in ns > * > - * Record that we sent @p n data units. If we may send more data units > + * Record that we sent @n data units (where @n matches the scale chosen > + * during ratelimit_set_speed). If we may send more data units > * in the current time slice, return 0 (i.e. no delay). Otherwise > * return the amount of time (in ns) until the start of the next time > * slice that will permit sending the next chunk of data. > diff --git a/block/backup.c b/block/backup.c > index a4fb288..bcfa321 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -208,7 +208,7 @@ static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp) > error_setg(errp, QERR_INVALID_PARAMETER, "speed"); > return; > } > - ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME); > + ratelimit_set_speed(&s->limit, speed, SLICE_TIME); > } > > static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret) > @@ -359,7 +359,8 @@ static bool coroutine_fn yield_and_check(BackupBlockJob *job) > */ > if (job->common.speed) { > uint64_t delay_ns = ratelimit_calculate_delay(&job->limit, > - job->sectors_read); > + job->sectors_read * > + BDRV_SECTOR_SIZE); > job->sectors_read = 0; > block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns); > } else { > diff --git a/block/commit.c b/block/commit.c > index 76a0d98..8b684e0 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -195,7 +195,8 @@ static void coroutine_fn commit_run(void *opaque) > s->common.offset += n * BDRV_SECTOR_SIZE; > > if (copy && s->common.speed) { > - delay_ns = ratelimit_calculate_delay(&s->limit, n); > + delay_ns = ratelimit_calculate_delay(&s->limit, > + n * BDRV_SECTOR_SIZE); You could probably factor out this calculation in conjunction with the offset update above, but no matter. > } > } > > @@ -217,7 +218,7 @@ static void commit_set_speed(BlockJob *job, int64_t speed, Error **errp) > error_setg(errp, QERR_INVALID_PARAMETER, "speed"); > return; > } > - ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME); > + ratelimit_set_speed(&s->limit, speed, SLICE_TIME); > } > > static const BlockJobDriver commit_job_driver = { > diff --git a/block/mirror.c b/block/mirror.c > index 2173a2f..a7d0960 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -391,7 +391,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) > bitmap_set(s->in_flight_bitmap, sector_num / sectors_per_chunk, nb_chunks); > while (nb_chunks > 0 && sector_num < end) { > int64_t ret; > - int io_sectors, io_sectors_acct; > + int io_sectors; > + int64_t io_bytes_acct; > BlockDriverState *file; > enum MirrorMethod { > MIRROR_METHOD_COPY, > @@ -439,16 +440,16 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) > switch (mirror_method) { > case MIRROR_METHOD_COPY: > io_sectors = mirror_do_read(s, sector_num, io_sectors); > - io_sectors_acct = io_sectors; > + io_bytes_acct = io_sectors * BDRV_SECTOR_SIZE; > break; > case MIRROR_METHOD_ZERO: > case MIRROR_METHOD_DISCARD: > mirror_do_zero_or_discard(s, sector_num, io_sectors, > mirror_method == MIRROR_METHOD_DISCARD); > if (write_zeroes_ok) { > - io_sectors_acct = 0; > + io_bytes_acct = 0; > } else { > - io_sectors_acct = io_sectors; > + io_bytes_acct = io_sectors * BDRV_SECTOR_SIZE; > } > break; > default: > @@ -458,7 +459,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) > sector_num += io_sectors; > nb_chunks -= DIV_ROUND_UP(io_sectors, sectors_per_chunk); > if (s->common.speed) { > - delay_ns = ratelimit_calculate_delay(&s->limit, io_sectors_acct); > + delay_ns = ratelimit_calculate_delay(&s->limit, io_bytes_acct); > } > } > return delay_ns; > @@ -918,7 +919,7 @@ static void mirror_set_speed(BlockJob *job, int64_t speed, Error **errp) > error_setg(errp, QERR_INVALID_PARAMETER, "speed"); > return; > } > - ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME); > + ratelimit_set_speed(&s->limit, speed, SLICE_TIME); > } > > static void mirror_complete(BlockJob *job, Error **errp) > diff --git a/block/stream.c b/block/stream.c > index 0113710..11c5cf1 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -191,7 +191,8 @@ static void coroutine_fn stream_run(void *opaque) > /* Publish progress */ > s->common.offset += n * BDRV_SECTOR_SIZE; > if (copy && s->common.speed) { > - delay_ns = ratelimit_calculate_delay(&s->limit, n); > + delay_ns = ratelimit_calculate_delay(&s->limit, > + n * BDRV_SECTOR_SIZE); Same kind of comment here. > } > } > > @@ -220,7 +221,7 @@ static void stream_set_speed(BlockJob *job, int64_t speed, Error **errp) > error_setg(errp, QERR_INVALID_PARAMETER, "speed"); > return; > } > - ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME); > + ratelimit_set_speed(&s->limit, speed, SLICE_TIME); > } > > static const BlockJobDriver stream_job_driver = { > Reviewed-by: John Snow <jsnow@redhat.com>
On 04/17/2017 02:18 PM, John Snow wrote: > > > On 04/11/2017 06:29 PM, Eric Blake wrote: >> The user interface specifies job rate limits in bytes/second. >> It's pointless to have our internal representation track things >> in sectors/second, particularly since we want to move away from >> sector-based interfaces. >> >> Fix up a doc typo found while verifying that the ratelimit >> code handles the scaling difference. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> +++ b/block/commit.c >> @@ -195,7 +195,8 @@ static void coroutine_fn commit_run(void *opaque) >> s->common.offset += n * BDRV_SECTOR_SIZE; >> >> if (copy && s->common.speed) { >> - delay_ns = ratelimit_calculate_delay(&s->limit, n); >> + delay_ns = ratelimit_calculate_delay(&s->limit, >> + n * BDRV_SECTOR_SIZE); > > You could probably factor out this calculation in conjunction with the > offset update above, but no matter. It gets simplified in a later patch, when I switch the entire function to track by bytes instead of sectors. >> +++ b/block/stream.c >> @@ -191,7 +191,8 @@ static void coroutine_fn stream_run(void *opaque) >> /* Publish progress */ >> s->common.offset += n * BDRV_SECTOR_SIZE; >> if (copy && s->common.speed) { >> - delay_ns = ratelimit_calculate_delay(&s->limit, n); >> + delay_ns = ratelimit_calculate_delay(&s->limit, >> + n * BDRV_SECTOR_SIZE); > > Same kind of comment here. And same response :)
diff --git a/include/qemu/ratelimit.h b/include/qemu/ratelimit.h index 8da1232..8dece48 100644 --- a/include/qemu/ratelimit.h +++ b/include/qemu/ratelimit.h @@ -24,7 +24,8 @@ typedef struct { /** Calculate and return delay for next request in ns * - * Record that we sent @p n data units. If we may send more data units + * Record that we sent @n data units (where @n matches the scale chosen + * during ratelimit_set_speed). If we may send more data units * in the current time slice, return 0 (i.e. no delay). Otherwise * return the amount of time (in ns) until the start of the next time * slice that will permit sending the next chunk of data. diff --git a/block/backup.c b/block/backup.c index a4fb288..bcfa321 100644 --- a/block/backup.c +++ b/block/backup.c @@ -208,7 +208,7 @@ static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp) error_setg(errp, QERR_INVALID_PARAMETER, "speed"); return; } - ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME); + ratelimit_set_speed(&s->limit, speed, SLICE_TIME); } static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret) @@ -359,7 +359,8 @@ static bool coroutine_fn yield_and_check(BackupBlockJob *job) */ if (job->common.speed) { uint64_t delay_ns = ratelimit_calculate_delay(&job->limit, - job->sectors_read); + job->sectors_read * + BDRV_SECTOR_SIZE); job->sectors_read = 0; block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns); } else { diff --git a/block/commit.c b/block/commit.c index 76a0d98..8b684e0 100644 --- a/block/commit.c +++ b/block/commit.c @@ -195,7 +195,8 @@ static void coroutine_fn commit_run(void *opaque) s->common.offset += n * BDRV_SECTOR_SIZE; if (copy && s->common.speed) { - delay_ns = ratelimit_calculate_delay(&s->limit, n); + delay_ns = ratelimit_calculate_delay(&s->limit, + n * BDRV_SECTOR_SIZE); } } @@ -217,7 +218,7 @@ static void commit_set_speed(BlockJob *job, int64_t speed, Error **errp) error_setg(errp, QERR_INVALID_PARAMETER, "speed"); return; } - ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME); + ratelimit_set_speed(&s->limit, speed, SLICE_TIME); } static const BlockJobDriver commit_job_driver = { diff --git a/block/mirror.c b/block/mirror.c index 2173a2f..a7d0960 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -391,7 +391,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) bitmap_set(s->in_flight_bitmap, sector_num / sectors_per_chunk, nb_chunks); while (nb_chunks > 0 && sector_num < end) { int64_t ret; - int io_sectors, io_sectors_acct; + int io_sectors; + int64_t io_bytes_acct; BlockDriverState *file; enum MirrorMethod { MIRROR_METHOD_COPY, @@ -439,16 +440,16 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) switch (mirror_method) { case MIRROR_METHOD_COPY: io_sectors = mirror_do_read(s, sector_num, io_sectors); - io_sectors_acct = io_sectors; + io_bytes_acct = io_sectors * BDRV_SECTOR_SIZE; break; case MIRROR_METHOD_ZERO: case MIRROR_METHOD_DISCARD: mirror_do_zero_or_discard(s, sector_num, io_sectors, mirror_method == MIRROR_METHOD_DISCARD); if (write_zeroes_ok) { - io_sectors_acct = 0; + io_bytes_acct = 0; } else { - io_sectors_acct = io_sectors; + io_bytes_acct = io_sectors * BDRV_SECTOR_SIZE; } break; default: @@ -458,7 +459,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) sector_num += io_sectors; nb_chunks -= DIV_ROUND_UP(io_sectors, sectors_per_chunk); if (s->common.speed) { - delay_ns = ratelimit_calculate_delay(&s->limit, io_sectors_acct); + delay_ns = ratelimit_calculate_delay(&s->limit, io_bytes_acct); } } return delay_ns; @@ -918,7 +919,7 @@ static void mirror_set_speed(BlockJob *job, int64_t speed, Error **errp) error_setg(errp, QERR_INVALID_PARAMETER, "speed"); return; } - ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME); + ratelimit_set_speed(&s->limit, speed, SLICE_TIME); } static void mirror_complete(BlockJob *job, Error **errp) diff --git a/block/stream.c b/block/stream.c index 0113710..11c5cf1 100644 --- a/block/stream.c +++ b/block/stream.c @@ -191,7 +191,8 @@ static void coroutine_fn stream_run(void *opaque) /* Publish progress */ s->common.offset += n * BDRV_SECTOR_SIZE; if (copy && s->common.speed) { - delay_ns = ratelimit_calculate_delay(&s->limit, n); + delay_ns = ratelimit_calculate_delay(&s->limit, + n * BDRV_SECTOR_SIZE); } } @@ -220,7 +221,7 @@ static void stream_set_speed(BlockJob *job, int64_t speed, Error **errp) error_setg(errp, QERR_INVALID_PARAMETER, "speed"); return; } - ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME); + ratelimit_set_speed(&s->limit, speed, SLICE_TIME); } static const BlockJobDriver stream_job_driver = {
The user interface specifies job rate limits in bytes/second. It's pointless to have our internal representation track things in sectors/second, particularly since we want to move away from sector-based interfaces. Fix up a doc typo found while verifying that the ratelimit code handles the scaling difference. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/qemu/ratelimit.h | 3 ++- block/backup.c | 5 +++-- block/commit.c | 5 +++-- block/mirror.c | 13 +++++++------ block/stream.c | 5 +++-- 5 files changed, 18 insertions(+), 13 deletions(-)