[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 39/47] blockdev: Fix active commit choice
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v7 39/47] blockdev: Fix active commit choice |
Date: |
Mon, 24 Aug 2020 17:06:14 +0200 |
Am 24.08.2020 um 16:41 hat Max Reitz geschrieben:
> On 24.08.20 16:07, Kevin Wolf wrote:
> > Am 24.08.2020 um 15:18 hat Max Reitz geschrieben:
> >> On 21.08.20 17:50, Kevin Wolf wrote:
> >>> Am 25.06.2020 um 17:22 hat Max Reitz geschrieben:
> >>>> We have to perform an active commit whenever the top node has a parent
> >>>> that has taken the WRITE permission on it.
> >>>>
> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>>> ---
> >>>> blockdev.c | 24 +++++++++++++++++++++---
> >>>> 1 file changed, 21 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/blockdev.c b/blockdev.c
> >>>> index 402f1d1df1..237fffbe53 100644
> >>>> --- a/blockdev.c
> >>>> +++ b/blockdev.c
> >>>> @@ -2589,6 +2589,7 @@ void qmp_block_commit(bool has_job_id, const char
> >>>> *job_id, const char *device,
> >>>> AioContext *aio_context;
> >>>> Error *local_err = NULL;
> >>>> int job_flags = JOB_DEFAULT;
> >>>> + uint64_t top_perm, top_shared;
> >>>>
> >>>> if (!has_speed) {
> >>>> speed = 0;
> >>>> @@ -2704,14 +2705,31 @@ void qmp_block_commit(bool has_job_id, const
> >>>> char *job_id, const char *device,
> >>>> goto out;
> >>>> }
> >>>>
> >>>> - if (top_bs == bs) {
> >>>> + /*
> >>>> + * Active commit is required if and only if someone has taken a
> >>>> + * WRITE permission on the top node.
> >>>
> >>> ...or if someone wants to take a WRITE permission while the job is
> >>> running.
> >>>
> >>> Future intentions of the user is something that we can't know, so maybe
> >>> this should become an option in the future (not in this series, of
> >>> course).
> >>>
> >>>> Historically, we have always
> >>>> + * used active commit for top nodes, so continue that practice.
> >>>> + * (Active commit is never really wrong.)
> >>>> + */
> >>>
> >>> Changing the practice would break compatibility with clients that start
> >>> an active commit job and then attach it to a read-write device, so we
> >>> must continue the practice. I think the comment should be clearer about
> >>> this, it sounds more like "no reason, but why not".
> >>
> >> I think that’s what I meant by “historically”. Is “legacily” a word?
> >>
> >> But sure, I can make it more explicit.
> >>
> >>> This is even more problematic because the commit job doesn't unshare
> >>> BLK_PERM_WRITE yet, so it would lead to silent corruption rather than an
> >>> error.
> >>>
> >>>> + bdrv_get_cumulative_perm(top_bs, &top_perm, &top_shared);
> >>>> + if (top_perm & BLK_PERM_WRITE ||
> >>>> + bdrv_skip_filters(top_bs) == bdrv_skip_filters(bs))
> >>>> + {
> >>>> if (has_backing_file) {
> >>>> error_setg(errp, "'backing-file' specified,"
> >>>> " but 'top' is the active layer");
> >>>
> >>> Hm, this error message isn't accurate any more.
> >>>
> >>> In fact, the implementation isn't consistent with the QAPI documentation
> >>> any more, because backing-file is only an error for the top level.
> >>
> >> Hm. I wanted to agree, and then I wanted to come up with a QAPI
> >> documentation that fits the new behavior (because I think it makes more
> >> sense to change the QAPI documentation along with the behavior change,
> >> rather than to force us to allow backing-file for anything that isn’t on
> >> the top layer).
> >>
> >> But in the process of coming up with a better description, I noticed
> >> that this doesn’t say “is a root node”, it says “is the active layer”.
> >> I would say a node in the active layer is a node that has some parent
> >> that has taken a WRITE permission on it. So actually I think that the
> >> documentation is right, and this code only now fits.
> >
> > Then you may have not only "the" active layer, but multiple active
> > layers. I find this a bit counterintuitive.
>
> Depends on what you count as a layer. I don’t think that’s a clearly
> defined term, is it? I only know of “active layer”, “format layer”,
> “protocol layer”, and you can at least have multiple format layers above
> each other. So I don’t find it counterintuitive.
>
> But perhaps it’d be best to just get away from the term “active layer”,
> as you propose below.
Hm, if I needed to describe what a layer is for me intuitively, I guess
it would be something like each non-filter node on a node chain with all
of the filters directly on top of it?
Depending on which link you follow, you get different sets of layers:
For bs->file, you get the format/protocol layer distinction. For
bs->backing, you get essentially what bdrv_backing_chain_next()
iterates.
In this context (which is talking about COW overlays), I expected the
bs->backing link to apply.
The active layer is then the COW layer that is directly referenced by a
guest device, block job or block export.
> > There is a simple reason why backing-file is an error for a root node:
> > It doesn't have overlays, so a value to write to the header of overlay
> > images just doesn't make sense.
>
> Ah, yeah...
>
> > The same reasoning doesn't apply for writable images that do have
> > overlays. Forbidding backing-file is a more arbitrary restriction there.
> > I'm not saying that we can't make arbitrary restrictions where allowing
> > an option is not worth the effort, but I feel they should be spelt out
> > more explicitly instead of twisting words like "active layer" until they
> > fit the code.
>
> I’m all for spelling it out more explicitly. I just noticed that I
> couldn’t clearly distinguish “active layer” from “other” cases of nodes
> with writers on them, which is why I noted that “active” to me means the
> post-patch behavior already.
>
> You’re right that there is no semantic reason for making it an error.
> So I just want it to be an error to be lazy. I hope you let me do that.
> (I don’t think there’s much of a problem with it, considering that
> commits on nodes that have the WRITE permission taken are basically just
> completely broken right now.)
That I'm happy to allow you to be lazy in this case is what I wanted to
express with "I'm not saying that we can't make arbitrary restrictions".
:-)
> >> Though I do think this wants for some clarification. Perhaps “If 'top'
> >> is the active layer (i.e., is a node that may be written to), specifying
> >> a backing [...]”?
> >
> > "If 'top' doesn't have an overlay image or is in use by a writer..."?
>
> I.e., avoiding the term “active layer” altogether? Sounds good. Only,
> I don’t know about “writer”... But it’s already used in
> BlockdevOptionsFile.dynamic-auto-read-only’s description, so I suppose
> we can use it here, too. (I just don’t know if as a
> non-block-layer-developer I’d know what it means.)
I was thinking of something like "is used read-write" at first, but then
realised that write-only is possible, too, so it wouldn't be entirely
accurate...
> (Also, yes, you’re right, the current behavior of giving all root nodes
> an active commit of course remains, even when there are no writers.)
>
> >> There’s more wrong with the specification, namely the whole part under
> >> @backing-file past the “(Since 2.1)”, starting with “If top == base”. I
> >> think all of that should go to the top level. (And “If top == active”
> >> should be changed to “If top is active (i.e., may be written to)”.)
> >
> > At least the latter only becomes wrong with this patch, so I think it
> > needs to be changed by this patch.
>
> Sure. So I understand you agree with moving the whole chunk, right?
I don't mind either way.
Kevin
signature.asc
Description: PGP signature