[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 08/13] block: Allow changing the backing file on
From: |
Alberto Garcia |
Subject: |
Re: [Qemu-block] [PATCH 08/13] block: Allow changing the backing file on reopen |
Date: |
Thu, 21 Feb 2019 15:46:36 +0100 |
User-agent: |
Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu) |
On Tue 12 Feb 2019 06:27:56 PM CET, Kevin Wolf wrote:
>> - In bdrv_reopen_commit(): perform the actual node replacement by
>> calling bdrv_set_backing_hd(). It may happen that there are
>> temporary implicit nodes between the BDS that we are reopening and
>> the backing file that we want to replace (e.g. a commit fiter node),
>> so we must skip them.
>
> Hmm... I really dislike getting into the business of dealing with
> implicit nodes here. If you want to manage the block graph at the node
> level, you should manage all of it and just avoid getting implicit
> nodes in the first place. Otherwise, we'd have to guess where in the
> implicit chain you really want to add or remove nodes, and we're bound
> to guess wrong for some users.
>
> There is one problem with not skipping implicit nodes, though: Even if
> you don't want to manage things at the node level, we already force
> you to specify 'backing'. If there are implicit nodes, you don't knwo
> the real node name of the first backing child.
>
> So I suggest that we allow skipping implicit nodes for the purpose of
> leaving the backing link unchanged; but we return an error if you want
> to change the backing link and there are implicit nodes in the way.
, that sounds good to me. It doesn't really affect any of the test
cases that I had
>
>> Although x-blockdev-reopen is meant to be used like blockdev-add,
>> there's an important thing that must be taken into account: the only
>> way to set a new backing file is by using a reference to an existing
>> node (previously added with e.g. blockdev-add). If 'backing' contains
>> a dictionary with a new set of options ({"driver": "qcow2", "file": {
>> ... }}) then it is interpreted that the _existing_ backing file must
>> be reopened with those options.
>>
>> Signed-off-by: Alberto Garcia <address@hidden>
>> ---
>> block.c | 124
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 122 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 897c8b85cd..10847416b2 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2909,6 +2909,27 @@ BlockDriverState *bdrv_open(const char *filename,
>> const char *reference,
>> }
>>
>> /*
>> + * Returns true if @child can be reached recursively from @bs
>> + */
>> +static bool bdrv_recurse_has_child(BlockDriverState *bs,
>> + BlockDriverState *child)
>> +{
>> + BdrvChild *c;
>> +
>> + if (bs == child) {
>> + return true;
>> + }
>> +
>> + QLIST_FOREACH(c, &bs->children, next) {
>> + if (bdrv_recurse_has_child(c->bs, child)) {
>> + return true;
>> + }
>> + }
>> +
>> + return false;
>> +}
>> +
>> +/*
>> * Adds a BlockDriverState to a simple queue for an atomic, transactional
>> * reopen of multiple devices.
>> *
>> @@ -3217,6 +3238,63 @@ static void bdrv_reopen_perm(BlockReopenQueue *q,
>> BlockDriverState *bs,
>> }
>>
>> /*
>> + * Return 0 if the 'backing' option of @bs can be replaced with
>> + * @value, otherwise return < 0 and set @errp.
>> + */
>> +static int bdrv_reopen_parse_backing(BlockDriverState *bs, QObject *value,
>> + Error **errp)
>> +{
>> + BlockDriverState *overlay_bs, *new_backing_bs;
>> + const char *str;
>> +
>> + switch (qobject_type(value)) {
>> + case QTYPE_QNULL:
>> + new_backing_bs = NULL;
>> + break;
>> + case QTYPE_QSTRING:
>> + str = qobject_get_try_str(value);
>> + new_backing_bs = bdrv_lookup_bs(NULL, str, errp);
>> + if (new_backing_bs == NULL) {
>> + return -EINVAL;
>> + } else if (bdrv_recurse_has_child(new_backing_bs, bs)) {
>> + error_setg(errp, "Making '%s' a backing file of '%s' "
>> + "would create a cycle", str, bs->node_name);
>> + return -EINVAL;
>> + }
>> + break;
>> + default:
>> + /* 'backing' does not allow any other data type */
>> + g_assert_not_reached();
>> + }
>> +
>> + if (new_backing_bs) {
>> + if (bdrv_get_aio_context(new_backing_bs) !=
>> bdrv_get_aio_context(bs)) {
>> + error_setg(errp, "Cannot use a new backing file "
>> + "with a different AioContext");
>> + return -EINVAL;
>> + }
>> + }
>
> This is okay for a first version, but in reality, you'd usually want to
> just move the backing file into the right AioContext. Of course, this is
> only correct if the backing file doesn't have other users in different
> AioContexts. To get a good implementation for this, we'd probably need
> to store the AioContext in BdrvChild, like we already concluded for
> other use cases.
>
> Maybe document this as one of the problems to be solved before we can
> remove the x- prefix.
>
>> +
>> + /*
>> + * Skip all links that point to an implicit node, if any
>> + * (e.g. a commit filter node). We don't want to change those.
>> + */
>> + overlay_bs = bs;
>> + while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
>> + overlay_bs = backing_bs(overlay_bs);
>> + }
>> +
>> + if (new_backing_bs != backing_bs(overlay_bs)) {
>> + if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs),
>> + errp)) {
>> + return -EPERM;
>> + }
>> + }
>
> Should this function check new_backing_bs->drv->supports_backing, too?
>
>> + return 0;
>> +}
>> +
>> +/*
>> * Prepares a BlockDriverState for reopen. All changes are staged in the
>> * 'opaque' field of the BDRVReopenState, which is used and allocated by
>> * the block driver layer .bdrv_reopen_prepare()
>> @@ -3359,6 +3437,19 @@ int bdrv_reopen_prepare(BDRVReopenState
>> *reopen_state, BlockReopenQueue *queue,
>> QObject *new = entry->value;
>> QObject *old = qdict_get(reopen_state->bs->options, entry->key);
>>
>> + /*
>> + * Allow changing the 'backing' option. The new value can be
>> + * either a reference to an existing node (using its node name)
>> + * or NULL to simply detach the current backing file.
>> + */
>> + if (!strcmp(entry->key, "backing")) {
>> + ret = bdrv_reopen_parse_backing(reopen_state->bs, new,
>> errp);
>> + if (ret < 0) {
>> + goto error;
>> + }
>> + continue; /* 'backing' option processed, go to the next one
>> */
>> + }
>> +
>> /* Allow child references (child_name=node_name) as long as they
>> * point to the current child (i.e. everything stays the same).
>> */
>> if (qobject_type(new) == QTYPE_QSTRING) {
>> @@ -3437,9 +3528,10 @@ error:
>> void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>> {
>> BlockDriver *drv;
>> - BlockDriverState *bs;
>> + BlockDriverState *bs, *new_backing_bs;
>> BdrvChild *child;
>> - bool old_can_write, new_can_write;
>> + bool old_can_write, new_can_write, change_backing_bs;
>> + QObject *qobj;
>>
>> assert(reopen_state != NULL);
>> bs = reopen_state->bs;
>> @@ -3464,6 +3556,15 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>> bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
>> bs->detect_zeroes = reopen_state->detect_zeroes;
>>
>> + qobj = qdict_get(bs->options, "backing");
>> + change_backing_bs = (qobj != NULL);
>> + if (change_backing_bs) {
>> + const char *str = qobject_get_try_str(qobj);
>> + new_backing_bs = str ? bdrv_find_node(str) : NULL;
>> + qdict_del(bs->explicit_options, "backing");
>> + qdict_del(bs->options, "backing");
>> + }
>> +
>> /* Remove child references from bs->options and bs->explicit_options.
>> * Child options were already removed in bdrv_reopen_queue_child() */
>> QLIST_FOREACH(child, &bs->children, next) {
>> @@ -3471,6 +3572,25 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>> qdict_del(bs->options, child->name);
>> }
>>
>> + /*
>> + * Change the backing file if a new one was specified. We do this
>> + * after updating bs->options, so bdrv_refresh_filename() (called
>> + * from bdrv_set_backing_hd()) has the new values.
>> + */
>> + if (change_backing_bs) {
>> + BlockDriverState *overlay = bs;
>> + /*
>> + * Skip all links that point to an implicit node, if any
>> + * (e.g. a commit filter node). We don't want to change those.
>> + */
>> + while (backing_bs(overlay) && backing_bs(overlay)->implicit) {
>> + overlay = backing_bs(overlay);
>> + }
>> + if (new_backing_bs != backing_bs(overlay)) {
>> + bdrv_set_backing_hd(overlay, new_backing_bs, &error_abort);
>
> I'm afraid we can't use &error_abort here because bdrv_attach_child()
> could still fail due to permission conflicts.
>
>> + }
>> + }
>> +
>> bdrv_refresh_limits(bs, NULL);
>>
>> bdrv_set_perm(reopen_state->bs, reopen_state->perm,
>
> Hm... Does bdrv_set_perm() work correctly when between bdrv_check_perm()
> and here the graph was changed?
>
> Kevin