From: Kevin Wolf <kwolf@redhat.com>
To: Alberto Garcia <berto@igalia.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
qemu-devel@nongnu.org, qemu-block@nongnu.org,
Max Reitz <mreitz@redhat.com>
Subject: Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen
Date: Wed, 5 May 2021 15:58:57 +0200 [thread overview]
Message-ID: <YJKkoQzEo5OdTn4i@merkur.fritz.box> (raw)
In-Reply-To: <31ccb1061199ee11bf9879f6c60608a19b83263d.1616000692.git.berto@igalia.com>
Am 17.03.2021 um 18:15 hat Alberto Garcia geschrieben:
> When the x-blockdev-reopen was added it allowed reconfiguring the
> graph by replacing backing files, but changing the 'file' option was
> forbidden. Because of this restriction some operations are not
> possible, notably inserting and removing block filters.
>
> This patch adds support for replacing the 'file' option. This is
> similar to replacing the backing file and the user is likewise
> responsible for the correctness of the resulting graph, otherwise this
> can lead to data corruption.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> @@ -4238,13 +4254,13 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
> }
>
> /* If we want to replace the backing file we need some extra checks */
> - if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) {
> + if (new_child_bs != bdrv_filter_or_cow_bs(overlay_bs)) {
I may be missing something, but I don't see how this whole block makes
sense for changing 'file'.
overlay_bs was found by going down the backing chain, so of course it
will be different from new_child_bs (unless you use the same node as
'backing' and as 'file'). So we run all this code that seems to be
concerned only with backing files.
Probably overlay_bs should be found by starting from child and using
bdrv_filter_or_cow_bs() only for the following loop iterations.
> int ret;
>
> /* Check for implicit nodes between bs and its backing file */
> if (bs != overlay_bs) {
> - error_setg(errp, "Cannot change backing link if '%s' has "
> - "an implicit backing file", bs->node_name);
> + error_setg(errp, "Cannot change %s link if '%s' has an implicit "
> + "child", parse_file ? "file" : "backing", bs->node_name);
> return -EPERM;
> }
With fixed overlay_bs, this check makes sense, though the comment needs
an update.
> /*
> @@ -4256,16 +4272,24 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
> * with bs->drv->supports_backing == true.
> */
> if (bdrv_is_backing_chain_frozen(overlay_bs,
> - child_bs(overlay_bs->backing), errp))
> + bdrv_filter_or_cow_bs(overlay_bs),
> + errp))
> {
> return -EPERM;
> }
This checks if bs->backing is frozen (overlay_bs == bs because of the
check above). What we really want to check is if child is frozen (i.e.
bs->backing for updating 'backing', bs->file for updating 'file). So
maybe this should be just written as that:
if (child && child->frozen) {
error_setg(errp, ...);
return -EPERM;
}
Or factor this part out from bdrv_is_backing_chain_frozen() into a
bdrv_is_child_frozen() or something like that.
Either way, this might make the whole (outdated) comment above
unnecessary because things would become a lot clearer.
> - reopen_state->replace_backing_bs = true;
> - reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL;
> - ret = bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran,
> - errp);
> - if (ret < 0) {
> - return ret;
> + if (parse_file) {
> + /* Store the old file bs, we'll need to refresh its permissions */
> + reopen_state->old_file_bs = bs->file->bs;
> +
> + /* And finally replace the child */
> + bdrv_replace_child(bs->file, new_child_bs, tran);
> + } else {
> + reopen_state->replace_backing_bs = true;
> + reopen_state->old_backing_bs = child_bs(bs->backing);
> + ret = bdrv_set_backing_noperm(bs, new_child_bs, tran, errp);
> + if (ret < 0) {
> + return ret;
> + }
> }
> }
Kevin
next prev parent reply other threads:[~2021-05-05 14:06 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-17 17:15 [PATCH v4 0/6] Allow changing bs->file on reopen Alberto Garcia
2021-03-17 17:15 ` [PATCH v4 1/6] block: Add bdrv_reopen_queue_free() Alberto Garcia
2021-03-18 13:32 ` Vladimir Sementsov-Ogievskiy
2021-03-17 17:15 ` [PATCH v4 2/6] block: Allow changing bs->file on reopen Alberto Garcia
2021-03-18 14:25 ` Vladimir Sementsov-Ogievskiy
2021-03-24 12:25 ` Alberto Garcia
2021-03-24 15:08 ` Vladimir Sementsov-Ogievskiy
2021-05-05 13:58 ` Kevin Wolf [this message]
2021-05-07 7:11 ` Vladimir Sementsov-Ogievskiy
2021-05-07 14:09 ` Kevin Wolf
2021-05-10 9:26 ` Vladimir Sementsov-Ogievskiy
2021-03-17 17:15 ` [PATCH v4 3/6] iotests: Test replacing files with x-blockdev-reopen Alberto Garcia
2021-05-05 15:57 ` Kevin Wolf
2021-03-17 17:15 ` [PATCH v4 4/6] block: Support multiple reopening " Alberto Garcia
2021-03-18 14:45 ` Vladimir Sementsov-Ogievskiy
2021-05-05 16:18 ` Kevin Wolf
2021-05-06 9:21 ` Vladimir Sementsov-Ogievskiy
2021-03-17 17:15 ` [PATCH v4 5/6] iotests: Test reopening multiple devices at the same time Alberto Garcia
2021-03-17 17:15 ` [PATCH v4 6/6] block: Make blockdev-reopen stable API Alberto Garcia
2021-05-14 15:53 ` [PATCH v4 0/6] Allow changing bs->file on reopen Vladimir Sementsov-Ogievskiy
2021-06-09 15:53 ` Kevin Wolf
2021-06-09 16:40 ` Vladimir Sementsov-Ogievskiy
2021-06-10 12:10 ` Vladimir Sementsov-Ogievskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YJKkoQzEo5OdTn4i@merkur.fritz.box \
--to=kwolf@redhat.com \
--cc=berto@igalia.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).