Re: [Qemu-devel] [Qemu-block] [PATCH 02/17] trace: Show blockjob actions via bytes, not sectors

[John Snow]

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 <address@hidden>
> ---
>  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 <address@hidden>