[Qemu-commits] [qemu/qemu] cdd346: nvme: Fix get/set number of queues feature, again

[GitHub]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: cdd346371e09709be8e46398bb097dc690a746f2
      
https://github.com/qemu/qemu/commit/cdd346371e09709be8e46398bb097dc690a746f2
  Author: Dan Aloni <address@hidden>
  Date:   2017-08-29 (Tue, 29 Aug 2017)

  Changed paths:
    M hw/block/nvme.c

  Log Message:
  -----------
  nvme: Fix get/set number of queues feature, again

The number of queues that should be return by the admin command should:

  1) Only mention the number of non-admin queues.
  2) It is zero-based, meaning that '0 == one non-admin queue',
     '1 == two non-admin queues', and so forth.

Because our `num_queues` means the number of queues _plus_ the admin
queue, then the right calculation for the number returned from the admin
command is `num_queues - 2`, combining the two requirements mentioned.

The issue was discovered by reducing num_queues from 64 to 8 and running
a Linux VM with an SMP parameter larger than that (e.g. 22). It tries to
utilize all queues, and therefore fails with an invalid queue number
when trying to queue I/Os on the last queue.

Signed-off-by: Dan Aloni <address@hidden>
CC: Alex Friedman <address@hidden>
CC: Keith Busch <address@hidden>
CC: Stefan Hajnoczi <address@hidden>
Reviewed-by: Keith Busch <address@hidden>
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: faa8215c1778159803d6af9368b922d8ef61aee2
      
https://github.com/qemu/qemu/commit/faa8215c1778159803d6af9368b922d8ef61aee2
  Author: Alberto Garcia <address@hidden>
  Date:   2017-08-29 (Tue, 29 Aug 2017)

  Changed paths:
    M include/qemu/throttle.h

  Log Message:
  -----------
  throttle: Fix wrong variable name in the header documentation

The level of the burst bucket is stored in bkt.burst_level, not
bkt.burst_length.

Signed-off-by: Alberto Garcia <address@hidden>
Reviewed-by: Manos Pitsidianakis <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 0770a7a6466cc2dbf4ac91841173ad4488e1fbc7
      
https://github.com/qemu/qemu/commit/0770a7a6466cc2dbf4ac91841173ad4488e1fbc7
  Author: Alberto Garcia <address@hidden>
  Date:   2017-08-29 (Tue, 29 Aug 2017)

  Changed paths:
    M util/throttle.c

  Log Message:
  -----------
  throttle: Update the throttle_fix_bucket() documentation

The way the throttling algorithm works is that requests start being
throttled once the bucket level exceeds the burst limit. When we get
there the bucket leaks at the level set by the user (bkt->avg), and
that leak rate is what prevents guest I/O from exceeding the desired
limit.

If we don't allow bursts (i.e. bkt->max == 0) then we can start
throttling requests immediately. The problem with keeping the
threshold at 0 is that it only allows one request at a time, and as
soon as there's a bit of I/O from the guest every other request will
be throttled and performance will suffer considerably. That can even
make the guest unable to reach the throttle limit if that limit is
high enough, and that happens regardless of the block scheduler used
by the guest.

Increasing that threshold gives flexibility to the guest, allowing it
to perform short bursts of I/O before being throttled. Increasing the
threshold too much does not make a difference in the long run (because
it's the leak rate what defines the actual throughput) but it does
allow the guest to perform longer initial bursts and exceed the
throttle limit for a short while.

A burst value of bkt->avg / 10 allows the guest to perform 100ms'
worth of I/O at the target rate without being throttled.

Signed-off-by: Alberto Garcia <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: fa36f1b2ebcd9a7b2a58c8e12dfb1cc8596c23c0
      
https://github.com/qemu/qemu/commit/fa36f1b2ebcd9a7b2a58c8e12dfb1cc8596c23c0
  Author: Alberto Garcia <address@hidden>
  Date:   2017-08-29 (Tue, 29 Aug 2017)

  Changed paths:
    M util/throttle.c

  Log Message:
  -----------
  throttle: Make throttle_is_valid() a bit less verbose

Use a pointer to the bucket instead of repeating cfg->buckets[i] all
the time. This makes the code more concise and will help us expand the
checks later and save a few line breaks.

Signed-off-by: Alberto Garcia <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 2a8be39ebad013e506e31b069ddcce8993a957bf
      
https://github.com/qemu/qemu/commit/2a8be39ebad013e506e31b069ddcce8993a957bf
  Author: Alberto Garcia <address@hidden>
  Date:   2017-08-29 (Tue, 29 Aug 2017)

  Changed paths:
    M util/throttle.c

  Log Message:
  -----------
  throttle: Remove throttle_fix_bucket() / throttle_unfix_bucket()

The throttling code can change internally the value of bkt->max if it
hasn't been set by the user. The problem with this is that if we want
to retrieve the original value we have to undo this change first. This
is ugly and unnecessary: this patch removes the throttle_fix_bucket()
and throttle_unfix_bucket() functions completely and moves the logic
to throttle_compute_wait().

Signed-off-by: Alberto Garcia <address@hidden>
Reviewed-by: Manos Pitsidianakis <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: d00e6923b1e2c1bec7840b0a0706764493648527
      
https://github.com/qemu/qemu/commit/d00e6923b1e2c1bec7840b0a0706764493648527
  Author: Alberto Garcia <address@hidden>
  Date:   2017-08-29 (Tue, 29 Aug 2017)

  Changed paths:
    M include/qemu/throttle.h
    M tests/test-throttle.c
    M util/throttle.c

  Log Message:
  -----------
  throttle: Make LeakyBucket.avg and LeakyBucket.max integer types

Both the throttling limits set with the throttling.iops-* and
throttling.bps-* options and their QMP equivalents defined in the
BlockIOThrottle struct are integer values.

Those limits are also reported in the BlockDeviceInfo struct and they
are integers there as well.

Therefore there's no reason to store them internally as double and do
the conversion everytime we're setting or querying them, so this patch
uses uint64_t for those types. Let's also use an unsigned type because
we don't allow negative values anyway.

LeakyBucket.level and LeakyBucket.burst_level do however remain double
because their value changes depending on the fraction of time elapsed
since the previous I/O operation.

Signed-off-by: Alberto Garcia <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 67335a4558d3cad2173aac0ce13b6c096b077c41
      
https://github.com/qemu/qemu/commit/67335a4558d3cad2173aac0ce13b6c096b077c41
  Author: Alberto Garcia <address@hidden>
  Date:   2017-08-29 (Tue, 29 Aug 2017)

  Changed paths:
    M include/qemu/throttle.h
    M util/throttle.c

  Log Message:
  -----------
  throttle: Make burst_length 64bit and add range checks

LeakyBucket.burst_length is defined as an unsigned integer but the
code never checks for overflows and it only makes sure that the value
is not 0.

In practice this means that the user can set something like
throttling.iops-total-max-length=4294967300 despite being larger than
UINT_MAX and the final value after casting to unsigned int will be 4.

This patch changes the data type to uint64_t. This does not increase
the storage size of LeakyBucket, and allows us to assign the value
directly from qemu_opt_get_number() or BlockIOThrottle and then do the
checks directly in throttle_is_valid().

The value of burst_length does not have a specific upper limit,
but since the bucket size is defined by max * burst_length we have
to prevent overflows. Instead of going for UINT64_MAX or something
similar this patch reuses THROTTLE_VALUE_MAX, which allows I/O bursts
of 1 GiB/s for 10 days in a row.

Signed-off-by: Alberto Garcia <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: d942feec979b1ad669ba43b8c9540a7e3cf75a3f
      
https://github.com/qemu/qemu/commit/d942feec979b1ad669ba43b8c9540a7e3cf75a3f
  Author: Alberto Garcia <address@hidden>
  Date:   2017-08-29 (Tue, 29 Aug 2017)

  Changed paths:
    M tests/test-throttle.c

  Log Message:
  -----------
  throttle: Test the valid range of config values

Signed-off-by: Alberto Garcia <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: e916a6e88a4ff6c39cd6f62fb162a561c6b89de8
      
https://github.com/qemu/qemu/commit/e916a6e88a4ff6c39cd6f62fb162a561c6b89de8
  Author: Eduardo Habkost <address@hidden>
  Date:   2017-08-30 (Wed, 30 Aug 2017)

  Changed paths:
    M util/oslib-posix.c

  Log Message:
  -----------
  oslib-posix: Print errors before aborting on qemu_alloc_stack()

If QEMU is running on a system that's out of memory and mmap()
fails, QEMU aborts with no error message at all, making it hard
to debug the reason for the failure.

Add perror() calls that will print error information before
aborting.

Signed-off-by: Eduardo Habkost <address@hidden>
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
Tested-by: Philippe Mathieu-Daudé <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: c3a8fe331eb4310e2d058f1c15a817d255840323
      
https://github.com/qemu/qemu/commit/c3a8fe331eb4310e2d058f1c15a817d255840323
  Author: Alberto Garcia <address@hidden>
  Date:   2017-08-30 (Wed, 30 Aug 2017)

  Changed paths:
    M block/qcow.c
    M block/qcow2.c
    M dump.c

  Log Message:
  -----------
  misc: Remove unused Error variables

There's a few cases which we're passing an Error pointer to a function
only to discard it immediately afterwards without checking it. In
these cases we can simply remove the variable and pass NULL instead.

Signed-off-by: Alberto Garcia <address@hidden>
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 47e1cb1f0a130baa438d895eff5d05004c9a9aa2
      
https://github.com/qemu/qemu/commit/47e1cb1f0a130baa438d895eff5d05004c9a9aa2
  Author: Stefan Hajnoczi <address@hidden>
  Date:   2017-08-30 (Wed, 30 Aug 2017)

  Changed paths:
    A COPYING.PYTHON
    A scripts/argparse.py

  Log Message:
  -----------
  scripts: add argparse module for Python 2.6 compatibility

The minimum Python version supported by QEMU is 2.6.  The argparse
standard library module was only added in Python 2.7.  Many scripts
would like to use argparse because it supports command-line
sub-commands.

This patch adds argparse.  See the top of argparse.py for details.

Suggested-by: Daniel P. Berrange <address@hidden>
Signed-off-by: Stefan Hajnoczi <address@hidden>
Acked-by: John Snow <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: c2d3189667409561772e8c1e5615c5166cd8aa2c
      
https://github.com/qemu/qemu/commit/c2d3189667409561772e8c1e5615c5166cd8aa2c
  Author: Stefan Hajnoczi <address@hidden>
  Date:   2017-08-30 (Wed, 30 Aug 2017)

  Changed paths:
    M tests/docker/docker.py

  Log Message:
  -----------
  docker.py: Python 2.6 argparse compatibility

Add the scripts/ directory to sys.path so Python 2.6 will be able to
import argparse.

Cc: Fam Zheng <address@hidden>
Signed-off-by: Stefan Hajnoczi <address@hidden>
Acked-by: John Snow <address@hidden>
Acked-by: Fam Zheng <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 0ea47d0f36112f0f38661e2e430edf32737c7f43
      
https://github.com/qemu/qemu/commit/0ea47d0f36112f0f38661e2e430edf32737c7f43
  Author: Stefan Hajnoczi <address@hidden>
  Date:   2017-08-30 (Wed, 30 Aug 2017)

  Changed paths:
    M tests/migration/guestperf/shell.py

  Log Message:
  -----------
  tests: migration/guestperf Python 2.6 argparse compatibility

Add the scripts/ directory to sys.path so Python 2.6 will be able to
import argparse.

Cc: Daniel P. Berrange <address@hidden>
Signed-off-by: Stefan Hajnoczi <address@hidden>
Acked-by: John Snow <address@hidden>
Acked-by: Fam Zheng <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: fb9429ddc124c46731185b2781804d4fd39b658c
      
https://github.com/qemu/qemu/commit/fb9429ddc124c46731185b2781804d4fd39b658c
  Author: Fred Rolland <address@hidden>
  Date:   2017-08-30 (Wed, 30 Aug 2017)

  Changed paths:
    M qemu-doc.texi

  Log Message:
  -----------
  qemu-doc: Add UUID support in initiator name

Update doc with the usage of UUID for initiator name.

Related-To: https://bugzilla.redhat.com/1006468
Signed-off-by: Fred Rolland <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 3e4c705212abfe8c9882a00beb2d1466a8a53cec
      
https://github.com/qemu/qemu/commit/3e4c705212abfe8c9882a00beb2d1466a8a53cec
  Author: Stefan Hajnoczi <address@hidden>
  Date:   2017-08-30 (Wed, 30 Aug 2017)

  Changed paths:
    M block/qcow2-cluster.c
    M block/qcow2.c

  Log Message:
  -----------
  qcow2: allocate cluster_cache/cluster_data on demand

Most qcow2 files are uncompressed so it is wasteful to allocate (32 + 1)
* cluster_size + 512 bytes upfront.  Allocate s->cluster_cache and
s->cluster_data when the first read operation is performance on a
compressed cluster.

The buffers are freed in .bdrv_close().  .bdrv_open() no longer has any
code paths that can allocate these buffers, so remove the free functions
in the error code path.

This patch can result in significant memory savings when many qcow2
disks are attached or backing file chains are long:

Before 12.81% (1,023,193,088B)
After   5.36% (393,893,888B)

Reported-by: Alexey Kardashevskiy <address@hidden>
Tested-by: Alexey Kardashevskiy <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Signed-off-by: Stefan Hajnoczi <address@hidden>
Message-id: address@hidden
Cc: Kevin Wolf <address@hidden>
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 1d2a8e0690a8b26833346c6e84e91773da66fbf4
      
https://github.com/qemu/qemu/commit/1d2a8e0690a8b26833346c6e84e91773da66fbf4
  Author: Peter Maydell <address@hidden>
  Date:   2017-08-31 (Thu, 31 Aug 2017)

  Changed paths:
    A COPYING.PYTHON
    M block/qcow.c
    M block/qcow2-cluster.c
    M block/qcow2.c
    M dump.c
    M hw/block/nvme.c
    M include/qemu/throttle.h
    M qemu-doc.texi
    A scripts/argparse.py
    M tests/docker/docker.py
    M tests/migration/guestperf/shell.py
    M tests/test-throttle.c
    M util/oslib-posix.c
    M util/throttle.c

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into 
staging

# gpg: Signature made Thu 31 Aug 2017 09:21:49 BST
# gpg:                using RSA key 0x9CA4ABB381AB73C8
# gpg: Good signature from "Stefan Hajnoczi <address@hidden>"
# gpg:                 aka "Stefan Hajnoczi <address@hidden>"
# Primary key fingerprint: 8695 A8BF D3F9 7CDA AC35  775A 9CA4 ABB3 81AB 73C8

* remotes/stefanha/tags/block-pull-request:
  qcow2: allocate cluster_cache/cluster_data on demand
  qemu-doc: Add UUID support in initiator name
  tests: migration/guestperf Python 2.6 argparse compatibility
  docker.py: Python 2.6 argparse compatibility
  scripts: add argparse module for Python 2.6 compatibility
  misc: Remove unused Error variables
  oslib-posix: Print errors before aborting on qemu_alloc_stack()
  throttle: Test the valid range of config values
  throttle: Make burst_length 64bit and add range checks
  throttle: Make LeakyBucket.avg and LeakyBucket.max integer types
  throttle: Remove throttle_fix_bucket() / throttle_unfix_bucket()
  throttle: Make throttle_is_valid() a bit less verbose
  throttle: Update the throttle_fix_bucket() documentation
  throttle: Fix wrong variable name in the header documentation
  nvme: Fix get/set number of queues feature, again

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/2e75021eb644...1d2a8e0690a8