On 04/11/2017 06:29 PM, Eric Blake wrote: > Upcoming patches are going to switch to byte-based interfaces > instead of sector-based. Even worse, trace_backup_do_cow_enter() > had a weird mix of cluster and sector indices. Make the tracing > uniformly use bytes. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > block/backup.c | 16 ++++++++++------ > block/commit.c | 3 ++- > block/mirror.c | 26 +++++++++++++++++--------- > block/stream.c | 3 ++- > block/trace-events | 14 +++++++------- > 5 files changed, 38 insertions(+), 24 deletions(-) > > diff --git a/block/backup.c b/block/backup.c > index bcfa321..b28df8c 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -102,6 +102,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, > void *bounce_buffer = NULL; > int ret = 0; > int64_t sectors_per_cluster = cluster_size_sectors(job); > + int64_t bytes_per_cluster = sectors_per_cluster * BDRV_SECTOR_SIZE; > int64_t start, end; > int n; > > @@ -110,18 +111,20 @@ static int coroutine_fn backup_do_cow(BackupBlockJob > *job, > start = sector_num / sectors_per_cluster; > end = DIV_ROUND_UP(sector_num + nb_sectors, sectors_per_cluster); > > - trace_backup_do_cow_enter(job, start, sector_num, nb_sectors); > + trace_backup_do_cow_enter(job, start * bytes_per_cluster, > + sector_num * BDRV_SECTOR_SIZE, > + nb_sectors * BDRV_SECTOR_SIZE); > > wait_for_overlapping_requests(job, start, end); > cow_request_begin(&cow_request, job, start, end); > > for (; start < end; start++) { > if (test_bit(start, job->done_bitmap)) { > - trace_backup_do_cow_skip(job, start); > + trace_backup_do_cow_skip(job, start * bytes_per_cluster); > continue; /* already copied */ > } > > - trace_backup_do_cow_process(job, start); > + trace_backup_do_cow_process(job, start * bytes_per_cluster); > > n = MIN(sectors_per_cluster, > job->common.len / BDRV_SECTOR_SIZE - > @@ -138,7 +141,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, > bounce_qiov.size, &bounce_qiov, > is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0); > if (ret < 0) { > - trace_backup_do_cow_read_fail(job, start, ret); > + trace_backup_do_cow_read_fail(job, start * bytes_per_cluster, > ret); > if (error_is_read) { > *error_is_read = true; > } > @@ -154,7 +157,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, > job->compress ? BDRV_REQ_WRITE_COMPRESSED : > 0); > } > if (ret < 0) { > - trace_backup_do_cow_write_fail(job, start, ret); > + trace_backup_do_cow_write_fail(job, start * bytes_per_cluster, > ret); > if (error_is_read) { > *error_is_read = false; > } > @@ -177,7 +180,8 @@ out: > > cow_request_end(&cow_request); > > - trace_backup_do_cow_return(job, sector_num, nb_sectors, ret); > + trace_backup_do_cow_return(job, sector_num * BDRV_SECTOR_SIZE, > + nb_sectors * BDRV_SECTOR_SIZE, ret); > > qemu_co_rwlock_unlock(&job->flush_rwlock); > > diff --git a/block/commit.c b/block/commit.c > index 8b684e0..11dab98 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -176,7 +176,8 @@ static void coroutine_fn commit_run(void *opaque) > COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE, > &n); > copy = (ret == 1); > - trace_commit_one_iteration(s, sector_num, n, ret); > + trace_commit_one_iteration(s, sector_num * BDRV_SECTOR_SIZE, > + n * BDRV_SECTOR_SIZE, ret); > if (copy) { > ret = commit_populate(s->top, s->base, sector_num, n, buf); > bytes_written += n * BDRV_SECTOR_SIZE; > diff --git a/block/mirror.c b/block/mirror.c > index a7d0960..d83d482 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -103,7 +103,8 @@ static void mirror_iteration_done(MirrorOp *op, int ret) > int64_t chunk_num; > int i, nb_chunks, sectors_per_chunk; > > - trace_mirror_iteration_done(s, op->sector_num, op->nb_sectors, ret); > + trace_mirror_iteration_done(s, op->sector_num * BDRV_SECTOR_SIZE, > + op->nb_sectors * BDRV_SECTOR_SIZE, ret); > > s->in_flight--; > s->sectors_in_flight -= op->nb_sectors; > @@ -268,7 +269,8 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t > sector_num, > nb_chunks = DIV_ROUND_UP(nb_sectors, sectors_per_chunk); > > while (s->buf_free_count < nb_chunks) { > - trace_mirror_yield_in_flight(s, sector_num, s->in_flight); > + trace_mirror_yield_in_flight(s, sector_num * BDRV_SECTOR_SIZE, > + s->in_flight); > mirror_wait_for_io(s); > } > > @@ -294,7 +296,8 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t > sector_num, > /* Copy the dirty cluster. */ > s->in_flight++; > s->sectors_in_flight += nb_sectors; > - trace_mirror_one_iteration(s, sector_num, nb_sectors); > + trace_mirror_one_iteration(s, sector_num * BDRV_SECTOR_SIZE, > + nb_sectors * BDRV_SECTOR_SIZE); > > blk_aio_preadv(source, sector_num * BDRV_SECTOR_SIZE, &op->qiov, 0, > mirror_read_complete, op); > @@ -346,13 +349,15 @@ static uint64_t coroutine_fn > mirror_iteration(MirrorBlockJob *s) > if (sector_num < 0) { > bdrv_set_dirty_iter(s->dbi, 0); > sector_num = bdrv_dirty_iter_next(s->dbi); > - trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap)); > + trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap) * > + BDRV_SECTOR_SIZE);
mirror_restart_iter(void *s, int64_t cnt) "s %p dirty count %"PRId64 I guess the print doesn't really bother to say what the unit is, just a "dirty count." Does this conflict with the bitmap series, though? (Won't need to scale by BDRV_SECTOR_SIZE after that, I think.) > assert(sector_num >= 0); > } > > first_chunk = sector_num / sectors_per_chunk; > while (test_bit(first_chunk, s->in_flight_bitmap)) { > - trace_mirror_yield_in_flight(s, sector_num, s->in_flight); > + trace_mirror_yield_in_flight(s, sector_num * BDRV_SECTOR_SIZE, > + s->in_flight); > mirror_wait_for_io(s); > } > > @@ -428,7 +433,8 @@ static uint64_t coroutine_fn > mirror_iteration(MirrorBlockJob *s) > } > > while (s->in_flight >= MAX_IN_FLIGHT) { > - trace_mirror_yield_in_flight(s, sector_num, s->in_flight); > + trace_mirror_yield_in_flight(s, sector_num * BDRV_SECTOR_SIZE, > + s->in_flight); > mirror_wait_for_io(s); > } > > @@ -811,7 +817,8 @@ static void coroutine_fn mirror_run(void *opaque) > s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) { > if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 || > (cnt == 0 && s->in_flight > 0)) { > - trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight); > + trace_mirror_yield(s, cnt * BDRV_SECTOR_SIZE, > + s->buf_free_count, s->in_flight); I guess this is another case where the unit wasn't really specified, so we can just change it. > mirror_wait_for_io(s); > continue; > } else if (cnt != 0) { > @@ -852,7 +859,7 @@ static void coroutine_fn mirror_run(void *opaque) > * whether to switch to target check one last time if I/O has > * come in the meanwhile, and if not flush the data to disk. > */ > - trace_mirror_before_drain(s, cnt); > + trace_mirror_before_drain(s, cnt * BDRV_SECTOR_SIZE); > > bdrv_drained_begin(bs); > cnt = bdrv_get_dirty_count(s->dirty_bitmap); > @@ -871,7 +878,8 @@ static void coroutine_fn mirror_run(void *opaque) > } > > ret = 0; > - trace_mirror_before_sleep(s, cnt, s->synced, delay_ns); > + trace_mirror_before_sleep(s, cnt * BDRV_SECTOR_SIZE, > + s->synced, delay_ns); > if (!s->synced) { > block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns); > if (block_job_is_cancelled(&s->common)) { > diff --git a/block/stream.c b/block/stream.c > index 11c5cf1..3134ec5 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -168,7 +168,8 @@ static void coroutine_fn stream_run(void *opaque) > > copy = (ret == 1); > } > - trace_stream_one_iteration(s, sector_num, n, ret); > + trace_stream_one_iteration(s, sector_num * BDRV_SECTOR_SIZE, > + n * BDRV_SECTOR_SIZE, ret); > if (copy) { > ret = stream_populate(blk, sector_num, n, buf); > } > diff --git a/block/trace-events b/block/trace-events > index 0bc5c0a..04f6463 100644 > --- a/block/trace-events > +++ b/block/trace-events > @@ -18,11 +18,11 @@ bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int > count, int flags) "bs %p off > bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, > int64_t cluster_offset, unsigned int cluster_bytes) "bs %p offset %"PRId64" > bytes %u cluster_offset %"PRId64" cluster_bytes %u" > > # block/stream.c > -stream_one_iteration(void *s, int64_t sector_num, int nb_sectors, int > is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d" > +stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int > is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d" > stream_start(void *bs, void *base, void *s) "bs %p base %p s %p" > > # block/commit.c > -commit_one_iteration(void *s, int64_t sector_num, int nb_sectors, int > is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d" > +commit_one_iteration(void *s, int64_t offset, uint64_t bytes, int > is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d" > commit_start(void *bs, void *base, void *top, void *s) "bs %p base %p top %p > s %p" > > # block/mirror.c > @@ -31,14 +31,14 @@ mirror_restart_iter(void *s, int64_t cnt) "s %p dirty > count %"PRId64 > mirror_before_flush(void *s) "s %p" > mirror_before_drain(void *s, int64_t cnt) "s %p dirty count %"PRId64 > mirror_before_sleep(void *s, int64_t cnt, int synced, uint64_t delay_ns) "s > %p dirty count %"PRId64" synced %d delay %"PRIu64"ns" > -mirror_one_iteration(void *s, int64_t sector_num, int nb_sectors) "s %p > sector_num %"PRId64" nb_sectors %d" > -mirror_iteration_done(void *s, int64_t sector_num, int nb_sectors, int ret) > "s %p sector_num %"PRId64" nb_sectors %d ret %d" > +mirror_one_iteration(void *s, int64_t offset, uint64_t bytes) "s %p offset > %" PRId64 " bytes %" PRIu64 > +mirror_iteration_done(void *s, int64_t offset, uint64_t bytes, int ret) "s > %p offset %" PRId64 " bytes %" PRIu64 " ret %d" > mirror_yield(void *s, int64_t cnt, int buf_free_count, int in_flight) "s %p > dirty count %"PRId64" free buffers %d in_flight %d" > -mirror_yield_in_flight(void *s, int64_t sector_num, int in_flight) "s %p > sector_num %"PRId64" in_flight %d" > +mirror_yield_in_flight(void *s, int64_t offset, int in_flight) "s %p offset > %" PRId64 " in_flight %d" > > # block/backup.c > -backup_do_cow_enter(void *job, int64_t start, int64_t sector_num, int > nb_sectors) "job %p start %"PRId64" sector_num %"PRId64" nb_sectors %d" > -backup_do_cow_return(void *job, int64_t sector_num, int nb_sectors, int ret) > "job %p sector_num %"PRId64" nb_sectors %d ret %d" > +backup_do_cow_enter(void *job, int64_t start, int64_t offset, uint64_t > bytes) "job %p start %" PRId64 " offset %" PRId64 " bytes %" PRIu64 > +backup_do_cow_return(void *job, int64_t offset, uint64_t bytes, int ret) > "job %p offset %" PRId64 " bytes %" PRIu64 " ret %d" > backup_do_cow_skip(void *job, int64_t start) "job %p start %"PRId64 > backup_do_cow_process(void *job, int64_t start) "job %p start %"PRId64 > backup_do_cow_read_fail(void *job, int64_t start, int ret) "job %p start > %"PRId64" ret %d" I guess these three were "cluster" based, but you didn't need to change anything to assert them as byte-based. > Under the assumption that it's okay to change tracing output without being able to differentiate between new and old output: Reviewed-by: John Snow <js...@redhat.com>