Re: [Qemu-block] [PATCH v2] tests: Avoid non-portable 'echo -ARG'

[Jeff Cody]

On Fri, Jun 30, 2017 at 02:58:31PM -0500, Eric Blake wrote:
> POSIX says that backslashes in the arguments to 'echo', as well as
> any use of 'echo -n' and 'echo -e', are non-portable; it recommends
> people should favor 'printf' instead.  This is definitely true where
> we do not control which shell is running (such as in makefile snippets
> or in documentation examples).  But even for scripts where we
> require bash (and therefore, where echo does what we want by default),
> it is still possible to use 'shopt -s xpg_echo' to change bash's
> behavior of echo.  And setting a good example never hurts when we are
> not sure if a snippet will be copied from a bash-only script to a
> general shell script (although I don't change the use of non-portable
> \e for ESC when we know the running shell is bash).
> 
> Replace 'echo -n "..."' with 'printf %s "..."', and 'echo -e "..."'
> with 'printf %b "...\n"', with the optimization that the %s/%b
> argument can be omitted if the string being printed contains no
> substitutions that could result in '%' (trivial if there is no
> $ or `, but also possible when using things like $ret that are
> known to be numeric given the assignment to ret just above).
> 
> In the qemu-iotests check script, fix unusual shell quoting
> that would result in word-splitting if 'date' outputs a space.
> 
> In test 051, take an opportunity to shorten the line.
> 
> In test 068, get rid of a pointless second invocation of bash.
> 
> CC: qemu-triv...@nongnu.org
> Signed-off-by: Eric Blake 
> 
> ---
> v2: be robust to potential % in substitutions [Max], rebase to master,
> shorten some long lines and an odd bash -c use
> 
> Add qemu-trivial in cc, although we may decide this is better through
> Max's block tree since it is mostly iotests related
> 
> ---
>  qemu-options.hx |  4 ++--
>  tests/multiboot/run_test.sh | 10 +-
>  tests/qemu-iotests/051  |  7 ---
>  tests/qemu-iotests/068  |  2 +-
>  tests/qemu-iotests/142  | 48 
> ++---
>  tests/qemu-iotests/171  | 14 ++---
>  tests/qemu-iotests/check| 18 -
>  tests/rocker/all| 10 +-
>  tests/tcg/cris/Makefile |  8 
>  9 files changed, 61 insertions(+), 60 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 297bd8a..05a0474 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4363,7 +4363,7 @@ The simplest (insecure) usage is to provide the secret 
> inline
> 
>  The simplest secure usage is to provide the secret via a file
> 
> - # echo -n "letmein" > mypasswd.txt
> + # printf "letmein" > mypasswd.txt
>   # $QEMU -object secret,id=sec0,file=mypasswd.txt,format=raw
> 
>  For greater security, AES-256-CBC should be used. To illustrate usage,
> @@ -4391,7 +4391,7 @@ telling openssl to base64 encode the result, but it 
> could be left
>  as raw bytes if desired.
> 
>  @example
> - # SECRET=$(echo -n "letmein" |
> + # SECRET=$(printf "letmein" |
>  openssl enc -aes-256-cbc -a -K $KEY -iv $IV)
>  @end example
> 
> diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
> index 78d7edf..9a1cd59 100755
> --- a/tests/multiboot/run_test.sh
> +++ b/tests/multiboot/run_test.sh
> @@ -26,7 +26,7 @@ run_qemu() {
>  local kernel=$1
>  shift
> 
> -echo -e "\n\n=== Running test case: $kernel $@ ===\n" >> test.log
> +printf %b "\n\n=== Running test case: $kernel $@ ===\n\n" >> test.log
> 
>  $QEMU \
>  -kernel $kernel \
> @@ -68,21 +68,21 @@ for t in mmap modules; do
>  pass=1
> 
>  if [ $debugexit != 1 ]; then
> -echo -e "\e[31m ?? \e[0m $t (no debugexit used, exit code $ret)"
> +printf "\e[31m ?? \e[0m $t (no debugexit used, exit code $ret)\n"
>  pass=0
>  elif [ $ret != 0 ]; then
> -echo -e "\e[31mFAIL\e[0m $t (exit code $ret)"
> +printf "\e[31mFAIL\e[0m $t (exit code $ret)\n"
>  pass=0
>  fi
> 
>  if ! diff $t.out test.log > /dev/null 2>&1; then
> -echo -e "\e[31mFAIL\e[0m $t (output difference)"
> +printf "\e[31mFAIL\e[0m $t (output difference)\n"
>  diff -u $t.out test.log
>  pass=0
>  fi
> 
>  if [ $pass == 1 ]; then
> -echo -e "\e[32mPASS\e[0m $t"
> +printf "\e[32mPASS\e[0m $t\n"
>  fi
> 
>  done
> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> index 26c29de..c3c08bf 100755
> --- a/tests/qemu-iotests/051
> +++ b/tests/qemu-iotests/051
> @@ -217,7 +217,7 @@ run_qemu -drive driver=null-co,cache=invalid_value
>  # Test 142 checks the direct=on cases
> 
>  for cache in writeback writethrough unsafe invalid_value; do
> -echo -e "info block\ninfo block file\ninfo block backing\ninfo block 
> backing-file" | \
> +printf "info block %s\n" '' file backing backing-file | \
>  run_qemu -drive 
> 

Re: [Qemu-block] [PATCH v4 1/2] iotests: Use absolute paths for executables

[Jeff Cody]

On Fri, Jun 30, 2017 at 02:45:46PM -0500, Eric Blake wrote:
> On 06/30/2017 02:41 PM, Eric Blake wrote:
> 
> > +++ 068.out.bad 2017-06-30 14:35:28.720241398 -0500
> > @@ -1,4 +1,5 @@
> >  QA output created by 068
> > +realpath: '': No such file or directory
> > 
> > The culprit? $QEMU_VXHS_PROG is empty for me, which means `set_prog_path
> > qnio_server` found nothing to use.  You'll have to add in a safety valve
> > that only calls 'type' if operating on a non-empty path in the first place.
> 
> I'm using this locally, in the meantime:
> 
> diff --git i/tests/qemu-iotests/common.config
> w/tests/qemu-iotests/common.config
> index c1dc425..6f97331 100644
> --- i/tests/qemu-iotests/common.config
> +++ w/tests/qemu-iotests/common.config
> @@ -107,7 +107,9 @@ export QEMU_PROG=$(realpath -- "$(type -p
> "$QEMU_PROG")")
>  export QEMU_IMG_PROG=$(realpath -- "$(type -p "$QEMU_IMG_PROG")")
>  export QEMU_IO_PROG=$(realpath -- "$(type -p "$QEMU_IO_PROG")")
>  export QEMU_NBD_PROG=$(realpath -- "$(type -p "$QEMU_NBD_PROG")")
> -export QEMU_VXHS_PROG=$(realpath -- "$(type -p "$QEMU_VXHS_PROG")")
> +if [ -n "$QEMU_VXHS_PROG" ]; then
> +export QEMU_VXHS_PROG=$(realpath -- "$(type -p "$QEMU_VXHS_PROG")")
> +fi
> 
>  _qemu_wrapper()
>  {
> 
> 
> Is qnio_server easily available for Fedora?
>

Depends on your definition of "easily".  It needs to be built from the
upstream project github [1].  Most developers will likely not have it
installed.

https://github.com/VeritasHyperScale/libqnio

Re: [Qemu-block] [PATCH v3 20/20] block: Make bdrv_is_allocated_above() byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:58PM -0500, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  In the common case, allocation is unlikely to ever use
> values that are not naturally sector-aligned, but it is possible
> that byte-based values will let us be more precise about allocation
> at the end of an unaligned file that can do byte-based access.
> 
> Changing the signature of the function to use int64_t *pnum ensures
> that the compiler enforces that all callers are updated.  For now,
> the io.c layer still assert()s that all callers are sector-aligned,
> but that can be relaxed when a later patch implements byte-based
> block status.  Therefore, for the most part this patch is just the
> addition of scaling at the callers followed by inverse scaling at
> bdrv_is_allocated().  But some code, particularly stream_run(),
> gets a lot simpler because it no longer has to mess with sectors.
> 
> For ease of review, bdrv_is_allocated() was tackled separately.
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: tweak function comments, favor bdrv_getlength() over ->total_sectors
> ---
>  include/block/block.h |  2 +-
>  block/commit.c| 20 
>  block/io.c| 42 --
>  block/mirror.c|  5 -
>  block/replication.c   | 17 -
>  block/stream.c| 21 +
>  qemu-img.c| 10 +++---
>  7 files changed, 61 insertions(+), 56 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 9b9d87b..13022d5 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -428,7 +428,7 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,
>  int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
>int64_t *pnum);
>  int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> -int64_t sector_num, int nb_sectors, int *pnum);
> +int64_t offset, int64_t bytes, int64_t *pnum);
> 
>  bool bdrv_is_read_only(BlockDriverState *bs);
>  bool bdrv_is_writable(BlockDriverState *bs);
> diff --git a/block/commit.c b/block/commit.c
> index 241aa95..774a8a5 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -146,7 +146,7 @@ static void coroutine_fn commit_run(void *opaque)
>  int64_t offset;
>  uint64_t delay_ns = 0;
>  int ret = 0;
> -int n = 0; /* sectors */
> +int64_t n = 0; /* bytes */
>  void *buf = NULL;
>  int bytes_written = 0;
>  int64_t base_len;
> @@ -171,7 +171,7 @@ static void coroutine_fn commit_run(void *opaque)
> 
>  buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
> 
> -for (offset = 0; offset < s->common.len; offset += n * BDRV_SECTOR_SIZE) 
> {
> +for (offset = 0; offset < s->common.len; offset += n) {
>  bool copy;
> 
>  /* Note that even when no rate limit is applied we need to yield
> @@ -183,15 +183,12 @@ static void coroutine_fn commit_run(void *opaque)
>  }
>  /* Copy if allocated above the base */
>  ret = bdrv_is_allocated_above(blk_bs(s->top), blk_bs(s->base),
> -  offset / BDRV_SECTOR_SIZE,
> -  COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
> -  );
> +  offset, COMMIT_BUFFER_SIZE, );
>  copy = (ret == 1);
> -trace_commit_one_iteration(s, offset, n * BDRV_SECTOR_SIZE, ret);
> +trace_commit_one_iteration(s, offset, n, ret);
>  if (copy) {
> -ret = commit_populate(s->top, s->base, offset,
> -  n * BDRV_SECTOR_SIZE, buf);
> -bytes_written += n * BDRV_SECTOR_SIZE;
> +ret = commit_populate(s->top, s->base, offset, n, buf);
> +bytes_written += n;
>  }
>  if (ret < 0) {
>  BlockErrorAction action =
> @@ -204,11 +201,10 @@ static void coroutine_fn commit_run(void *opaque)
>  }
>  }
>  /* Publish progress */
> -s->common.offset += n * BDRV_SECTOR_SIZE;
> +s->common.offset += n;
> 
>  if (copy && s->common.speed) {
> -delay_ns = ratelimit_calculate_delay(>limit,
> - n * BDRV_SECTOR_SIZE);
> +delay_ns = ratelimit_calculate_delay(>limit, n);
>  }
>  }
> 
> diff --git a/block/io.c b/block/io.c
> index 5bbf153..061a162 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1903,54 +1903,52 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState 
> *bs, int64_t offset,
>  /*
>   * Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP]
>   *
> - * Return true if the given sector is 

Re: [Qemu-block] [PATCH v3 19/20] block: Minimize raw use of bds->total_sectors

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:57PM -0500, Eric Blake wrote:
> bdrv_is_allocated_above() was relying on intermediate->total_sectors,
> which is a field that can have stale contents depending on the value
> of intermediate->has_variable_length.  An audit shows that we are safe
> (we were first calling through bdrv_co_get_block_status() which in
> turn calls bdrv_nb_sectors() and therefore just refreshed the current
> length), but it's nicer to favor our accessor functions to avoid having
> to repeat such an audit, even if it means refresh_total_sectors() is
> called more frequently.
> 
> Suggested-by: John Snow 
> Signed-off-by: Eric Blake 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: new patch
> ---
>  block/io.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 0545180..5bbf153 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1924,6 +1924,7 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>  intermediate = top;
>  while (intermediate && intermediate != base) {
>  int64_t pnum_inter;
> +int64_t size_inter;
>  int psectors_inter;
> 
>  ret = bdrv_is_allocated(intermediate, sector_num * BDRV_SECTOR_SIZE,
> @@ -1941,13 +1942,14 @@ int bdrv_is_allocated_above(BlockDriverState *top,
> 
>  /*
>   * [sector_num, nb_sectors] is unallocated on top but intermediate
> - * might have
> - *
> - * [sector_num+x, nr_sectors] allocated.
> + * might have [sector_num+x, nb_sectors-x] allocated.
>   */
> +size_inter = bdrv_nb_sectors(intermediate);
> +if (size_inter < 0) {
> +return size_inter;
> +}
>  if (n > psectors_inter &&
> -(intermediate == top ||
> - sector_num + psectors_inter < intermediate->total_sectors)) {
> +(intermediate == top || sector_num + psectors_inter < 
> size_inter)) {
>  n = psectors_inter;
>  }
> 
> -- 
> 2.9.4
> 
> 

Re: [Qemu-block] [PATCH v3 18/20] block: Make bdrv_is_allocated() byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:56PM -0500, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  In the common case, allocation is unlikely to ever use
> values that are not naturally sector-aligned, but it is possible
> that byte-based values will let us be more precise about allocation
> at the end of an unaligned file that can do byte-based access.
> 
> Changing the signature of the function to use int64_t *pnum ensures
> that the compiler enforces that all callers are updated.  For now,
> the io.c layer still assert()s that all callers are sector-aligned
> on input and that *pnum is sector-aligned on return to the caller,
> but that can be relaxed when a later patch implements byte-based
> block status.  Therefore, this code adds usages like
> DIV_ROUND_UP(,BDRV_SECTOR_SIZE) to callers that still want aligned
> values, where the call might reasonbly give non-aligned results
> in the future; on the other hand, no rounding is needed for callers
> that should just continue to work with byte alignment.
> 
> For the most part this patch is just the addition of scaling at the
> callers followed by inverse scaling at bdrv_is_allocated().  But
> some code, particularly bdrv_commit(), gets a lot simpler because it
> no longer has to mess with sectors; also, it is now possible to pass
> NULL if the caller does not care how much of the image is allocated
> beyond the initial offset.
> 
> For ease of review, bdrv_is_allocated_above() will be tackled
> separately.
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: rebase to earlier changes, tweak commit message
> ---
>  include/block/block.h |  4 +--
>  block/backup.c| 17 -
>  block/commit.c| 21 +++-
>  block/io.c| 49 +---
>  block/stream.c|  5 ++--
>  block/vvfat.c | 34 ++---
>  migration/block.c |  9 ---
>  qemu-img.c|  5 +++-
>  qemu-io-cmds.c| 70 
> +++
>  9 files changed, 114 insertions(+), 100 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 5cdd690..9b9d87b 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -425,8 +425,8 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,
>  int64_t sector_num,
>  int nb_sectors, int *pnum,
>  BlockDriverState **file);
> -int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int 
> nb_sectors,
> -  int *pnum);
> +int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
> +  int64_t *pnum);
>  int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
>  int64_t sector_num, int nb_sectors, int *pnum);
> 
> diff --git a/block/backup.c b/block/backup.c
> index 04def91..b2048bf 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -47,12 +47,6 @@ typedef struct BackupBlockJob {
>  QLIST_HEAD(, CowRequest) inflight_reqs;
>  } BackupBlockJob;
> 
> -/* Size of a cluster in sectors, instead of bytes. */
> -static inline int64_t cluster_size_sectors(BackupBlockJob *job)
> -{
> -  return job->cluster_size / BDRV_SECTOR_SIZE;
> -}
> -
>  /* See if in-flight requests overlap and wait for them to complete */
>  static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
> int64_t offset,
> @@ -433,7 +427,6 @@ static void coroutine_fn backup_run(void *opaque)
>  BackupCompleteData *data;
>  BlockDriverState *bs = blk_bs(job->common.blk);
>  int64_t offset;
> -int64_t sectors_per_cluster = cluster_size_sectors(job);
>  int ret = 0;
> 
>  QLIST_INIT(>inflight_reqs);
> @@ -465,12 +458,13 @@ static void coroutine_fn backup_run(void *opaque)
>  }
> 
>  if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
> -int i, n;
> +int i;
> +int64_t n;
> 
>  /* Check to see if these blocks are already in the
>   * backing file. */
> 
> -for (i = 0; i < sectors_per_cluster;) {
> +for (i = 0; i < job->cluster_size;) {
>  /* bdrv_is_allocated() only returns true/false based
>   * on the first set of sectors it comes across that
>   * are are all in the same state.
> @@ -478,9 +472,8 @@ static void coroutine_fn backup_run(void *opaque)
>   * backup cluster length.  We end up copying more than
>   * needed but at some point that is always the case. */
>  alloced =
> -

Re: [Qemu-block] [PATCH v3 17/20] backup: Switch backup_run() to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:55PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Change the internal
> loop iteration of backups to track by bytes instead of sectors
> (although we are still guaranteed that we iterate by steps that
> are cluster-aligned).
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: no change
> ---
>  block/backup.c | 32 +++-
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index c029d44..04def91 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -370,11 +370,10 @@ static int coroutine_fn 
> backup_run_incremental(BackupBlockJob *job)
>  int ret = 0;
>  int clusters_per_iter;
>  uint32_t granularity;
> -int64_t sector;
> +int64_t offset;
>  int64_t cluster;
>  int64_t end;
>  int64_t last_cluster = -1;
> -int64_t sectors_per_cluster = cluster_size_sectors(job);
>  BdrvDirtyBitmapIter *dbi;
> 
>  granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
> @@ -382,8 +381,8 @@ static int coroutine_fn 
> backup_run_incremental(BackupBlockJob *job)
>  dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
> 
>  /* Find the next dirty sector(s) */
> -while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
> -cluster = sector / sectors_per_cluster;
> +while ((offset = bdrv_dirty_iter_next(dbi) * BDRV_SECTOR_SIZE) >= 0) {
> +cluster = offset / job->cluster_size;
> 
>  /* Fake progress updates for any clusters we skipped */
>  if (cluster != last_cluster + 1) {
> @@ -410,7 +409,8 @@ static int coroutine_fn 
> backup_run_incremental(BackupBlockJob *job)
>  /* If the bitmap granularity is smaller than the backup granularity,
>   * we need to advance the iterator pointer to the next cluster. */
>  if (granularity < job->cluster_size) {
> -bdrv_set_dirty_iter(dbi, cluster * sectors_per_cluster);
> +bdrv_set_dirty_iter(dbi,
> +cluster * job->cluster_size / 
> BDRV_SECTOR_SIZE);
>  }
> 
>  last_cluster = cluster - 1;
> @@ -432,17 +432,15 @@ static void coroutine_fn backup_run(void *opaque)
>  BackupBlockJob *job = opaque;
>  BackupCompleteData *data;
>  BlockDriverState *bs = blk_bs(job->common.blk);
> -int64_t start, end;
> +int64_t offset;
>  int64_t sectors_per_cluster = cluster_size_sectors(job);
>  int ret = 0;
> 
>  QLIST_INIT(>inflight_reqs);
>  qemu_co_rwlock_init(>flush_rwlock);
> 
> -start = 0;
> -end = DIV_ROUND_UP(job->common.len, job->cluster_size);
> -
> -job->done_bitmap = bitmap_new(end);
> +job->done_bitmap = bitmap_new(DIV_ROUND_UP(job->common.len,
> +   job->cluster_size));
> 
>  job->before_write.notify = backup_before_write_notify;
>  bdrv_add_before_write_notifier(bs, >before_write);
> @@ -457,7 +455,8 @@ static void coroutine_fn backup_run(void *opaque)
>  ret = backup_run_incremental(job);
>  } else {
>  /* Both FULL and TOP SYNC_MODE's require copying.. */
> -for (; start < end; start++) {
> +for (offset = 0; offset < job->common.len;
> + offset += job->cluster_size) {
>  bool error_is_read;
>  int alloced = 0;
> 
> @@ -480,8 +479,8 @@ static void coroutine_fn backup_run(void *opaque)
>   * needed but at some point that is always the case. */
>  alloced =
>  bdrv_is_allocated(bs,
> -start * sectors_per_cluster + i,
> -sectors_per_cluster - i, );
> +  (offset >> BDRV_SECTOR_BITS) + i,
> +  sectors_per_cluster - i, );
>  i += n;
> 
>  if (alloced || n == 0) {
> @@ -499,9 +498,8 @@ static void coroutine_fn backup_run(void *opaque)
>  if (alloced < 0) {
>  ret = alloced;
>  } else {
> -ret = backup_do_cow(job, start * job->cluster_size,
> -job->cluster_size, _is_read,
> -false);
> +ret = backup_do_cow(job, offset, job->cluster_size,
> +_is_read, false);
>  }
>  if (ret < 0) {
>  /* Depending on error action, fail now or retry cluster */
> @@ -510,7 +508,7 @@ static void coroutine_fn backup_run(void *opaque)
>  if (action == BLOCK_ERROR_ACTION_REPORT) {
>  break;
>  } else {
> -start--;
> +offset 

Re: [Qemu-block] [PATCH v3 16/20] backup: Switch backup_do_cow() to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:54PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Convert another internal
> function (no semantic change).
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: no change
> ---
>  block/backup.c | 62 
> --
>  1 file changed, 26 insertions(+), 36 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index cfbd921..c029d44 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -91,7 +91,7 @@ static void cow_request_end(CowRequest *req)
>  }
> 
>  static int coroutine_fn backup_do_cow(BackupBlockJob *job,
> -  int64_t sector_num, int nb_sectors,
> +  int64_t offset, uint64_t bytes,
>bool *error_is_read,
>bool is_write_notifier)
>  {
> @@ -101,34 +101,28 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>  QEMUIOVector bounce_qiov;
>  void *bounce_buffer = NULL;
>  int ret = 0;
> -int64_t sectors_per_cluster = cluster_size_sectors(job);
> -int64_t start, end; /* clusters */
> +int64_t start, end; /* bytes */
>  int n; /* bytes */
> 
>  qemu_co_rwlock_rdlock(>flush_rwlock);
> 
> -start = sector_num / sectors_per_cluster;
> -end = DIV_ROUND_UP(sector_num + nb_sectors, sectors_per_cluster);
> +start = QEMU_ALIGN_DOWN(offset, job->cluster_size);
> +end = QEMU_ALIGN_UP(bytes + offset, job->cluster_size);
> 
> -trace_backup_do_cow_enter(job, start * job->cluster_size,
> -  sector_num * BDRV_SECTOR_SIZE,
> -  nb_sectors * BDRV_SECTOR_SIZE);
> +trace_backup_do_cow_enter(job, start, offset, bytes);
> 
> -wait_for_overlapping_requests(job, start * job->cluster_size,
> -  end * job->cluster_size);
> -cow_request_begin(_request, job, start * job->cluster_size,
> -  end * job->cluster_size);
> +wait_for_overlapping_requests(job, start, end);
> +cow_request_begin(_request, job, start, end);
> 
> -for (; start < end; start++) {
> -if (test_bit(start, job->done_bitmap)) {
> -trace_backup_do_cow_skip(job, start * job->cluster_size);
> +for (; start < end; start += job->cluster_size) {
> +if (test_bit(start / job->cluster_size, job->done_bitmap)) {
> +trace_backup_do_cow_skip(job, start);
>  continue; /* already copied */
>  }
> 
> -trace_backup_do_cow_process(job, start * job->cluster_size);
> +trace_backup_do_cow_process(job, start);
> 
> -n = MIN(job->cluster_size,
> -job->common.len - start * job->cluster_size);
> +n = MIN(job->cluster_size, job->common.len - start);
> 
>  if (!bounce_buffer) {
>  bounce_buffer = blk_blockalign(blk, job->cluster_size);
> @@ -137,11 +131,10 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>  iov.iov_len = n;
>  qemu_iovec_init_external(_qiov, , 1);
> 
> -ret = blk_co_preadv(blk, start * job->cluster_size,
> -bounce_qiov.size, _qiov,
> +ret = blk_co_preadv(blk, start, bounce_qiov.size, _qiov,
>  is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0);
>  if (ret < 0) {
> -trace_backup_do_cow_read_fail(job, start * job->cluster_size, 
> ret);
> +trace_backup_do_cow_read_fail(job, start, ret);
>  if (error_is_read) {
>  *error_is_read = true;
>  }
> @@ -149,22 +142,22 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>  }
> 
>  if (buffer_is_zero(iov.iov_base, iov.iov_len)) {
> -ret = blk_co_pwrite_zeroes(job->target, start * 
> job->cluster_size,
> +ret = blk_co_pwrite_zeroes(job->target, start,
> bounce_qiov.size, BDRV_REQ_MAY_UNMAP);
>  } else {
> -ret = blk_co_pwritev(job->target, start * job->cluster_size,
> +ret = blk_co_pwritev(job->target, start,
>   bounce_qiov.size, _qiov,
>   job->compress ? BDRV_REQ_WRITE_COMPRESSED : 
> 0);
>  }
>  if (ret < 0) {
> -trace_backup_do_cow_write_fail(job, start * job->cluster_size, 
> ret);
> +trace_backup_do_cow_write_fail(job, start, ret);
>  if (error_is_read) {
>  *error_is_read = false;
>  }
>  goto out;
>  }
> 
> -set_bit(start, job->done_bitmap);
> +set_bit(start / job->cluster_size, job->done_bitmap);
> 
>  

Re: [Qemu-block] [PATCH v3 15/20] backup: Switch block_backup.h to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:53PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Continue by converting
> the public interface to backup jobs (no semantic change), including
> a change to CowRequest to track by bytes instead of cluster indices.
> 
> Note that this does not change the difference between the public
> interface (starting point, and size of the subsequent range) and
> the internal interface (starting and end points).
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: change a couple more parameter names
> ---
>  include/block/block_backup.h | 11 +--
>  block/backup.c   | 33 -
>  block/replication.c  | 12 
>  3 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/include/block/block_backup.h b/include/block/block_backup.h
> index 8a75947..994a3bd 100644
> --- a/include/block/block_backup.h
> +++ b/include/block/block_backup.h
> @@ -21,17 +21,16 @@
>  #include "block/block_int.h"
> 
>  typedef struct CowRequest {
> -int64_t start;
> -int64_t end;
> +int64_t start_byte;
> +int64_t end_byte;
>  QLIST_ENTRY(CowRequest) list;
>  CoQueue wait_queue; /* coroutines blocked on this request */
>  } CowRequest;
> 
> -void backup_wait_for_overlapping_requests(BlockJob *job, int64_t sector_num,
> -  int nb_sectors);
> +void backup_wait_for_overlapping_requests(BlockJob *job, int64_t offset,
> +  uint64_t bytes);
>  void backup_cow_request_begin(CowRequest *req, BlockJob *job,
> -  int64_t sector_num,
> -  int nb_sectors);
> +  int64_t offset, uint64_t bytes);
>  void backup_cow_request_end(CowRequest *req);
> 
>  void backup_do_checkpoint(BlockJob *job, Error **errp);
> diff --git a/block/backup.c b/block/backup.c
> index 4e64710..cfbd921 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -55,7 +55,7 @@ static inline int64_t cluster_size_sectors(BackupBlockJob 
> *job)
> 
>  /* See if in-flight requests overlap and wait for them to complete */
>  static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
> -   int64_t start,
> +   int64_t offset,
> int64_t end)
>  {
>  CowRequest *req;
> @@ -64,7 +64,7 @@ static void coroutine_fn 
> wait_for_overlapping_requests(BackupBlockJob *job,
>  do {
>  retry = false;
>  QLIST_FOREACH(req, >inflight_reqs, list) {
> -if (end > req->start && start < req->end) {
> +if (end > req->start_byte && offset < req->end_byte) {
>  qemu_co_queue_wait(>wait_queue, NULL);
>  retry = true;
>  break;
> @@ -75,10 +75,10 @@ static void coroutine_fn 
> wait_for_overlapping_requests(BackupBlockJob *job,
> 
>  /* Keep track of an in-flight request */
>  static void cow_request_begin(CowRequest *req, BackupBlockJob *job,
> - int64_t start, int64_t end)
> +  int64_t offset, int64_t end)
>  {
> -req->start = start;
> -req->end = end;
> +req->start_byte = offset;
> +req->end_byte = end;
>  qemu_co_queue_init(>wait_queue);
>  QLIST_INSERT_HEAD(>inflight_reqs, req, list);
>  }
> @@ -114,8 +114,10 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>sector_num * BDRV_SECTOR_SIZE,
>nb_sectors * BDRV_SECTOR_SIZE);
> 
> -wait_for_overlapping_requests(job, start, end);
> -cow_request_begin(_request, job, start, end);
> +wait_for_overlapping_requests(job, start * job->cluster_size,
> +  end * job->cluster_size);
> +cow_request_begin(_request, job, start * job->cluster_size,
> +  end * job->cluster_size);
> 
>  for (; start < end; start++) {
>  if (test_bit(start, job->done_bitmap)) {
> @@ -277,32 +279,29 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
>  bitmap_zero(backup_job->done_bitmap, len);
>  }
> 
> -void backup_wait_for_overlapping_requests(BlockJob *job, int64_t sector_num,
> -  int nb_sectors)
> +void backup_wait_for_overlapping_requests(BlockJob *job, int64_t offset,
> +  uint64_t bytes)
>  {
>  BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common);
> -int64_t sectors_per_cluster = cluster_size_sectors(backup_job);
>  int64_t start, end;
> 
>  assert(job->driver->job_type == 

Re: [Qemu-block] [PATCH v3 14/20] backup: Switch BackupBlockJob to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:52PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Continue by converting an
> internal structure (no semantic change), and all references to
> tracking progress.  Drop a redundant local variable bytes_per_cluster.
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: no change
> ---
>  block/backup.c | 33 +++--
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 06431ac..4e64710 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -39,7 +39,7 @@ typedef struct BackupBlockJob {
>  BlockdevOnError on_source_error;
>  BlockdevOnError on_target_error;
>  CoRwlock flush_rwlock;
> -uint64_t sectors_read;
> +uint64_t bytes_read;
>  unsigned long *done_bitmap;
>  int64_t cluster_size;
>  bool compress;
> @@ -102,16 +102,15 @@ 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;
> +int64_t start, end; /* clusters */
> +int n; /* bytes */
> 
>  qemu_co_rwlock_rdlock(>flush_rwlock);
> 
>  start = sector_num / sectors_per_cluster;
>  end = DIV_ROUND_UP(sector_num + nb_sectors, sectors_per_cluster);
> 
> -trace_backup_do_cow_enter(job, start * bytes_per_cluster,
> +trace_backup_do_cow_enter(job, start * job->cluster_size,
>sector_num * BDRV_SECTOR_SIZE,
>nb_sectors * BDRV_SECTOR_SIZE);
> 
> @@ -120,28 +119,27 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
> 
>  for (; start < end; start++) {
>  if (test_bit(start, job->done_bitmap)) {
> -trace_backup_do_cow_skip(job, start * bytes_per_cluster);
> +trace_backup_do_cow_skip(job, start * job->cluster_size);
>  continue; /* already copied */
>  }
> 
> -trace_backup_do_cow_process(job, start * bytes_per_cluster);
> +trace_backup_do_cow_process(job, start * job->cluster_size);
> 
> -n = MIN(sectors_per_cluster,
> -job->common.len / BDRV_SECTOR_SIZE -
> -start * sectors_per_cluster);
> +n = MIN(job->cluster_size,
> +job->common.len - start * job->cluster_size);
> 
>  if (!bounce_buffer) {
>  bounce_buffer = blk_blockalign(blk, job->cluster_size);
>  }
>  iov.iov_base = bounce_buffer;
> -iov.iov_len = n * BDRV_SECTOR_SIZE;
> +iov.iov_len = n;
>  qemu_iovec_init_external(_qiov, , 1);
> 
>  ret = blk_co_preadv(blk, start * job->cluster_size,
>  bounce_qiov.size, _qiov,
>  is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0);
>  if (ret < 0) {
> -trace_backup_do_cow_read_fail(job, start * bytes_per_cluster, 
> ret);
> +trace_backup_do_cow_read_fail(job, start * job->cluster_size, 
> ret);
>  if (error_is_read) {
>  *error_is_read = true;
>  }
> @@ -157,7 +155,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 * bytes_per_cluster, 
> ret);
> +trace_backup_do_cow_write_fail(job, start * job->cluster_size, 
> ret);
>  if (error_is_read) {
>  *error_is_read = false;
>  }
> @@ -169,8 +167,8 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>  /* Publish progress, guest I/O counts as progress too.  Note that the
>   * offset field is an opaque progress value, it is not a disk offset.
>   */
> -job->sectors_read += n;
> -job->common.offset += n * BDRV_SECTOR_SIZE;
> +job->bytes_read += n;
> +job->common.offset += n;
>  }
> 
>  out:
> @@ -363,9 +361,8 @@ static bool coroutine_fn yield_and_check(BackupBlockJob 
> *job)
>   */
>  if (job->common.speed) {
>  uint64_t delay_ns = ratelimit_calculate_delay(>limit,
> -  job->sectors_read *
> -  BDRV_SECTOR_SIZE);
> -job->sectors_read = 0;
> +  job->bytes_read);
> +job->bytes_read = 0;
>  block_job_sleep_ns(>common, QEMU_CLOCK_REALTIME, delay_ns);
>  } else {
>  block_job_sleep_ns(>common, QEMU_CLOCK_REALTIME, 0);
> 

Re: [Qemu-block] [PATCH v3 13/20] block: Drop unused bdrv_round_sectors_to_clusters()

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:51PM -0500, Eric Blake wrote:
> Now that the last user [mirror_iteration()] has converted to using
> bytes, we no longer need a function to round sectors to clusters.
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: hoist to earlier series, no change
> ---
>  include/block/block.h |  4 
>  block/io.c| 21 -
>  2 files changed, 25 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 3e91cac..5cdd690 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -473,10 +473,6 @@ const char *bdrv_get_device_or_node_name(const 
> BlockDriverState *bs);
>  int bdrv_get_flags(BlockDriverState *bs);
>  int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
>  ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
> -void bdrv_round_sectors_to_clusters(BlockDriverState *bs,
> -int64_t sector_num, int nb_sectors,
> -int64_t *cluster_sector_num,
> -int *cluster_nb_sectors);
>  void bdrv_round_to_clusters(BlockDriverState *bs,
>  int64_t offset, unsigned int bytes,
>  int64_t *cluster_offset,
> diff --git a/block/io.c b/block/io.c
> index c72d701..d9fec1f 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -419,27 +419,6 @@ static void mark_request_serialising(BdrvTrackedRequest 
> *req, uint64_t align)
>  }
> 
>  /**
> - * Round a region to cluster boundaries (sector-based)
> - */
> -void bdrv_round_sectors_to_clusters(BlockDriverState *bs,
> -int64_t sector_num, int nb_sectors,
> -int64_t *cluster_sector_num,
> -int *cluster_nb_sectors)
> -{
> -BlockDriverInfo bdi;
> -
> -if (bdrv_get_info(bs, ) < 0 || bdi.cluster_size == 0) {
> -*cluster_sector_num = sector_num;
> -*cluster_nb_sectors = nb_sectors;
> -} else {
> -int64_t c = bdi.cluster_size / BDRV_SECTOR_SIZE;
> -*cluster_sector_num = QEMU_ALIGN_DOWN(sector_num, c);
> -*cluster_nb_sectors = QEMU_ALIGN_UP(sector_num - *cluster_sector_num 
> +
> -nb_sectors, c);
> -}
> -}
> -
> -/**
>   * Round a region to cluster boundaries
>   */
>  void bdrv_round_to_clusters(BlockDriverState *bs,
> -- 
> 2.9.4
> 
> 

Re: [Qemu-block] [PATCH v3 12/20] mirror: Switch mirror_iteration() to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:50PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Change the internal
> loop iteration of mirroring to track by bytes instead of sectors
> (although we are still guaranteed that we iterate by steps that
> are both sector-aligned and multiples of the granularity).  Drop
> the now-unused mirror_clip_sectors().
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v3: rebase to Paolo's thread-safety changes, R-b kept
> v2: straightforward rebase to earlier mirror_clip_bytes() change, R-b kept
> ---
>  block/mirror.c | 105 
> +
>  1 file changed, 46 insertions(+), 59 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 81ff784..0eb2af4 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -184,15 +184,6 @@ static inline int64_t mirror_clip_bytes(MirrorBlockJob 
> *s,
>  return MIN(bytes, s->bdev_length - offset);
>  }
> 
> -/* Clip nb_sectors relative to sector_num to not exceed end-of-file */
> -static inline int mirror_clip_sectors(MirrorBlockJob *s,
> -  int64_t sector_num,
> -  int nb_sectors)
> -{
> -return MIN(nb_sectors,
> -   s->bdev_length / BDRV_SECTOR_SIZE - sector_num);
> -}
> -
>  /* Round offset and/or bytes to target cluster if COW is needed, and
>   * return the offset of the adjusted tail against original. */
>  static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
> @@ -336,30 +327,28 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
>  static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>  {
>  BlockDriverState *source = s->source;
> -int64_t sector_num, first_chunk;
> +int64_t offset, first_chunk;
>  uint64_t delay_ns = 0;
>  /* At least the first dirty chunk is mirrored in one iteration. */
>  int nb_chunks = 1;
> -int64_t end = s->bdev_length / BDRV_SECTOR_SIZE;
>  int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
>  bool write_zeroes_ok = 
> bdrv_can_write_zeroes_with_unmap(blk_bs(s->target));
>  int max_io_bytes = MAX(s->buf_size / MAX_IN_FLIGHT, MAX_IO_BYTES);
> 
>  bdrv_dirty_bitmap_lock(s->dirty_bitmap);
> -sector_num = bdrv_dirty_iter_next(s->dbi);
> -if (sector_num < 0) {
> +offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
> +if (offset < 0) {
>  bdrv_set_dirty_iter(s->dbi, 0);
> -sector_num = bdrv_dirty_iter_next(s->dbi);
> +offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
>  trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap) *
>BDRV_SECTOR_SIZE);
> -assert(sector_num >= 0);
> +assert(offset >= 0);
>  }
>  bdrv_dirty_bitmap_unlock(s->dirty_bitmap);
> 
> -first_chunk = sector_num / sectors_per_chunk;
> +first_chunk = offset / s->granularity;
>  while (test_bit(first_chunk, s->in_flight_bitmap)) {
> -trace_mirror_yield_in_flight(s, sector_num * BDRV_SECTOR_SIZE,
> - s->in_flight);
> +trace_mirror_yield_in_flight(s, offset, s->in_flight);
>  mirror_wait_for_io(s);
>  }
> 
> @@ -368,25 +357,26 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>  /* Find the number of consective dirty chunks following the first dirty
>   * one, and wait for in flight requests in them. */
>  bdrv_dirty_bitmap_lock(s->dirty_bitmap);
> -while (nb_chunks * sectors_per_chunk < (s->buf_size >> 
> BDRV_SECTOR_BITS)) {
> +while (nb_chunks * s->granularity < s->buf_size) {
>  int64_t next_dirty;
> -int64_t next_sector = sector_num + nb_chunks * sectors_per_chunk;
> -int64_t next_chunk = next_sector / sectors_per_chunk;
> -if (next_sector >= end ||
> -!bdrv_get_dirty_locked(source, s->dirty_bitmap, next_sector)) {
> +int64_t next_offset = offset + nb_chunks * s->granularity;
> +int64_t next_chunk = next_offset / s->granularity;
> +if (next_offset >= s->bdev_length ||
> +!bdrv_get_dirty_locked(source, s->dirty_bitmap,
> +   next_offset >> BDRV_SECTOR_BITS)) {
>  break;
>  }
>  if (test_bit(next_chunk, s->in_flight_bitmap)) {
>  break;
>  }
> 
> -next_dirty = bdrv_dirty_iter_next(s->dbi);
> -if (next_dirty > next_sector || next_dirty < 0) {
> +next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
> +if (next_dirty > next_offset || next_dirty < 0) {
>  /* The bitmap iterator's cache is stale, refresh it */
> -bdrv_set_dirty_iter(s->dbi, next_sector);
> -

Re: [Qemu-block] [PATCH v3 11/20] mirror: Switch mirror_do_read() to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:49PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Convert another internal
> function (no semantic change).
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: rebase to earlier changes
> ---
>  block/mirror.c | 75 
> ++
>  1 file changed, 33 insertions(+), 42 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 1a4602a..81ff784 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -196,7 +196,7 @@ static inline int mirror_clip_sectors(MirrorBlockJob *s,
>  /* Round offset and/or bytes to target cluster if COW is needed, and
>   * return the offset of the adjusted tail against original. */
>  static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
> -unsigned int *bytes)
> +uint64_t *bytes)
>  {
>  bool need_cow;
>  int ret = 0;
> @@ -204,6 +204,7 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t 
> *offset,
>  unsigned int align_bytes = *bytes;
>  int max_bytes = s->granularity * s->max_iov;
> 
> +assert(*bytes < INT_MAX);
>  need_cow = !test_bit(*offset / s->granularity, s->cow_bitmap);
>  need_cow |= !test_bit((*offset + *bytes - 1) / s->granularity,
>s->cow_bitmap);
> @@ -239,59 +240,50 @@ static inline void mirror_wait_for_io(MirrorBlockJob *s)
>  }
> 
>  /* Submit async read while handling COW.
> - * Returns: The number of sectors copied after and including sector_num,
> - *  excluding any sectors copied prior to sector_num due to 
> alignment.
> - *  This will be nb_sectors if no alignment is necessary, or
> - *  (new_end - sector_num) if tail is rounded up or down due to
> + * Returns: The number of bytes copied after and including offset,
> + *  excluding any bytes copied prior to offset due to alignment.
> + *  This will be @bytes if no alignment is necessary, or
> + *  (new_end - offset) if tail is rounded up or down due to
>   *  alignment or buffer limit.
>   */
> -static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
> -  int nb_sectors)
> +static uint64_t mirror_do_read(MirrorBlockJob *s, int64_t offset,
> +   uint64_t bytes)
>  {
>  BlockBackend *source = s->common.blk;
> -int sectors_per_chunk, nb_chunks;
> -int ret;
> +int nb_chunks;
> +uint64_t ret;
>  MirrorOp *op;
> -int max_sectors;
> +uint64_t max_bytes;
> 
> -sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> -max_sectors = sectors_per_chunk * s->max_iov;
> +max_bytes = s->granularity * s->max_iov;
> 
>  /* We can only handle as much as buf_size at a time. */
> -nb_sectors = MIN(s->buf_size >> BDRV_SECTOR_BITS, nb_sectors);
> -nb_sectors = MIN(max_sectors, nb_sectors);
> -assert(nb_sectors);
> -assert(nb_sectors < BDRV_REQUEST_MAX_SECTORS);
> -ret = nb_sectors;
> +bytes = MIN(s->buf_size, MIN(max_bytes, bytes));
> +assert(bytes);
> +assert(bytes < BDRV_REQUEST_MAX_BYTES);
> +ret = bytes;
> 
>  if (s->cow_bitmap) {
> -int64_t offset = sector_num * BDRV_SECTOR_SIZE;
> -unsigned int bytes = nb_sectors * BDRV_SECTOR_SIZE;
> -int gap;
> -
> -gap = mirror_cow_align(s, , );
> -sector_num = offset / BDRV_SECTOR_SIZE;
> -nb_sectors = bytes / BDRV_SECTOR_SIZE;
> -ret += gap / BDRV_SECTOR_SIZE;
> +ret += mirror_cow_align(s, , );
>  }
> -assert(nb_sectors << BDRV_SECTOR_BITS <= s->buf_size);
> -/* The sector range must meet granularity because:
> +assert(bytes <= s->buf_size);
> +/* The range will be sector-aligned because:
>   * 1) Caller passes in aligned values;
> - * 2) mirror_cow_align is used only when target cluster is larger. */
> -assert(!(sector_num % sectors_per_chunk));
> -nb_chunks = DIV_ROUND_UP(nb_sectors, sectors_per_chunk);
> + * 2) mirror_cow_align is used only when target cluster is larger.
> + * But it might not be cluster-aligned at end-of-file. */
> +assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> +nb_chunks = DIV_ROUND_UP(bytes, s->granularity);
> 
>  while (s->buf_free_count < nb_chunks) {
> -trace_mirror_yield_in_flight(s, sector_num * BDRV_SECTOR_SIZE,
> - s->in_flight);
> +trace_mirror_yield_in_flight(s, offset, s->in_flight);
>  mirror_wait_for_io(s);
>  }
> 
>  /* Allocate a MirrorOp that is used as an AIO callback.  */
>  op = g_new(MirrorOp, 1);
>  op->s = s;
> -op->offset = sector_num * BDRV_SECTOR_SIZE;
> -op->bytes = nb_sectors * BDRV_SECTOR_SIZE;
> +op->offset = 

Re: [Qemu-block] [PATCH v3 10/20] mirror: Switch mirror_cow_align() to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:48PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Convert another internal
> function (no semantic change), and add mirror_clip_bytes() as a
> counterpart to mirror_clip_sectors().  Some of the conversion is
> a bit tricky, requiring temporaries to convert between units; it
> will be cleared up in a following patch.
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: tweak mirror_clip_bytes() signature to match previous patch
> ---
>  block/mirror.c | 64 
> ++
>  1 file changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 1a43304..1a4602a 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -176,6 +176,15 @@ static void mirror_read_complete(void *opaque, int ret)
>  aio_context_release(blk_get_aio_context(s->common.blk));
>  }
> 
> +/* Clip bytes relative to offset to not exceed end-of-file */
> +static inline int64_t mirror_clip_bytes(MirrorBlockJob *s,
> +int64_t offset,
> +int64_t bytes)
> +{
> +return MIN(bytes, s->bdev_length - offset);
> +}
> +
> +/* Clip nb_sectors relative to sector_num to not exceed end-of-file */
>  static inline int mirror_clip_sectors(MirrorBlockJob *s,
>int64_t sector_num,
>int nb_sectors)
> @@ -184,44 +193,39 @@ static inline int mirror_clip_sectors(MirrorBlockJob *s,
> s->bdev_length / BDRV_SECTOR_SIZE - sector_num);
>  }
> 
> -/* Round sector_num and/or nb_sectors to target cluster if COW is needed, and
> - * return the offset of the adjusted tail sector against original. */
> -static int mirror_cow_align(MirrorBlockJob *s,
> -int64_t *sector_num,
> -int *nb_sectors)
> +/* Round offset and/or bytes to target cluster if COW is needed, and
> + * return the offset of the adjusted tail against original. */
> +static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
> +unsigned int *bytes)
>  {
>  bool need_cow;
>  int ret = 0;
> -int chunk_sectors = s->granularity >> BDRV_SECTOR_BITS;
> -int64_t align_sector_num = *sector_num;
> -int align_nb_sectors = *nb_sectors;
> -int max_sectors = chunk_sectors * s->max_iov;
> +int64_t align_offset = *offset;
> +unsigned int align_bytes = *bytes;
> +int max_bytes = s->granularity * s->max_iov;
> 
> -need_cow = !test_bit(*sector_num / chunk_sectors, s->cow_bitmap);
> -need_cow |= !test_bit((*sector_num + *nb_sectors - 1) / chunk_sectors,
> +need_cow = !test_bit(*offset / s->granularity, s->cow_bitmap);
> +need_cow |= !test_bit((*offset + *bytes - 1) / s->granularity,
>s->cow_bitmap);
>  if (need_cow) {
> -bdrv_round_sectors_to_clusters(blk_bs(s->target), *sector_num,
> -   *nb_sectors, _sector_num,
> -   _nb_sectors);
> +bdrv_round_to_clusters(blk_bs(s->target), *offset, *bytes,
> +   _offset, _bytes);
>  }
> 
> -if (align_nb_sectors > max_sectors) {
> -align_nb_sectors = max_sectors;
> +if (align_bytes > max_bytes) {
> +align_bytes = max_bytes;
>  if (need_cow) {
> -align_nb_sectors = QEMU_ALIGN_DOWN(align_nb_sectors,
> -   s->target_cluster_size >>
> -   BDRV_SECTOR_BITS);
> +align_bytes = QEMU_ALIGN_DOWN(align_bytes,
> +  s->target_cluster_size);
>  }
>  }
> -/* Clipping may result in align_nb_sectors unaligned to chunk boundary, 
> but
> +/* Clipping may result in align_bytes unaligned to chunk boundary, but
>   * that doesn't matter because it's already the end of source image. */
> -align_nb_sectors = mirror_clip_sectors(s, align_sector_num,
> -   align_nb_sectors);
> +align_bytes = mirror_clip_bytes(s, align_offset, align_bytes);
> 
> -ret = align_sector_num + align_nb_sectors - (*sector_num + *nb_sectors);
> -*sector_num = align_sector_num;
> -*nb_sectors = align_nb_sectors;
> +ret = align_offset + align_bytes - (*offset + *bytes);
> +*offset = align_offset;
> +*bytes = align_bytes;
>  assert(ret >= 0);
>  return ret;
>  }
> @@ -257,10 +261,18 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t 
> sector_num,
>  nb_sectors = MIN(s->buf_size >> BDRV_SECTOR_BITS, nb_sectors);
>  nb_sectors = MIN(max_sectors, nb_sectors);
>  

Re: [Qemu-block] [PATCH v3 09/20] mirror: Update signature of mirror_clip_sectors()

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:47PM -0500, Eric Blake wrote:
> Rather than having a void function that modifies its input
> in-place as the output, change the signature to reduce a layer
> of indirection and return the result.
> 
> Suggested-by: John Snow 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: new patch
> ---
>  block/mirror.c | 15 ---
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index af27bcc..1a43304 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -176,12 +176,12 @@ static void mirror_read_complete(void *opaque, int ret)
>  aio_context_release(blk_get_aio_context(s->common.blk));
>  }
> 
> -static inline void mirror_clip_sectors(MirrorBlockJob *s,
> -   int64_t sector_num,
> -   int *nb_sectors)
> +static inline int mirror_clip_sectors(MirrorBlockJob *s,
> +  int64_t sector_num,
> +  int nb_sectors)
>  {
> -*nb_sectors = MIN(*nb_sectors,
> -  s->bdev_length / BDRV_SECTOR_SIZE - sector_num);
> +return MIN(nb_sectors,
> +   s->bdev_length / BDRV_SECTOR_SIZE - sector_num);
>  }
> 
>  /* Round sector_num and/or nb_sectors to target cluster if COW is needed, and
> @@ -216,7 +216,8 @@ static int mirror_cow_align(MirrorBlockJob *s,
>  }
>  /* Clipping may result in align_nb_sectors unaligned to chunk boundary, 
> but
>   * that doesn't matter because it's already the end of source image. */
> -mirror_clip_sectors(s, align_sector_num, _nb_sectors);
> +align_nb_sectors = mirror_clip_sectors(s, align_sector_num,
> +   align_nb_sectors);
> 
>  ret = align_sector_num + align_nb_sectors - (*sector_num + *nb_sectors);
>  *sector_num = align_sector_num;
> @@ -445,7 +446,7 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>  return 0;
>  }
> 
> -mirror_clip_sectors(s, sector_num, _sectors);
> +io_sectors = mirror_clip_sectors(s, sector_num, io_sectors);
>  switch (mirror_method) {
>  case MIRROR_METHOD_COPY:
>  io_sectors = mirror_do_read(s, sector_num, io_sectors);
> -- 
> 2.9.4
> 

Re: [Qemu-block] [PATCH v3 08/20] mirror: Switch mirror_do_zero_or_discard() to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:46PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Convert another internal
> function (no semantic change).
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: no change
> ---
>  block/mirror.c | 20 +++-
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 9e28d59..af27bcc 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -305,8 +305,8 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t 
> sector_num,
>  }
> 
>  static void mirror_do_zero_or_discard(MirrorBlockJob *s,
> -  int64_t sector_num,
> -  int nb_sectors,
> +  int64_t offset,
> +  uint64_t bytes,
>bool is_discard)
>  {
>  MirrorOp *op;
> @@ -315,16 +315,16 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
>   * so the freeing in mirror_iteration_done is nop. */
>  op = g_new0(MirrorOp, 1);
>  op->s = s;
> -op->offset = sector_num * BDRV_SECTOR_SIZE;
> -op->bytes = nb_sectors * BDRV_SECTOR_SIZE;
> +op->offset = offset;
> +op->bytes = bytes;
> 
>  s->in_flight++;
> -s->bytes_in_flight += nb_sectors * BDRV_SECTOR_SIZE;
> +s->bytes_in_flight += bytes;
>  if (is_discard) {
> -blk_aio_pdiscard(s->target, sector_num << BDRV_SECTOR_BITS,
> +blk_aio_pdiscard(s->target, offset,
>   op->bytes, mirror_write_complete, op);
>  } else {
> -blk_aio_pwrite_zeroes(s->target, sector_num * BDRV_SECTOR_SIZE,
> +blk_aio_pwrite_zeroes(s->target, offset,
>op->bytes, s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
>mirror_write_complete, op);
>  }
> @@ -453,7 +453,8 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>  break;
>  case MIRROR_METHOD_ZERO:
>  case MIRROR_METHOD_DISCARD:
> -mirror_do_zero_or_discard(s, sector_num, io_sectors,
> +mirror_do_zero_or_discard(s, sector_num * BDRV_SECTOR_SIZE,
> +  io_sectors * BDRV_SECTOR_SIZE,
>mirror_method == 
> MIRROR_METHOD_DISCARD);
>  if (write_zeroes_ok) {
>  io_bytes_acct = 0;
> @@ -657,7 +658,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob 
> *s)
>  continue;
>  }
> 
> -mirror_do_zero_or_discard(s, sector_num, nb_sectors, false);
> +mirror_do_zero_or_discard(s, sector_num * BDRV_SECTOR_SIZE,
> +  nb_sectors * BDRV_SECTOR_SIZE, false);
>  sector_num += nb_sectors;
>  }
> 
> -- 
> 2.9.4
> 

Re: [Qemu-block] [PATCH v3 07/20] mirror: Switch MirrorBlockJob to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:45PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Continue by converting an
> internal structure (no semantic change), and all references to the
> buffer size.
> 
> [checkpatch has a false positive on use of MIN() in this patch]
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: no change
> ---
>  block/mirror.c | 79 
> --
>  1 file changed, 38 insertions(+), 41 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index b4dfe95..9e28d59 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -24,9 +24,8 @@
> 
>  #define SLICE_TIME1ULL /* ns */
>  #define MAX_IN_FLIGHT 16
> -#define MAX_IO_SECTORS ((1 << 20) >> BDRV_SECTOR_BITS) /* 1 Mb */
> -#define DEFAULT_MIRROR_BUF_SIZE \
> -(MAX_IN_FLIGHT * MAX_IO_SECTORS * BDRV_SECTOR_SIZE)
> +#define MAX_IO_BYTES (1 << 20) /* 1 Mb */
> +#define DEFAULT_MIRROR_BUF_SIZE (MAX_IN_FLIGHT * MAX_IO_BYTES)
> 
>  /* The mirroring buffer is a list of granularity-sized chunks.
>   * Free chunks are organized in a list.
> @@ -67,11 +66,11 @@ typedef struct MirrorBlockJob {
>  uint64_t last_pause_ns;
>  unsigned long *in_flight_bitmap;
>  int in_flight;
> -int64_t sectors_in_flight;
> +int64_t bytes_in_flight;
>  int ret;
>  bool unmap;
>  bool waiting_for_io;
> -int target_cluster_sectors;
> +int target_cluster_size;
>  int max_iov;
>  bool initial_zeroing_ongoing;
>  } MirrorBlockJob;
> @@ -79,8 +78,8 @@ typedef struct MirrorBlockJob {
>  typedef struct MirrorOp {
>  MirrorBlockJob *s;
>  QEMUIOVector qiov;
> -int64_t sector_num;
> -int nb_sectors;
> +int64_t offset;
> +uint64_t bytes;
>  } MirrorOp;
> 
>  static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
> @@ -101,13 +100,12 @@ static void mirror_iteration_done(MirrorOp *op, int ret)
>  MirrorBlockJob *s = op->s;
>  struct iovec *iov;
>  int64_t chunk_num;
> -int i, nb_chunks, sectors_per_chunk;
> +int i, nb_chunks;
> 
> -trace_mirror_iteration_done(s, op->sector_num * BDRV_SECTOR_SIZE,
> -op->nb_sectors * BDRV_SECTOR_SIZE, ret);
> +trace_mirror_iteration_done(s, op->offset, op->bytes, ret);
> 
>  s->in_flight--;
> -s->sectors_in_flight -= op->nb_sectors;
> +s->bytes_in_flight -= op->bytes;
>  iov = op->qiov.iov;
>  for (i = 0; i < op->qiov.niov; i++) {
>  MirrorBuffer *buf = (MirrorBuffer *) iov[i].iov_base;
> @@ -115,16 +113,15 @@ static void mirror_iteration_done(MirrorOp *op, int ret)
>  s->buf_free_count++;
>  }
> 
> -sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> -chunk_num = op->sector_num / sectors_per_chunk;
> -nb_chunks = DIV_ROUND_UP(op->nb_sectors, sectors_per_chunk);
> +chunk_num = op->offset / s->granularity;
> +nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);
>  bitmap_clear(s->in_flight_bitmap, chunk_num, nb_chunks);
>  if (ret >= 0) {
>  if (s->cow_bitmap) {
>  bitmap_set(s->cow_bitmap, chunk_num, nb_chunks);
>  }
>  if (!s->initial_zeroing_ongoing) {
> -s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE;
> +s->common.offset += op->bytes;
>  }
>  }
>  qemu_iovec_destroy(>qiov);
> @@ -144,7 +141,8 @@ static void mirror_write_complete(void *opaque, int ret)
>  if (ret < 0) {
>  BlockErrorAction action;
> 
> -bdrv_set_dirty_bitmap(s->dirty_bitmap, op->sector_num, 
> op->nb_sectors);
> +bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset >> 
> BDRV_SECTOR_BITS,
> +  op->bytes >> BDRV_SECTOR_BITS);
>  action = mirror_error_action(s, false, -ret);
>  if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
>  s->ret = ret;
> @@ -163,7 +161,8 @@ static void mirror_read_complete(void *opaque, int ret)
>  if (ret < 0) {
>  BlockErrorAction action;
> 
> -bdrv_set_dirty_bitmap(s->dirty_bitmap, op->sector_num, 
> op->nb_sectors);
> +bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset >> 
> BDRV_SECTOR_BITS,
> +  op->bytes >> BDRV_SECTOR_BITS);
>  action = mirror_error_action(s, true, -ret);
>  if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
>  s->ret = ret;
> @@ -171,7 +170,7 @@ static void mirror_read_complete(void *opaque, int ret)
> 
>  mirror_iteration_done(op, ret);
>  } else {
> -blk_aio_pwritev(s->target, op->sector_num * BDRV_SECTOR_SIZE, 
> >qiov,
> +blk_aio_pwritev(s->target, op->offset, >qiov,
>  0, mirror_write_complete, op);
>  }
>  

Re: [Qemu-block] [PATCH v3 06/20] commit: Switch commit_run() to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:44PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Change the internal
> loop iteration of committing to track by bytes instead of sectors
> (although we are still guaranteed that we iterate by steps that
> are sector-aligned).
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: no change
> ---
>  block/commit.c | 16 ++--
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 6f67d78..c3a7bca 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -143,17 +143,16 @@ static void coroutine_fn commit_run(void *opaque)
>  {
>  CommitBlockJob *s = opaque;
>  CommitCompleteData *data;
> -int64_t sector_num, end;
> +int64_t offset;
>  uint64_t delay_ns = 0;
>  int ret = 0;
> -int n = 0;
> +int n = 0; /* sectors */
>  void *buf = NULL;
>  int bytes_written = 0;
>  int64_t base_len;
> 
>  ret = s->common.len = blk_getlength(s->top);
> 
> -
>  if (s->common.len < 0) {
>  goto out;
>  }
> @@ -170,10 +169,9 @@ static void coroutine_fn commit_run(void *opaque)
>  }
>  }
> 
> -end = s->common.len >> BDRV_SECTOR_BITS;
>  buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
> 
> -for (sector_num = 0; sector_num < end; sector_num += n) {
> +for (offset = 0; offset < s->common.len; offset += n * BDRV_SECTOR_SIZE) 
> {
>  bool copy;
> 
>  /* Note that even when no rate limit is applied we need to yield
> @@ -185,15 +183,13 @@ static void coroutine_fn commit_run(void *opaque)
>  }
>  /* Copy if allocated above the base */
>  ret = bdrv_is_allocated_above(blk_bs(s->top), blk_bs(s->base),
> -  sector_num,
> +  offset / BDRV_SECTOR_SIZE,
>COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
>);
>  copy = (ret == 1);
> -trace_commit_one_iteration(s, sector_num * BDRV_SECTOR_SIZE,
> -   n * BDRV_SECTOR_SIZE, ret);
> +trace_commit_one_iteration(s, offset, n * BDRV_SECTOR_SIZE, ret);
>  if (copy) {
> -ret = commit_populate(s->top, s->base,
> -  sector_num * BDRV_SECTOR_SIZE,
> +ret = commit_populate(s->top, s->base, offset,
>n * BDRV_SECTOR_SIZE, buf);
>  bytes_written += n * BDRV_SECTOR_SIZE;
>  }
> -- 
> 2.9.4
> 

Re: [Qemu-block] [PATCH v3 05/20] commit: Switch commit_populate() to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:43PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Start by converting an
> internal function (no semantic change).
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: no change
> ---
>  block/commit.c | 15 ---
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 4cda7f2..6f67d78 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -47,26 +47,25 @@ typedef struct CommitBlockJob {
>  } CommitBlockJob;
> 
>  static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
> -int64_t sector_num, int nb_sectors,
> +int64_t offset, uint64_t bytes,
>  void *buf)
>  {
>  int ret = 0;
>  QEMUIOVector qiov;
>  struct iovec iov = {
>  .iov_base = buf,
> -.iov_len = nb_sectors * BDRV_SECTOR_SIZE,
> +.iov_len = bytes,
>  };
> 
> +assert(bytes < SIZE_MAX);
>  qemu_iovec_init_external(, , 1);
> 
> -ret = blk_co_preadv(bs, sector_num * BDRV_SECTOR_SIZE,
> -qiov.size, , 0);
> +ret = blk_co_preadv(bs, offset, qiov.size, , 0);
>  if (ret < 0) {
>  return ret;
>  }
> 
> -ret = blk_co_pwritev(base, sector_num * BDRV_SECTOR_SIZE,
> - qiov.size, , 0);
> +ret = blk_co_pwritev(base, offset, qiov.size, , 0);
>  if (ret < 0) {
>  return ret;
>  }
> @@ -193,7 +192,9 @@ static void coroutine_fn commit_run(void *opaque)
>  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);
> +ret = commit_populate(s->top, s->base,
> +  sector_num * BDRV_SECTOR_SIZE,
> +  n * BDRV_SECTOR_SIZE, buf);
>  bytes_written += n * BDRV_SECTOR_SIZE;
>  }
>  if (ret < 0) {
> -- 
> 2.9.4
> 

Re: [Qemu-block] [PATCH v3 04/20] stream: Switch stream_run() to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:42PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Change the internal
> loop iteration of streaming to track by bytes instead of sectors
> (although we are still guaranteed that we iterate by steps that
> are sector-aligned).
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: no change
> ---
>  block/stream.c | 24 ++--
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 746d525..2f9618b 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -108,12 +108,11 @@ static void coroutine_fn stream_run(void *opaque)
>  BlockBackend *blk = s->common.blk;
>  BlockDriverState *bs = blk_bs(blk);
>  BlockDriverState *base = s->base;
> -int64_t sector_num = 0;
> -int64_t end = -1;
> +int64_t offset = 0;
>  uint64_t delay_ns = 0;
>  int error = 0;
>  int ret = 0;
> -int n = 0;
> +int n = 0; /* sectors */
>  void *buf;
> 
>  if (!bs->backing) {
> @@ -126,7 +125,6 @@ static void coroutine_fn stream_run(void *opaque)
>  goto out;
>  }
> 
> -end = s->common.len >> BDRV_SECTOR_BITS;
>  buf = qemu_blockalign(bs, STREAM_BUFFER_SIZE);
> 
>  /* Turn on copy-on-read for the whole block device so that guest read
> @@ -138,7 +136,7 @@ static void coroutine_fn stream_run(void *opaque)
>  bdrv_enable_copy_on_read(bs);
>  }
> 
> -for (sector_num = 0; sector_num < end; sector_num += n) {
> +for ( ; offset < s->common.len; offset += n * BDRV_SECTOR_SIZE) {
>  bool copy;
> 
>  /* Note that even when no rate limit is applied we need to yield
> @@ -151,28 +149,26 @@ static void coroutine_fn stream_run(void *opaque)
> 
>  copy = false;
> 
> -ret = bdrv_is_allocated(bs, sector_num,
> +ret = bdrv_is_allocated(bs, offset / BDRV_SECTOR_SIZE,
>  STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, );
>  if (ret == 1) {
>  /* Allocated in the top, no need to copy.  */
>  } else if (ret >= 0) {
>  /* Copy if allocated in the intermediate images.  Limit to the
> - * known-unallocated area [sector_num, sector_num+n).  */
> + * known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE).  
> */
>  ret = bdrv_is_allocated_above(backing_bs(bs), base,
> -  sector_num, n, );
> +  offset / BDRV_SECTOR_SIZE, n, );
> 
>  /* Finish early if end of backing file has been reached */
>  if (ret == 0 && n == 0) {
> -n = end - sector_num;
> +n = (s->common.len - offset) / BDRV_SECTOR_SIZE;
>  }
> 
>  copy = (ret == 1);
>  }
> -trace_stream_one_iteration(s, sector_num * BDRV_SECTOR_SIZE,
> -   n * BDRV_SECTOR_SIZE, ret);
> +trace_stream_one_iteration(s, offset, n * BDRV_SECTOR_SIZE, ret);
>  if (copy) {
> -ret = stream_populate(blk, sector_num * BDRV_SECTOR_SIZE,
> -  n * BDRV_SECTOR_SIZE, buf);
> +ret = stream_populate(blk, offset, n * BDRV_SECTOR_SIZE, buf);
>  }
>  if (ret < 0) {
>  BlockErrorAction action =
> @@ -211,7 +207,7 @@ out:
>  /* Modify backing chain and close BDSes in main loop */
>  data = g_malloc(sizeof(*data));
>  data->ret = ret;
> -data->reached_end = sector_num == end;
> +data->reached_end = offset == s->common.len;
>  block_job_defer_to_main_loop(>common, stream_complete, data);
>  }
> 
> -- 
> 2.9.4
> 

[Qemu-block] [PATCH v2] tests: Avoid non-portable 'echo -ARG'

[Eric Blake]

POSIX says that backslashes in the arguments to 'echo', as well as
any use of 'echo -n' and 'echo -e', are non-portable; it recommends
people should favor 'printf' instead.  This is definitely true where
we do not control which shell is running (such as in makefile snippets
or in documentation examples).  But even for scripts where we
require bash (and therefore, where echo does what we want by default),
it is still possible to use 'shopt -s xpg_echo' to change bash's
behavior of echo.  And setting a good example never hurts when we are
not sure if a snippet will be copied from a bash-only script to a
general shell script (although I don't change the use of non-portable
\e for ESC when we know the running shell is bash).

Replace 'echo -n "..."' with 'printf %s "..."', and 'echo -e "..."'
with 'printf %b "...\n"', with the optimization that the %s/%b
argument can be omitted if the string being printed contains no
substitutions that could result in '%' (trivial if there is no
$ or `, but also possible when using things like $ret that are
known to be numeric given the assignment to ret just above).

In the qemu-iotests check script, fix unusual shell quoting
that would result in word-splitting if 'date' outputs a space.

In test 051, take an opportunity to shorten the line.

In test 068, get rid of a pointless second invocation of bash.

CC: qemu-triv...@nongnu.org
Signed-off-by: Eric Blake 

---
v2: be robust to potential % in substitutions [Max], rebase to master,
shorten some long lines and an odd bash -c use

Add qemu-trivial in cc, although we may decide this is better through
Max's block tree since it is mostly iotests related

---
 qemu-options.hx |  4 ++--
 tests/multiboot/run_test.sh | 10 +-
 tests/qemu-iotests/051  |  7 ---
 tests/qemu-iotests/068  |  2 +-
 tests/qemu-iotests/142  | 48 ++---
 tests/qemu-iotests/171  | 14 ++---
 tests/qemu-iotests/check| 18 -
 tests/rocker/all| 10 +-
 tests/tcg/cris/Makefile |  8 
 9 files changed, 61 insertions(+), 60 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 297bd8a..05a0474 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4363,7 +4363,7 @@ The simplest (insecure) usage is to provide the secret 
inline

 The simplest secure usage is to provide the secret via a file

- # echo -n "letmein" > mypasswd.txt
+ # printf "letmein" > mypasswd.txt
  # $QEMU -object secret,id=sec0,file=mypasswd.txt,format=raw

 For greater security, AES-256-CBC should be used. To illustrate usage,
@@ -4391,7 +4391,7 @@ telling openssl to base64 encode the result, but it could 
be left
 as raw bytes if desired.

 @example
- # SECRET=$(echo -n "letmein" |
+ # SECRET=$(printf "letmein" |
 openssl enc -aes-256-cbc -a -K $KEY -iv $IV)
 @end example

diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
index 78d7edf..9a1cd59 100755
--- a/tests/multiboot/run_test.sh
+++ b/tests/multiboot/run_test.sh
@@ -26,7 +26,7 @@ run_qemu() {
 local kernel=$1
 shift

-echo -e "\n\n=== Running test case: $kernel $@ ===\n" >> test.log
+printf %b "\n\n=== Running test case: $kernel $@ ===\n\n" >> test.log

 $QEMU \
 -kernel $kernel \
@@ -68,21 +68,21 @@ for t in mmap modules; do
 pass=1

 if [ $debugexit != 1 ]; then
-echo -e "\e[31m ?? \e[0m $t (no debugexit used, exit code $ret)"
+printf "\e[31m ?? \e[0m $t (no debugexit used, exit code $ret)\n"
 pass=0
 elif [ $ret != 0 ]; then
-echo -e "\e[31mFAIL\e[0m $t (exit code $ret)"
+printf "\e[31mFAIL\e[0m $t (exit code $ret)\n"
 pass=0
 fi

 if ! diff $t.out test.log > /dev/null 2>&1; then
-echo -e "\e[31mFAIL\e[0m $t (output difference)"
+printf "\e[31mFAIL\e[0m $t (output difference)\n"
 diff -u $t.out test.log
 pass=0
 fi

 if [ $pass == 1 ]; then
-echo -e "\e[32mPASS\e[0m $t"
+printf "\e[32mPASS\e[0m $t\n"
 fi

 done
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 26c29de..c3c08bf 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -217,7 +217,7 @@ run_qemu -drive driver=null-co,cache=invalid_value
 # Test 142 checks the direct=on cases

 for cache in writeback writethrough unsafe invalid_value; do
-echo -e "info block\ninfo block file\ninfo block backing\ninfo block 
backing-file" | \
+printf "info block %s\n" '' file backing backing-file | \
 run_qemu -drive 
file="$TEST_IMG",cache=$cache,backing.file.filename="$TEST_IMG.base",backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=$device_id
 -nodefaults
 done

@@ -325,8 +325,9 @@ echo "qemu-io $device_id \"write -P 0x22 0 4k\"" | run_qemu 
-drive file="$TEST_I

 $QEMU_IO -c "read -P 0x22 0 4k" "$TEST_IMG" | _filter_qemu_io

-echo -e 

Re: [Qemu-block] [PATCH v3 03/20] stream: Switch stream_populate() to byte-based

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:41PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based.  Start by converting an
> internal function (no semantic change).
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: no change
> ---
>  block/stream.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 6cb3939..746d525 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -41,20 +41,20 @@ typedef struct StreamBlockJob {
>  } StreamBlockJob;
> 
>  static int coroutine_fn stream_populate(BlockBackend *blk,
> -int64_t sector_num, int nb_sectors,
> +int64_t offset, uint64_t bytes,
>  void *buf)
>  {
>  struct iovec iov = {
>  .iov_base = buf,
> -.iov_len  = nb_sectors * BDRV_SECTOR_SIZE,
> +.iov_len  = bytes,
>  };
>  QEMUIOVector qiov;
> 
> +assert(bytes < SIZE_MAX);
>  qemu_iovec_init_external(, , 1);
> 
>  /* Copy-on-read the unallocated clusters */
> -return blk_co_preadv(blk, sector_num * BDRV_SECTOR_SIZE, qiov.size, 
> ,
> - BDRV_REQ_COPY_ON_READ);
> +return blk_co_preadv(blk, offset, qiov.size, , 
> BDRV_REQ_COPY_ON_READ);
>  }
> 
>  typedef struct {
> @@ -171,7 +171,8 @@ static void coroutine_fn stream_run(void *opaque)
>  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);
> +ret = stream_populate(blk, sector_num * BDRV_SECTOR_SIZE,
> +  n * BDRV_SECTOR_SIZE, buf);
>  }
>  if (ret < 0) {
>  BlockErrorAction action =
> -- 
> 2.9.4
> 

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

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:40PM -0500, 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.
> 
> The trace interface is low enough that there are no stability
> guarantees, and therefore nothing wrong with changing our units,
> even in cases like trace_backup_do_cow_skip() where we are not
> changing the trace output.  So make the tracing uniformly use
> bytes.
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: improve commit message, no code change
> ---
>  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 9ca1d8e..06431ac 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(_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, _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(_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(>flush_rwlock);
> 
> diff --git a/block/commit.c b/block/commit.c
> index 6993994..4cda7f2 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -190,7 +190,8 @@ static void coroutine_fn commit_run(void *opaque)
>COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
>);
>  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 eb27efc..b4dfe95 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);
> 
>  

Re: [Qemu-block] [PATCH v4 1/2] iotests: Use absolute paths for executables

[Eric Blake]

On 06/30/2017 02:41 PM, Eric Blake wrote:

> +++ 068.out.bad   2017-06-30 14:35:28.720241398 -0500
> @@ -1,4 +1,5 @@
>  QA output created by 068
> +realpath: '': No such file or directory
> 
> The culprit? $QEMU_VXHS_PROG is empty for me, which means `set_prog_path
> qnio_server` found nothing to use.  You'll have to add in a safety valve
> that only calls 'type' if operating on a non-empty path in the first place.

I'm using this locally, in the meantime:

diff --git i/tests/qemu-iotests/common.config
w/tests/qemu-iotests/common.config
index c1dc425..6f97331 100644
--- i/tests/qemu-iotests/common.config
+++ w/tests/qemu-iotests/common.config
@@ -107,7 +107,9 @@ export QEMU_PROG=$(realpath -- "$(type -p
"$QEMU_PROG")")
 export QEMU_IMG_PROG=$(realpath -- "$(type -p "$QEMU_IMG_PROG")")
 export QEMU_IO_PROG=$(realpath -- "$(type -p "$QEMU_IO_PROG")")
 export QEMU_NBD_PROG=$(realpath -- "$(type -p "$QEMU_NBD_PROG")")
-export QEMU_VXHS_PROG=$(realpath -- "$(type -p "$QEMU_VXHS_PROG")")
+if [ -n "$QEMU_VXHS_PROG" ]; then
+export QEMU_VXHS_PROG=$(realpath -- "$(type -p "$QEMU_VXHS_PROG")")
+fi

 _qemu_wrapper()
 {


Is qnio_server easily available for Fedora?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 01/20] blockjob: Track job ratelimits via bytes, not sectors

[Jeff Cody]

On Tue, Jun 27, 2017 at 02:24:39PM -0500, 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.
> 
> Repetition of expressions like 'n * BDRV_SECTOR_SIZE' will be
> cleaned up later when functions are converted to iterate over
> images by bytes rather than by sectors.
> 
> Signed-off-by: Eric Blake 
> Reviewed-by: John Snow 
> 

Reviewed-by: Jeff Cody 

> ---
> v2: adjust commit message based on review; no code change
> ---
>  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 5387fbd..9ca1d8e 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(>limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME);
> +ratelimit_set_speed(>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(>limit,
> -  job->sectors_read);
> +  job->sectors_read *
> +  BDRV_SECTOR_SIZE);
>  job->sectors_read = 0;
>  block_job_sleep_ns(>common, QEMU_CLOCK_REALTIME, delay_ns);
>  } else {
> diff --git a/block/commit.c b/block/commit.c
> index 524bd54..6993994 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -209,7 +209,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(>limit, n);
> +delay_ns = ratelimit_calculate_delay(>limit,
> + n * BDRV_SECTOR_SIZE);
>  }
>  }
> 
> @@ -231,7 +232,7 @@ static void commit_set_speed(BlockJob *job, int64_t 
> speed, Error **errp)
>  error_setg(errp, QERR_INVALID_PARAMETER, "speed");
>  return;
>  }
> -ratelimit_set_speed(>limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME);
> +ratelimit_set_speed(>limit, speed, SLICE_TIME);
>  }
> 
>  static const BlockJobDriver commit_job_driver = {
> diff --git a/block/mirror.c b/block/mirror.c
> index 61a862d..eb27efc 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -396,7 +396,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,
> @@ -444,16 +445,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 {
> 

Re: [Qemu-block] [PATCH v4 1/2] iotests: Use absolute paths for executables

[Eric Blake]

On 06/29/2017 09:46 PM, Max Reitz wrote:

>>> +++ b/tests/qemu-iotests/common.config
>>> @@ -103,6 +103,12 @@ if [ -z "$QEMU_VXHS_PROG" ]; then
>>>  export QEMU_VXHS_PROG="`set_prog_path qnio_server`"
>>>  fi
>>>  
>>> +export QEMU_PROG=$(realpath -- "$(type -p "$QEMU_PROG")")
>>
>> ...now that you updated per my review to favor 'type' over 'which'?
>> Otherwise, the R-b stands.
> 
> Thanks, will fix and apply then...
> 
> ...and done, applied to my block branch:
> 
> https://github.com/XanClic/qemu/commits/block

Sorry for not noticing sooner, but you'll need to replace v4 (commit
0f0fec82 on your branch) with a fix, because now your branch does the
following on all iotests for me:

068 3s ... - output mismatch (see 068.out.bad)
--- /home/eblake/qemu/tests/qemu-iotests/068.out2017-06-26
22:02:56.057734882 -0500
+++ 068.out.bad 2017-06-30 14:35:28.720241398 -0500
@@ -1,4 +1,5 @@
 QA output created by 068
+realpath: '': No such file or directory

The culprit? $QEMU_VXHS_PROG is empty for me, which means `set_prog_path
qnio_server` found nothing to use.  You'll have to add in a safety valve
that only calls 'type' if operating on a non-empty path in the first place.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v22 19/30] qcow2: add persistent dirty bitmaps support

[Eric Blake]

On 06/30/2017 12:58 PM, John Snow wrote:

>>
>> "Structure of a bitmap directory entry:
>> ...
>>  8 - 11:bitmap_table_size
>> Number of entries in the bitmap table of the bitmap."
>>
> 
> This is the number of bitmaps stored in the qcow2, not the size of one
> particular bitmap.

No, I was quoting from "Structure of a bitmap directory entry" (the same
struct that also includes a per-bitmap granularity).

However, on re-reading the difference between the bitmap table and the
bitmap data, I see that the bitmap_table_size it formally the number of
cluster mappings that the bitmap occupies - so while it is not a precise
size of the bitmap, it IS an approximation (where scaling has introduced
lost precision for all sizes that map within the final cluster).  But my
point remains - we have some imprecision on whether bitmap_table_size
has to be clamped to the same number as you would get if the current
virtual disk size is converted into bitmaps, or whether it is valid to
have a qcow2 file where a bitmap table contains fewer cluster mappings
than would be required to cover the whole virtual file, or conversely
contains more cluster mappings than what you get even when rounding the
virtual table size up to the bitmap granularity and further scaled by
how many bits fit per cluster.

Concretely, if I'm using 512-byte clusters (the qcow2 minimum), and
qcow2 forces me to use a bitmap granularity no smaller than 9 (each bit
of the bitmap covers 512 bytes), then one cluster of bitmap data covers
2M of virtual size.  If I have an image with a virtual size of exactly
4M, and a bitmap covering that image, then my bitmap table _should_ have
exactly two mappings (bitmap_table_size should be 2, because it required
2 8-byte entries in the table to give the mapping for the 2 clusters
used to contain all the bits of the bitmap; the remaining 496 bytes of
the bitmap table should be 0 because there are no further mappings).
But note that any size in the range (2M, 4M] as the same
bitmap_table_size of 2 for that granularity.

If the bitmap is tied to the image (has the 'auto' and/or 'dirty' bit
set), then I agree that the bitmap size should match the virtual image
size.  But if the bitmap is independent, what technical reason do we
have that prevents us from having a bitmap covering 2M or less of data
(bitmap_table_size of 1), or more than 4M of data (bitmap_table_size of
3 or more), even though it has no relation to the current virtual image
size of 4M?

Meanwhile, if I use a bitmap granularity of 1k instead of 512, in the
same image with 512-byte clusters, a virtual size of 2M is
indistinguishable from a virtual size of 4M (both bitmaps fit within a
single cluster, or bitmap_table_size of 1; although the 2M case only
uses 256 of the 512 bytes in the bitmap data cluster).

I'm worried that we are too strict in stating that ALL bitmaps are tied
to the current virtual image size, but I'm also worried that our spec
might not be strict enough in enforcing that certain bitmaps MUST match
the virtual size (if the bitmap is automatically tracking writes to the
virtual image); and also worried whether we are setting ourselves up for
obscure failures based on the interaction of resize and/or internal
snapshots when bitmaps are in play (or whether we are being conservative
and forbidding those interactions until we've had time to further prove
that we handle their interaction safely).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v22 19/30] qcow2: add persistent dirty bitmaps support

[John Snow]

On 06/30/2017 01:47 PM, Eric Blake wrote:
> On 06/29/2017 09:23 PM, Max Reitz wrote:
>> On 2017-06-30 04:18, Eric Blake wrote:
>>> On 06/28/2017 07:05 AM, Vladimir Sementsov-Ogievskiy wrote:
 Store persistent dirty bitmaps in qcow2 image.

 Signed-off-by: Vladimir Sementsov-Ogievskiy 
 Reviewed-by: Max Reitz 
 ---
> 
>>>
>>> This grabs the size (currently in sectors, although I plan to fix it to
>>> be in bytes)...
>>>
 +const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
 +uint8_t *buf = NULL;
 +BdrvDirtyBitmapIter *dbi;
 +uint64_t *tb;
 +uint64_t tb_size =
 +size_to_clusters(s,
 +bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
>>>
>>> ...then finds out how many bytes are required to serialize the entire
>>> image, where bm_size should be the same as before...
>>>
 +while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
 +uint64_t cluster = sector / sbc;
 +uint64_t end, write_size;
 +int64_t off;
 +
 +sector = cluster * sbc;
 +end = MIN(bm_size, sector + sbc);
 +write_size =
 +bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - 
 sector);
>>>
>>> But here, rather than tackling the entire image at once, you are
>>> subdividing your queries along arbitrary lines. But nowhere do I see a
>>> call to bdrv_dirty_bitmap_serialization_align() to make sure your
>>> end-sector value is properly aligned; if it is not aligned, you will
>>> trigger an assertion failure here...
>>
>> It's 4:21 am here, so I cannot claim to be right, but if I remember
>> correctly, it will automatically aligned because sbc is the number of
>> bits (and thus sectors) a bitmap cluster covers.
> 
> Okay, I re-read the spec.  First thing I noticed: we have a potential
> conflict if an image is allowed to be resized:
> 
> "All stored bitmaps are related to the virtual disk stored in the same
> image, so each bitmap size is equal to the virtual disk size."
> 
> If you resize an image, does the bitmap size have to be adjusted as
> well?  What if you create one bitmap, then take an internal snapshot,
> then resize?  Or do we declare that (at least for now) the presence of a
> bitmap is incompatible with the use of an internal snapshot?
> 
> Conversely, we state that:
> 
> 
> "Structure of a bitmap directory entry:
> ...
>  8 - 11:bitmap_table_size
> Number of entries in the bitmap table of the bitmap."
> 

This is the number of bitmaps stored in the qcow2, not the size of one
particular bitmap.

> Since a bitmap therefore tracks its own size, I think the earlier
> statement that all bitmap sizes are equal to the virtual disk size is
> too strict (there seems to be no technical reason why a bitmap can't
> have a different size that the image).
> 
> 
> But, having read that, you are correct that we are subdividing our
> bitmaps according to what fits in a qcow2 cluster, and the smallest
> qcow2 cluster we can come up with is 512-bytes (or 4k bits of the
> bitmap).  Checking hbitmap.c, we are merely asserting that our alignment
> is always a multiple of 64 << hb->granularity.  But since we are
> partitioning a cluster at a time, our minimum alignment will be 512 <<
> hb->granularity, which is always aligned.
> 
> So all the more I need to do is add an assertion.
> 
>> I'm for fixing it later. I would have announced the series "applied" an
>> hour ago if I hadn't had to bisect iotest 055 breakage...
> 
> I'm working it into my v4 posting of dirty-bitmap cleanups.
> 

Re: [Qemu-block] [Qemu-devel] [PATCH v22 06/30] block/dirty-bitmap: add deserialize_ones func

[John Snow]

On 06/29/2017 10:01 PM, Eric Blake wrote:
> On 06/29/2017 08:55 PM, Eric Blake wrote:
>> On 06/28/2017 07:05 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Add bdrv_dirty_bitmap_deserialize_ones() function, which is needed for
>>> qcow2 bitmap loading, to handle unallocated bitmap parts, marked as
>>> all-ones.
>>>
> 
>>> + * hbitmap_deserialize_ones
>>> + * @hb: HBitmap to operate on.
>>> + * @start: First bit to restore.
>>> + * @count: Number of bits to restore.
>>
>> This part is accurate (the dirty-bitmap is using an underlying bitmap
>> with "one bit per sector" before my series, afterwards it will be "one
>> bit per byte", remembering that hbitmap really stores only one bit per
>> granularity multiple of the underlying unit), if incomplete (the code
>> asserts that things are aligned, but doesn't document that the caller
>> must pass in aligned values); but again, that's matching the
>> pre-existing deserialize_zeroes code.
> 
> Okay, I looked again; the documentation for
> hbitmap_serialization_granularity() has a blanket statement that all
> other hbitmap_serialization_* functions taking a start and count must be
> aligned. Indirect, but at least documented, so I retract my statement
> about the docs being incomplete.
> 

If the docs are confusing, please send patches to amend them; the bitmap
code is definitely very confusing at times and I appreciate any and all
insight to make the names, variable and documentation read better to be
more intuitive.

Thanks!

Re: [Qemu-block] [PATCH v22 19/30] qcow2: add persistent dirty bitmaps support

[Eric Blake]

On 06/29/2017 09:23 PM, Max Reitz wrote:
> On 2017-06-30 04:18, Eric Blake wrote:
>> On 06/28/2017 07:05 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Store persistent dirty bitmaps in qcow2 image.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> Reviewed-by: Max Reitz 
>>> ---

>>
>> This grabs the size (currently in sectors, although I plan to fix it to
>> be in bytes)...
>>
>>> +const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
>>> +uint8_t *buf = NULL;
>>> +BdrvDirtyBitmapIter *dbi;
>>> +uint64_t *tb;
>>> +uint64_t tb_size =
>>> +size_to_clusters(s,
>>> +bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
>>
>> ...then finds out how many bytes are required to serialize the entire
>> image, where bm_size should be the same as before...
>>
>>> +while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
>>> +uint64_t cluster = sector / sbc;
>>> +uint64_t end, write_size;
>>> +int64_t off;
>>> +
>>> +sector = cluster * sbc;
>>> +end = MIN(bm_size, sector + sbc);
>>> +write_size =
>>> +bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - 
>>> sector);
>>
>> But here, rather than tackling the entire image at once, you are
>> subdividing your queries along arbitrary lines. But nowhere do I see a
>> call to bdrv_dirty_bitmap_serialization_align() to make sure your
>> end-sector value is properly aligned; if it is not aligned, you will
>> trigger an assertion failure here...
> 
> It's 4:21 am here, so I cannot claim to be right, but if I remember
> correctly, it will automatically aligned because sbc is the number of
> bits (and thus sectors) a bitmap cluster covers.

Okay, I re-read the spec.  First thing I noticed: we have a potential
conflict if an image is allowed to be resized:

"All stored bitmaps are related to the virtual disk stored in the same
image, so each bitmap size is equal to the virtual disk size."

If you resize an image, does the bitmap size have to be adjusted as
well?  What if you create one bitmap, then take an internal snapshot,
then resize?  Or do we declare that (at least for now) the presence of a
bitmap is incompatible with the use of an internal snapshot?

Conversely, we state that:


"Structure of a bitmap directory entry:
...
 8 - 11:bitmap_table_size
Number of entries in the bitmap table of the bitmap."

Since a bitmap therefore tracks its own size, I think the earlier
statement that all bitmap sizes are equal to the virtual disk size is
too strict (there seems to be no technical reason why a bitmap can't
have a different size that the image).


But, having read that, you are correct that we are subdividing our
bitmaps according to what fits in a qcow2 cluster, and the smallest
qcow2 cluster we can come up with is 512-bytes (or 4k bits of the
bitmap).  Checking hbitmap.c, we are merely asserting that our alignment
is always a multiple of 64 << hb->granularity.  But since we are
partitioning a cluster at a time, our minimum alignment will be 512 <<
hb->granularity, which is always aligned.

So all the more I need to do is add an assertion.

> I'm for fixing it later. I would have announced the series "applied" an
> hour ago if I hadn't had to bisect iotest 055 breakage...

I'm working it into my v4 posting of dirty-bitmap cleanups.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] tests: Avoid non-portable 'echo -ARG'

[Eric Blake]

On 06/30/2017 09:38 AM, Max Reitz wrote:
> On 2017-06-28 16:21, Eric Blake wrote:
>> POSIX says that backslashes in the arguments to 'echo', as well as
>> any use of 'echo -n' and 'echo -e', are non-portable; it recommends
>> people should favor 'printf' instead.  This is definitely true where
>> we do not control which shell is running (such as in makefile snippets
>> or in documentation examples).  But even for scripts where we
>> require bash (and therefore, where echo does what we want by default),
>> it is still possible to use 'shopt -s xpg_echo' to change bash's
>> behavior of echo.  And setting a good example never hurts when we are
>> not sure if a snippet will be copied from a bash-only script to a
>> general shell script (although I don't change the use of non-portable
>> \e for ESC when we know the running shell is bash).
>>
>> Replace 'echo -n "..."' with 'printf "..."', and 'echo -e "..."'
>> with 'printf "...\n"'.
>>
>> In the qemu-iotests check script, also fix unusual shell quoting
>> that would result in word-splitting if 'date' outputs a space.
>>
>> Signed-off-by: Eric Blake 
>> ---

> Question 1: Who's going to take this? :-)

It's test-related, touching mostly iotests. But it's also a candidate
for qemu-trivial (I'll cc them on v2).

> 
> Question 2: This breaks 171 if TEST_DIR contains a % (e.g.
> "TEST_DIR=/tmp/foo%% ./check -raw 171"). Is that OK?

No.  A more formal fix is using 'printf %s "..."' in place of 'echo -n
"..."', if the "..." contains any substitutions.  The extra %s is not
needed when there are no risky substitutions.  I'll spin up a v2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] tests: Avoid non-portable 'echo -ARG'

[Max Reitz]

On 2017-06-28 16:21, Eric Blake wrote:
> POSIX says that backslashes in the arguments to 'echo', as well as
> any use of 'echo -n' and 'echo -e', are non-portable; it recommends
> people should favor 'printf' instead.  This is definitely true where
> we do not control which shell is running (such as in makefile snippets
> or in documentation examples).  But even for scripts where we
> require bash (and therefore, where echo does what we want by default),
> it is still possible to use 'shopt -s xpg_echo' to change bash's
> behavior of echo.  And setting a good example never hurts when we are
> not sure if a snippet will be copied from a bash-only script to a
> general shell script (although I don't change the use of non-portable
> \e for ESC when we know the running shell is bash).
> 
> Replace 'echo -n "..."' with 'printf "..."', and 'echo -e "..."'
> with 'printf "...\n"'.
> 
> In the qemu-iotests check script, also fix unusual shell quoting
> that would result in word-splitting if 'date' outputs a space.
> 
> Signed-off-by: Eric Blake 
> ---
> 
> Of course, Stefan's pending patch:
> [PATCH 3/5] qemu-iotests: 068: extract _qemu() function
> also touches 068, so there may be some (obvious) merge conflicts
> to resolve there depending on what goes in first.
> 
>  qemu-options.hx |  4 ++--
>  tests/multiboot/run_test.sh | 10 +-
>  tests/qemu-iotests/051  |  7 ---
>  tests/qemu-iotests/068  |  2 +-
>  tests/qemu-iotests/142  | 48 
> ++---
>  tests/qemu-iotests/171  | 14 ++---
>  tests/qemu-iotests/check| 18 -
>  tests/rocker/all| 10 +-
>  tests/tcg/cris/Makefile |  8 
>  9 files changed, 61 insertions(+), 60 deletions(-)

Question 1: Who's going to take this? :-)

Question 2: This breaks 171 if TEST_DIR contains a % (e.g.
"TEST_DIR=/tmp/foo%% ./check -raw 171"). Is that OK?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [RFC] QMP design: Fixing query-block and friends

[Alberto Garcia]

On Fri 30 Jun 2017 04:22:11 PM CEST, Kevin Wolf wrote:
> Am 30.06.2017 um 15:01 hat Alberto Garcia geschrieben:
>> On Tue 27 Jun 2017 06:31:45 PM CEST, Kevin Wolf wrote:
>> > * Speaking of recursion: ImageInfo recursively includes information
>> >   about all images in the backing chain. This is what makes the output
>> >   of query-named-block-nodes so redundant. It is also inconsistent
>> >   because the runtime information (BlockInfo/BlockDeviceInfo) isn't
>> >   recursive.
>> 
>> I've also been told of cases where a query-block command on a VM with a
>> very large amount of images in all backing chains would slow down the
>> guest because of this recursion.
>> 
>> I haven't been able to reproduce this problem myself, but being able to
>> see only the devices seen by the guest (as opposed to the complete
>> graph) seems like a good idea.
>
> I think the information only really becomes redundant if you use
> query-named-block-nodes because then you list every backing file node
> and each of them contains the recursive information.
>
> With query-block, you start at the top and include the information for
> each image in the backing file chain only once. If we did indeed have
> a problem there, that might mean that we do in fact need filtering to
> reduce the overhead. But I don't think any of the information involves
> any I/O to get, so it seems unlikely to me that this would make a big
> difference.

Yes, that was also what I thought, but I haven't been able to reproduce
the problem so I don't know yet. In my own tests with thousands of
backing files I haven't noticed any slowdown caused by query-block.

Berto

Re: [Qemu-block] [RFC] QMP design: Fixing query-block and friends

[Kevin Wolf]

Am 30.06.2017 um 15:01 hat Alberto Garcia geschrieben:
> On Tue 27 Jun 2017 06:31:45 PM CEST, Kevin Wolf wrote:
> > * Speaking of recursion: ImageInfo recursively includes information
> >   about all images in the backing chain. This is what makes the output
> >   of query-named-block-nodes so redundant. It is also inconsistent
> >   because the runtime information (BlockInfo/BlockDeviceInfo) isn't
> >   recursive.
> 
> I've also been told of cases where a query-block command on a VM with a
> very large amount of images in all backing chains would slow down the
> guest because of this recursion.
> 
> I haven't been able to reproduce this problem myself, but being able to
> see only the devices seen by the guest (as opposed to the complete
> graph) seems like a good idea.

I think the information only really becomes redundant if you use
query-named-block-nodes because then you list every backing file node
and each of them contains the recursive information.

With query-block, you start at the top and include the information for
each image in the backing file chain only once. If we did indeed have a
problem there, that might mean that we do in fact need filtering to
reduce the overhead. But I don't think any of the information involves
any I/O to get, so it seems unlikely to me that this would make a big
difference.

Kevin

Re: [Qemu-block] [PATCH v6 0/5] Improve I/O tests coverage of LUKS driver

[Max Reitz]

On 2017-06-26 14:35, Daniel P. Berrange wrote:
> The main goal of this series is to get the I/O tests passing
> 100% with LUKS when run with './check -luks'. It also adds a
> few more combinations to the LUKS/dmcrypt interoperability
> test.
> 
> To make LUKS testing not quite as slow, we drop the PBKDF
> iteration count down to a very small value. This doesn't
> remove all overhead, as formatting the volume will always
> measure PBKDF timing over a 1 second interval.

Thanks, applied to my block branch:

https://github.com/XanClic/qemu/commits/block

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [RFC] QMP design: Fixing query-block and friends

[Alberto Garcia]

On Tue 27 Jun 2017 06:31:45 PM CEST, Kevin Wolf wrote:
> * Speaking of recursion: ImageInfo recursively includes information
>   about all images in the backing chain. This is what makes the output
>   of query-named-block-nodes so redundant. It is also inconsistent
>   because the runtime information (BlockInfo/BlockDeviceInfo) isn't
>   recursive.

I've also been told of cases where a query-block command on a VM with a
very large amount of images in all backing chains would slow down the
guest because of this recursion.

I haven't been able to reproduce this problem myself, but being able to
see only the devices seen by the guest (as opposed to the complete
graph) seems like a good idea.

Berto

Re: [Qemu-block] [PATCH 0/6] virtio: use ioeventfd in TCG and qtest mode

[Stefan Hajnoczi]

On Wed, Jun 28, 2017 at 07:47:18PM +0100, Stefan Hajnoczi wrote:
> This patch series fixes qemu-iotests 068.  Since commit
> ea4f3cebc4e0224605ab9dd9724aa4e7768fe372 ("qemu-iotests: 068: test iothread
> mode") the test case has attempted to use dataplane without -M accel=kvm.
> Although QEMU is capable of running TCG or qtest with emulated ioeventfd/irqfd
> we haven't enabled it yet.
> 
> Unfortunately the virtio test cases fail when ioeventfd is enabled in qtest
> mode.  This is because they make assumptions about virtqueue ISR signalling.
> They assume that a request is completed when ISR becomes 1.  However, the ISR
> can be set to 1 even though no new request has completed since commit
> 83d768b5640946b7da55ce8335509df297e2c7cd "virtio: set ISR on dataplane
> notifications".
> 
> This issue is solved by introducing a proper qvirtqueue_get_buf() API (similar
> to the Linux guest drivers) instead of making assumptions about the ISR.  Most
> of the patches update the test cases to use the new API.
> 
> Stefan Hajnoczi (6):
>   libqos: fix typo in virtio.h QVirtQueue->used comment
>   libqos: add virtio used ring support
>   tests: fix virtio-scsi-test ISR dependence
>   tests: fix virtio-blk-test ISR dependence
>   tests: fix virtio-net-test ISR dependence
>   virtio-pci: use ioeventfd even when KVM is disabled
> 
>  tests/libqos/virtio.h|  8 ++-
>  hw/virtio/virtio-pci.c   |  2 +-
>  tests/libqos/virtio.c| 60 
> 
>  tests/virtio-blk-test.c  | 27 ++
>  tests/virtio-net-test.c  |  6 ++---
>  tests/virtio-scsi-test.c |  2 +-
>  6 files changed, 89 insertions(+), 16 deletions(-)
> 
> -- 
> 2.9.4
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH v1 2/3] util/oslib-win32: Remove invalid check

[Peter Maydell]

On 29 June 2017 at 18:16, Alistair Francis  wrote:
> There is no way nhandles can be zero in this section so that part of the
> if statement will always be false. Let's just remove it to make the code
> easier to read.
>
> Signed-off-by: Alistair Francis 
> Acked-by: Edgar E. Iglesias 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Fam Zheng 
> ---
>
>  util/oslib-win32.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 80e4668935..7ec0f8e083 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -414,7 +414,7 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, 
> gint nhandles,
>  /* If we have a timeout, or no handles to poll, be satisfied
>   * with just noticing we have messages waiting.
>   */
> -if (timeout != 0 || nhandles == 0) {
> +if (timeout != 0) {
>  return 1;
>  }

The comment and the code now don't match -- does the comment
need updating too?

thanks
-- PMM

Re: [Qemu-block] [PATCH v1 2/3] util/oslib-win32: Remove invalid check

[Paolo Bonzini]

On 29/06/2017 19:16, Alistair Francis wrote:
> There is no way nhandles can be zero in this section so that part of the
> if statement will always be false. Let's just remove it to make the code
> easier to read.
> 
> Signed-off-by: Alistair Francis 
> Acked-by: Edgar E. Iglesias 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Fam Zheng 
> ---
> 
>  util/oslib-win32.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 80e4668935..7ec0f8e083 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -414,7 +414,7 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, 
> gint nhandles,
>  /* If we have a timeout, or no handles to poll, be satisfied
>   * with just noticing we have messages waiting.
>   */
> -if (timeout != 0 || nhandles == 0) {
> +if (timeout != 0) {
>  return 1;
>  }
>  

Can you explain this better?

Paolo

Re: [Qemu-block] [PATCH] tests: Avoid non-portable 'echo -ARG'

[Stefan Hajnoczi]

On Wed, Jun 28, 2017 at 09:21:37AM -0500, Eric Blake wrote:
> POSIX says that backslashes in the arguments to 'echo', as well as
> any use of 'echo -n' and 'echo -e', are non-portable; it recommends
> people should favor 'printf' instead.  This is definitely true where
> we do not control which shell is running (such as in makefile snippets
> or in documentation examples).  But even for scripts where we
> require bash (and therefore, where echo does what we want by default),
> it is still possible to use 'shopt -s xpg_echo' to change bash's
> behavior of echo.  And setting a good example never hurts when we are
> not sure if a snippet will be copied from a bash-only script to a
> general shell script (although I don't change the use of non-portable
> \e for ESC when we know the running shell is bash).
> 
> Replace 'echo -n "..."' with 'printf "..."', and 'echo -e "..."'
> with 'printf "...\n"'.
> 
> In the qemu-iotests check script, also fix unusual shell quoting
> that would result in word-splitting if 'date' outputs a space.
> 
> Signed-off-by: Eric Blake 
> ---
> 
> Of course, Stefan's pending patch:
> [PATCH 3/5] qemu-iotests: 068: extract _qemu() function
> also touches 068, so there may be some (obvious) merge conflicts
> to resolve there depending on what goes in first.
> 
>  qemu-options.hx |  4 ++--
>  tests/multiboot/run_test.sh | 10 +-
>  tests/qemu-iotests/051  |  7 ---
>  tests/qemu-iotests/068  |  2 +-
>  tests/qemu-iotests/142  | 48 
> ++---
>  tests/qemu-iotests/171  | 14 ++---
>  tests/qemu-iotests/check| 18 -
>  tests/rocker/all| 10 +-
>  tests/tcg/cris/Makefile |  8 
>  9 files changed, 61 insertions(+), 60 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature