subject:"\[Qemu\-devel\] \[PATCH\] rbd\: Detect rbd image resizes and propagate them"

Re: [Qemu-devel] [PATCH] rbd: Detect rbd image resizes and propagate them

2017-09-15 Thread Eric Blake

On 09/15/2017 07:33 AM, Kevin Wolf wrote:
> Am 13.09.2017 um 18:44 hat Adam Wolfe Gordon geschrieben:
>> Register a watcher with rbd so that we get notified when an image is
>> resized. Propagate resizes to parent block devices so that guest devices
>> get resized without user intervention.
>>
>> Signed-off-by: Adam Wolfe Gordon 
>> ---

> 
> There's more wrong about this than just making assumptions about the
> graph layout above our own node (which would be wrong enough already).
> 
> Other assumptions that you are making here and that don't hold true
> generally are that there is only one parent node (no reason why the
> image couldn't be used by two different users) and that we always
> inherit from a parent. If this node was created explicitly by the user
> with its own -blockdev (or -drive) option, it doesn't inherit options
> from any of its parents.
> 
> Basically, a block driver shouldn't care about its users and it can't
> access them in a clean way. This is intentional.
> 
>> +if (parent == NULL) {
>> +error_report("bs %s does not have parent", 
>> bdrv_get_device_or_node_name(bs));
>> +return;
>> +}
>> +
>> +/* Force parents to re-read our size. */
>> +was_variable_length = bs->drv->has_variable_length;
>> +bs->drv->has_variable_length = true;
>> +new_parent_len = bdrv_getlength(parent);
>> +if (new_parent_len < 0) {
>> +error_report("getlength failed on parent %s", 
>> bdrv_get_device_or_node_name(parent));
>> +bs->drv->has_variable_length = was_variable_length;
>> +return;
>> +}
>> +bs->drv->has_variable_length = was_variable_length;
>> +
>> +/* Notify block backends that that we have resized.
>> +   Copied from bdrv_parent_cb_resize. */
>> +QLIST_FOREACH(c, >parents, next_parent) {
>> +if (c->role->resize) {
>> +c->role->resize(c);
>> +}
>> +}
> 
> This is the right approach that you should use consistently instead.

Also, we need to fix existing bugs in block.c first; right now,
bdrv_truncate() forgets that refresh_total_sectors() can fail, and then
calls bdrv_dirty_bitmap_truncate() even though that will act on the
wrong length.  If we are going to allow devices to recognize that they
have been externally resized outside of qemu's control, then we need to
make sure that we react to size updates consistently.

I'm trying to fix that [1], but it may interact with what you are trying
to do here.

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg03776.html

-- 
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-devel] [PATCH] rbd: Detect rbd image resizes and propagate them

2017-09-15 Thread Kevin Wolf

Am 13.09.2017 um 18:44 hat Adam Wolfe Gordon geschrieben:
> Register a watcher with rbd so that we get notified when an image is
> resized. Propagate resizes to parent block devices so that guest devices
> get resized without user intervention.
> 
> Signed-off-by: Adam Wolfe Gordon 
> ---
> Hello!
> 
> We are using this patch in production at DigitalOcean and have tested it 
> fairly
> extensively. However, we use the same block device configuration everywhere, 
> so
> I'd be interested to get an answer to the question I've left in the code:
> 
> > /* NOTE: This assumes there's only one layer between us and the
> >block-backend. Is this always true? */
> 
> If that isn't true, this patch will need a bit of work to traverse up the 
> block
> device tree and find the block-backend.
> 
> Of course, any other feedback would be welcome too - this is my first foray 
> into
> the qemu code.
> 
> Regards,
> Adam
> 
>  block/rbd.c | 80 
> +
>  1 file changed, 80 insertions(+)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 144f350e1f..1c9fcbec1f 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -96,6 +96,8 @@ typedef struct BDRVRBDState {
>  rbd_image_t image;
>  char *image_name;
>  char *snap;
> +uint64_t watcher_handle;
> +uint64_t size_bytes;
>  } BDRVRBDState;
>  
>  static char *qemu_rbd_next_tok(char *src, char delim, char **p)
> @@ -540,6 +542,67 @@ out:
>  return rados_str;
>  }
>  
> +/* BH for when rbd notifies us of an update. */
> +static void qemu_rbd_update_bh(void *arg)
> +{
> +BlockDriverState *parent, *bs = arg;
> +BDRVRBDState *s = bs->opaque;
> +bool was_variable_length;
> +uint64_t new_size_bytes;
> +int64_t new_parent_len;
> +BdrvChild *c;
> +int r;
> +
> +r = rbd_get_size(s->image, _size_bytes);
> +if (r < 0) {
> +error_report("error reading size for %s: %s", s->name, strerror(-r));
> +return;
> +}
> +
> +/* Avoid no-op resizes on non-resize notifications. */
> +if (new_size_bytes == s->size_bytes) {
> +error_printf("skipping non-resize rbd cb\n");
> +return;
> +}
> +
> +/* NOTE: This assumes there's only one layer between us and the
> +   block-backend. Is this always true? */
> +parent = bs->inherits_from;

There's more wrong about this than just making assumptions about the
graph layout above our own node (which would be wrong enough already).

Other assumptions that you are making here and that don't hold true
generally are that there is only one parent node (no reason why the
image couldn't be used by two different users) and that we always
inherit from a parent. If this node was created explicitly by the user
with its own -blockdev (or -drive) option, it doesn't inherit options
from any of its parents.

Basically, a block driver shouldn't care about its users and it can't
access them in a clean way. This is intentional.

> +if (parent == NULL) {
> +error_report("bs %s does not have parent", 
> bdrv_get_device_or_node_name(bs));
> +return;
> +}
> +
> +/* Force parents to re-read our size. */
> +was_variable_length = bs->drv->has_variable_length;
> +bs->drv->has_variable_length = true;
> +new_parent_len = bdrv_getlength(parent);
> +if (new_parent_len < 0) {
> +error_report("getlength failed on parent %s", 
> bdrv_get_device_or_node_name(parent));
> +bs->drv->has_variable_length = was_variable_length;
> +return;
> +}
> +bs->drv->has_variable_length = was_variable_length;
> +
> +/* Notify block backends that that we have resized.
> +   Copied from bdrv_parent_cb_resize. */
> +QLIST_FOREACH(c, >parents, next_parent) {
> +if (c->role->resize) {
> +c->role->resize(c);
> +}
> +}

This is the right approach that you should use consistently instead.

You don't only notify BlockBackends here (you do so if there is no layer
between rbd and the BlockBackend, i.e. rbd is the root node), but you
also notify any parent nodes (BlockDriverStates), which can react to
this.

child_file/child_format don't currently handle .resize, so if you have
any layers between rbd and the BB, this code doesn't do anything at the
moment.

However, you could introduce an implementation of .resize. An important
point here is that it depends on the specific block driver what to do
with this notification. Maybe we need a new BlockDriver callback. Raw
images will just propagate this and call .resize on all of their
parents. For qcow2 images, the size change of the image file doesn't
make a difference for the virtual disk that the driver exposes, so it
will silently ignore the notification.

Kevin

Re: [Qemu-devel] [PATCH] rbd: Detect rbd image resizes and propagate them

2017-09-13 Thread no-reply

Hi,

This series failed build test on s390x host. Please find the details below.

Subject: [Qemu-devel] [PATCH] rbd: Detect rbd image resizes and propagate them
Message-id: 20170913164424.32129-1-...@digitalocean.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
echo -n "Using CC: "
realpath $CC
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]  patchew/1505298088-10878-1-git-send-email-th...@redhat.com 
-> patchew/1505298088-10878-1-git-send-email-th...@redhat.com
 * [new tag] patchew/20170913164424.32129-1-...@digitalocean.com -> 
patchew/20170913164424.32129-1-...@digitalocean.com
Switched to a new branch 'test'
c0e97b7 rbd: Detect rbd image resizes and propagate them

=== OUTPUT BEGIN ===
=== ENV ===
XDG_SESSION_ID=862
SHELL=/bin/sh
USER=fam
PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug
PATH=/usr/bin:/bin
PWD=/var/tmp/patchew-tester-tmp-ihkih90_/src
LANG=en_US.UTF-8
HOME=/home/fam
SHLVL=2
LOGNAME=fam
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus
XDG_RUNTIME_DIR=/run/user/1012
_=/usr/bin/env
=== PACKAGES ===
gpg-pubkey-873529b8-54e386ff
xz-libs-5.2.2-2.fc24.s390x
libxshmfence-1.2-3.fc24.s390x
giflib-4.1.6-15.fc24.s390x
trousers-lib-0.3.13-6.fc24.s390x
ncurses-base-6.0-6.20160709.fc25.noarch
gmp-6.1.1-1.fc25.s390x
libidn-1.33-1.fc25.s390x
slang-2.3.0-7.fc25.s390x
pkgconfig-0.29.1-1.fc25.s390x
alsa-lib-1.1.1-2.fc25.s390x
yum-metadata-parser-1.1.4-17.fc25.s390x
python3-slip-dbus-0.6.4-4.fc25.noarch
python2-cssselect-0.9.2-1.fc25.noarch
createrepo_c-libs-0.10.0-6.fc25.s390x
initscripts-9.69-1.fc25.s390x
parted-3.2-21.fc25.s390x
flex-2.6.0-3.fc25.s390x
colord-libs-1.3.4-1.fc25.s390x
python-osbs-client-0.33-3.fc25.noarch
perl-Pod-Simple-3.35-1.fc25.noarch
python2-simplejson-3.10.0-1.fc25.s390x
brltty-5.4-2.fc25.s390x
librados2-10.2.4-2.fc25.s390x
tcp_wrappers-7.6-83.fc25.s390x
libcephfs_jni1-10.2.4-2.fc25.s390x
nettle-devel-3.3-1.fc25.s390x
bzip2-devel-1.0.6-21.fc25.s390x
libuuid-2.28.2-2.fc25.s390x
python3-dnf-1.1.10-6.fc25.noarch
texlive-kpathsea-doc-svn41139-33.fc25.1.noarch
openssh-7.4p1-4.fc25.s390x
texlive-kpathsea-bin-svn40473-33.20160520.fc25.1.s390x
texlive-graphics-svn41015-33.fc25.1.noarch
texlive-dvipdfmx-def-svn40328-33.fc25.1.noarch
texlive-mfware-svn40768-33.fc25.1.noarch
texlive-texlive-scripts-svn41433-33.fc25.1.noarch
texlive-euro-svn22191.1.1-33.fc25.1.noarch
texlive-etex-svn37057.0-33.fc25.1.noarch
texlive-iftex-svn29654.0.2-33.fc25.1.noarch
texlive-palatino-svn31835.0-33.fc25.1.noarch
texlive-texlive-docindex-svn41430-33.fc25.1.noarch
texlive-xunicode-svn30466.0.981-33.fc25.1.noarch
texlive-koma-script-svn41508-33.fc25.1.noarch
texlive-pst-grad-svn15878.1.06-33.fc25.1.noarch
texlive-pst-blur-svn15878.2.0-33.fc25.1.noarch
texlive-jknapltx-svn19440.0-33.fc25.1.noarch
texinfo-6.1-4.fc25.s390x
openssl-devel-1.0.2k-1.fc25.s390x
jansson-2.10-2.fc25.s390x
fedora-repos-25-4.noarch
python3-libs-3.5.3-6.fc25.s390x
perl-Errno-1.25-387.fc25.s390x
acl-2.2.52-13.fc25.s390x
systemd-pam-231-17.fc25.s390x
NetworkManager-libnm-1.4.4-5.fc25.s390x
poppler-0.45.0-5.fc25.s390x
ccache-3.3.4-1.fc25.s390x
valgrind-3.12.0-9.fc25.s390x
perl-open-1.10-387.fc25.noarch
libgcc-6.4.1-1.fc25.s390x
libsoup-2.56.1-1.fc25.s390x
libstdc++-devel-6.4.1-1.fc25.s390x
nss-softokn-devel-3.31.0-1.0.fc25.s390x
libobjc-6.4.1-1.fc25.s390x
python2-rpm-4.13.0.1-2.fc25.s390x
python2-gluster-3.10.5-1.fc25.s390x
rpm-build-4.13.0.1-2.fc25.s390x
glibc-static-2.24-10.fc25.s390x
lz4-1.8.0-1.fc25.s390x
xapian-core-libs-1.2.24-1.fc25.s390x
elfutils-libelf-devel-0.169-1.fc25.s390x
libaio-0.3.110-6.fc24.s390x
libfontenc-1.1.3-3.fc24.s390x
lzo-2.08-8.fc24.s390x
isl-0.14-5.fc24.s390x
libXau-1.0.8-6.fc24.s390x
linux-atm-libs-2.5.1-14.fc24.s390x
libXext-1.3.3-4.fc24.s390x
libXxf86vm-1.1.4-3.fc24.s390x
bison-3.0.4-4.fc24.s390x
perl-srpm-macros-1-20.fc25.noarch
gawk-4.1.3-8.fc25.s390x
libwayland-client-1.12.0-1.fc25.s390x
perl-Exporter-5.72-366.fc25.noarch
perl-version-0.99.17-1.fc25.s390x
fftw-libs-double-3.3.5-3.fc25.s390x
libssh2-1.8.0-1.fc25.s390x
ModemManager-glib-1.6.4-1.fc25.s390x
newt-python3-0.52.19-2.fc25.s390x
python-munch-2.0.4-3.fc25.noarch
python-bugzilla-1.2.2-4.fc25.noarch
libedit-3.1-16.20160618cvs.fc25.s390x
createrepo_c-0.10.0-6.fc25.s390x
device-mapper-multipath-libs-0.4.9-83.fc25.s390x
yum-3.4.3-510.fc25.noarch
mozjs17-17.0.0-16.fc25.s390x
libselinux-2.5-13.fc25.s390x
python2-pyparsing-2.1.10-1.fc25

Re: [Qemu-devel] [PATCH] rbd: Detect rbd image resizes and propagate them

2017-09-13 Thread no-reply

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH] rbd: Detect rbd image resizes and propagate them
Message-id: 20170913164424.32129-1-...@digitalocean.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]
patchew/1505298088-10878-1-git-send-email-th...@redhat.com -> 
patchew/1505298088-10878-1-git-send-email-th...@redhat.com
 * [new tag]   patchew/20170913164424.32129-1-...@digitalocean.com 
-> patchew/20170913164424.32129-1-...@digitalocean.com
Switched to a new branch 'test'
c0e97b7e5b rbd: Detect rbd image resizes and propagate them

=== OUTPUT BEGIN ===
Checking PATCH 1/1: rbd: Detect rbd image resizes and propagate them...
WARNING: line over 80 characters
#57: FILE: block/rbd.c:572:
+error_report("bs %s does not have parent", 
bdrv_get_device_or_node_name(bs));

ERROR: line over 90 characters
#66: FILE: block/rbd.c:581:
+error_report("getlength failed on parent %s", 
bdrv_get_device_or_node_name(parent));

total: 1 errors, 1 warnings, 107 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-devel] [PATCH] rbd: Detect rbd image resizes and propagate them

2017-09-13 Thread Adam Wolfe Gordon via Qemu-devel

Register a watcher with rbd so that we get notified when an image is
resized. Propagate resizes to parent block devices so that guest devices
get resized without user intervention.

Signed-off-by: Adam Wolfe Gordon 
---
Hello!

We are using this patch in production at DigitalOcean and have tested it fairly
extensively. However, we use the same block device configuration everywhere, so
I'd be interested to get an answer to the question I've left in the code:

> /* NOTE: This assumes there's only one layer between us and the
>block-backend. Is this always true? */

If that isn't true, this patch will need a bit of work to traverse up the block
device tree and find the block-backend.

Of course, any other feedback would be welcome too - this is my first foray into
the qemu code.

Regards,
Adam

 block/rbd.c | 80 +
 1 file changed, 80 insertions(+)

diff --git a/block/rbd.c b/block/rbd.c
index 144f350e1f..1c9fcbec1f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -96,6 +96,8 @@ typedef struct BDRVRBDState {
 rbd_image_t image;
 char *image_name;
 char *snap;
+uint64_t watcher_handle;
+uint64_t size_bytes;
 } BDRVRBDState;
 
 static char *qemu_rbd_next_tok(char *src, char delim, char **p)
@@ -540,6 +542,67 @@ out:
 return rados_str;
 }
 
+/* BH for when rbd notifies us of an update. */
+static void qemu_rbd_update_bh(void *arg)
+{
+BlockDriverState *parent, *bs = arg;
+BDRVRBDState *s = bs->opaque;
+bool was_variable_length;
+uint64_t new_size_bytes;
+int64_t new_parent_len;
+BdrvChild *c;
+int r;
+
+r = rbd_get_size(s->image, _size_bytes);
+if (r < 0) {
+error_report("error reading size for %s: %s", s->name, strerror(-r));
+return;
+}
+
+/* Avoid no-op resizes on non-resize notifications. */
+if (new_size_bytes == s->size_bytes) {
+error_printf("skipping non-resize rbd cb\n");
+return;
+}
+
+/* NOTE: This assumes there's only one layer between us and the
+   block-backend. Is this always true? */
+parent = bs->inherits_from;
+if (parent == NULL) {
+error_report("bs %s does not have parent", 
bdrv_get_device_or_node_name(bs));
+return;
+}
+
+/* Force parents to re-read our size. */
+was_variable_length = bs->drv->has_variable_length;
+bs->drv->has_variable_length = true;
+new_parent_len = bdrv_getlength(parent);
+if (new_parent_len < 0) {
+error_report("getlength failed on parent %s", 
bdrv_get_device_or_node_name(parent));
+bs->drv->has_variable_length = was_variable_length;
+return;
+}
+bs->drv->has_variable_length = was_variable_length;
+
+/* Notify block backends that that we have resized.
+   Copied from bdrv_parent_cb_resize. */
+QLIST_FOREACH(c, >parents, next_parent) {
+if (c->role->resize) {
+c->role->resize(c);
+}
+}
+
+s->size_bytes = new_size_bytes;
+}
+
+/* Called from non-qemu thread - careful! */
+static void qemu_rbd_update_cb(void *arg)
+{
+BlockDriverState *bs = arg;
+
+aio_bh_schedule_oneshot(bdrv_get_aio_context(bs), qemu_rbd_update_bh, bs);
+}
+
 static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
 {
@@ -672,9 +735,25 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
 }
 }
 
+r = rbd_update_watch(s->image, >watcher_handle, qemu_rbd_update_cb, bs);
+if (r < 0) {
+error_setg_errno(errp, -r, "error registering watcher on %s", s->name);
+goto failed_watch;
+}
+
+r = rbd_get_size(s->image, >size_bytes);
+if (r < 0) {
+error_setg_errno(errp, -r, "error reading size for %s", s->name);
+goto failed_sz;
+}
+
 qemu_opts_del(opts);
 return 0;
 
+failed_sz:
+rbd_update_unwatch(s->image, s->watcher_handle);
+failed_watch:
+rbd_close(s->image);
 failed_open:
 rados_ioctx_destroy(s->io_ctx);
 failed_shutdown:
@@ -712,6 +791,7 @@ static void qemu_rbd_close(BlockDriverState *bs)
 {
 BDRVRBDState *s = bs->opaque;
 
+rbd_update_unwatch(s->image, s->watcher_handle);
 rbd_close(s->image);
 rados_ioctx_destroy(s->io_ctx);
 g_free(s->snap);
-- 
2.14.1

Re: [Qemu-devel] [PATCH] rbd: Detect rbd image resizes and propagate them

Re: [Qemu-devel] [PATCH] rbd: Detect rbd image resizes and propagate them

Re: [Qemu-devel] [PATCH] rbd: Detect rbd image resizes and propagate them

Re: [Qemu-devel] [PATCH] rbd: Detect rbd image resizes and propagate them

[Qemu-devel] [PATCH] rbd: Detect rbd image resizes and propagate them

5 matches

Site Navigation

Mail list logo

Footer information