From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Alberto Garcia <berto@igalia.com>, qemu-devel@nongnu.org
Cc: qemu-block@nongnu.org, Kevin Wolf <kwolf@redhat.com>,
Max Reitz <mreitz@redhat.com>
Subject: Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen
Date: Wed, 24 Mar 2021 18:08:38 +0300 [thread overview]
Message-ID: <0d11e778-e23a-b809-77b8-31ac0c638201@virtuozzo.com> (raw)
In-Reply-To: <w515z1gviul.fsf@maestria.local.igalia.com>
24.03.2021 15:25, Alberto Garcia wrote:
> On Thu 18 Mar 2021 03:25:07 PM CET, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>> static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
>>> BlockReopenQueue *queue,
>>> - Transaction *set_backings_tran, Error **errp);
>>> + Transaction *tran, Error **errp);
>>
>> I'd not call it just "tran" to not interfere with transaction
>> actions. Of course, reopen should be finally refactored to work
>> cleanly on Transaction API, but that is not done yet. And here we pass
>> a transaction pointer only to keep children modification.. So, let's
>> make it change_child_tran, or something like this.
>
> The name change looks good to me.
>
>>> + } else if (bdrv_recurse_has_child(new_child_bs, bs)) {
>>> + error_setg(errp, "Making '%s' a %s of '%s' would create a cycle",
>>> + str, parse_file ? "file" : "backing file",
>>
>> maybe s/"file"/"file child"/
>
> Ok.
>
>>> default:
>>> - /* 'backing' does not allow any other data type */
>>> + /* The options QDict has been flattened, so 'backing' and 'file'
>>> + * do not allow any other data type here. */
>>
>> checkpatch should complain that you didn't fix style of the comment...
>
> I actually don't like to use the proposed style for 2-line comments in
> many cases. I think it makes sense for big comment blocks but adds noise
> for shorter comments.
>
>>> + } else {
>>> + /*
>>> + * Ensure that @bs can really handle backing files, because we are
>>> + * about to give it one (or swap the existing one)
>>> + */
>>> + if (bs->drv->is_filter) {
>>> + /* Filters always have a file or a backing child */
>>
>> Probably we can assert bs->backing, as otherwise backing option should
>> be unsupported [preexisting, not about this patch]
>
> Yes, I see that this was added in commit 1d42f48c3a, maybe Max has good
> reasons to keep it this way?
>
>>> if (bdrv_is_backing_chain_frozen(overlay_bs,
>>> - child_bs(overlay_bs->backing), errp))
>>> + bdrv_filter_or_cow_bs(overlay_bs),
>>> + errp))
>>> {
>>> return -EPERM;
>>> }
>
> I just realized that this part is probably not ok if you want to change
> bs->file on a node that is not a filter, because this would check
> bs->backing->frozen and not bs->file->frozen.
>
>>> + 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);
>>
>> I think that actually, we need also to update inherits_from and do
>> refresh_limits like in bdrv_set_backing_noperm().
>
> Yes, I think you're right.
>
>> Probably, bdrv_replace_child should do it. Probably not (there are
>> still a lot of things to refactor in block.c :)..
>>
>> Hm. Also, using blockdev-reopen probably means that we are in a
>> blockdev word, so we should not care about inherits_from here.
>
> But with blockdev-reopen we do update inherits_from for backing files,
> don't we?
We do.. But as I understand resent Kevin's explanation on my "[PATCH RFC 0/3] block: drop inherits_from", inherits_from exists to support pre-blockdev era..
Anyway, better to support it and don't care, and drop all inherits_from logic at some bright future point.
>
>> Also, you don't create reopen_state->replace_file_bs, like for
>> backing.. On bdrv_reopen_comnmit replace_backing_bs is used to remove
>> corresponding options.. Shouldn't we do the same with file options?
>
> I think you're right.
>
>>> - self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 'file'")
>>> - self.reopen(opts, {'file': ''}, "Cannot change the option 'file'")
>>> + self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'")
>>
>> Interesting that error-message say about device='', not 'not-found'...
>
> That's because 'file' refers to a node name.
>
> Thanks for reviewing,
>
> Berto
>
--
Best regards,
Vladimir
next prev parent reply other threads:[~2021-03-24 15:09 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 [this message]
2021-05-05 13:58 ` Kevin Wolf
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=0d11e778-e23a-b809-77b8-31ac0c638201@virtuozzo.com \
--to=vsementsov@virtuozzo.com \
--cc=berto@igalia.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/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).