jcs.org / qemu
qemu with hax to log dma reads & writes jcs.org/2018/11/12/vfio
fork atom

throttle: Fix crash on reopen

The throttle block filter can be reopened, and with this it is
possible to change the throttle group that the filter belongs to.

The way the code does that is the following:

- On throttle_reopen_prepare(): create a new ThrottleGroupMember
and attach it to the new throttle group.

- On throttle_reopen_commit(): detach the old ThrottleGroupMember,
delete it and replace it with the new one.

The problem with this is that by replacing the ThrottleGroupMember the
previous value of io_limits_disabled is lost, causing an assertion
failure in throttle_co_drain_end().

This problem can be reproduced by reopening a throttle node:

$QEMU -monitor stdio
-object throttle-group,id=tg0,x-iops-total=1000 \
-blockdev node-name=hd0,driver=qcow2,file.driver=file,file.filename=hd.qcow2 \
-blockdev node-name=root,driver=throttle,throttle-group=tg0,file=hd0,read-only=on

(qemu) block_stream root
block/throttle.c:214: throttle_co_drain_end: Assertion `tgm->io_limits_disabled' failed.

Since we only want to change the throttle group on reopen there's no
need to create a ThrottleGroupMember and discard the old one. It's
easier if we simply detach it from its current group and attach it to
the new one.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-id: 20180608151536.7378-1-berto@igalia.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
(cherry picked from commit bc33c047d1ec0b35c9cd8be62bcefae2da28654f)
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

authored by

Alberto Garcia and committed by
Michael Roth ef67e673 081eac8b

+33 -21
+33 -21
block/throttle.c
··· 36 36 }, 37 37 }; 38 38 39 - static int throttle_configure_tgm(BlockDriverState *bs, 40 - ThrottleGroupMember *tgm, 41 - QDict *options, Error **errp) 39 + /* 40 + * If this function succeeds then the throttle group name is stored in 41 + * @group and must be freed by the caller. 42 + * If there's an error then @group remains unmodified. 43 + */ 44 + static int throttle_parse_options(QDict *options, char **group, Error **errp) 42 45 { 43 46 int ret; 44 47 const char *group_name; ··· 63 66 goto fin; 64 67 } 65 68 66 - /* Register membership to group with name group_name */ 67 - throttle_group_register_tgm(tgm, group_name, bdrv_get_aio_context(bs)); 69 + *group = g_strdup(group_name); 68 70 ret = 0; 69 71 fin: 70 72 qemu_opts_del(opts); ··· 75 77 int flags, Error **errp) 76 78 { 77 79 ThrottleGroupMember *tgm = bs->opaque; 80 + char *group; 81 + int ret; 78 82 79 83 bs->file = bdrv_open_child(NULL, options, "file", bs, 80 84 &child_file, false, errp); ··· 84 88 bs->supported_write_flags = bs->file->bs->supported_write_flags; 85 89 bs->supported_zero_flags = bs->file->bs->supported_zero_flags; 86 90 87 - return throttle_configure_tgm(bs, tgm, options, errp); 91 + ret = throttle_parse_options(options, &group, errp); 92 + if (ret == 0) { 93 + /* Register membership to group with name group_name */ 94 + throttle_group_register_tgm(tgm, group, bdrv_get_aio_context(bs)); 95 + g_free(group); 96 + } 97 + 98 + return ret; 88 99 } 89 100 90 101 static void throttle_close(BlockDriverState *bs) ··· 160 171 static int throttle_reopen_prepare(BDRVReopenState *reopen_state, 161 172 BlockReopenQueue *queue, Error **errp) 162 173 { 163 - ThrottleGroupMember *tgm; 174 + int ret; 175 + char *group = NULL; 164 176 165 177 assert(reopen_state != NULL); 166 178 assert(reopen_state->bs != NULL); 167 179 168 - reopen_state->opaque = g_new0(ThrottleGroupMember, 1); 169 - tgm = reopen_state->opaque; 170 - 171 - return throttle_configure_tgm(reopen_state->bs, tgm, reopen_state->options, 172 - errp); 180 + ret = throttle_parse_options(reopen_state->options, &group, errp); 181 + reopen_state->opaque = group; 182 + return ret; 173 183 } 174 184 175 185 static void throttle_reopen_commit(BDRVReopenState *reopen_state) 176 186 { 177 - ThrottleGroupMember *old_tgm = reopen_state->bs->opaque; 178 - ThrottleGroupMember *new_tgm = reopen_state->opaque; 187 + BlockDriverState *bs = reopen_state->bs; 188 + ThrottleGroupMember *tgm = bs->opaque; 189 + char *group = reopen_state->opaque; 190 + 191 + assert(group); 179 192 180 - throttle_group_unregister_tgm(old_tgm); 181 - g_free(old_tgm); 182 - reopen_state->bs->opaque = new_tgm; 193 + if (strcmp(group, throttle_group_get_name(tgm))) { 194 + throttle_group_unregister_tgm(tgm); 195 + throttle_group_register_tgm(tgm, group, bdrv_get_aio_context(bs)); 196 + } 197 + g_free(reopen_state->opaque); 183 198 reopen_state->opaque = NULL; 184 199 } 185 200 186 201 static void throttle_reopen_abort(BDRVReopenState *reopen_state) 187 202 { 188 - ThrottleGroupMember *tgm = reopen_state->opaque; 189 - 190 - throttle_group_unregister_tgm(tgm); 191 - g_free(tgm); 203 + g_free(reopen_state->opaque); 192 204 reopen_state->opaque = NULL; 193 205 } 194 206