Re: [PATCH v7 14/47] stream: Deal with filters
On 20.08.20 12:49, Vladimir Sementsov-Ogievskiy wrote:
> 20.08.2020 12:22, Max Reitz wrote:
>> On 20.08.20 10:31, Max Reitz wrote:
>>
>> [...]
>>
>>> So all in all, I believe the biggest surprise about what’s written into
>>> the top layer isn’t that it may be a json:{} filename, but the filename
>>> of a node that maybe doesn’t even exist anymore? (Oh, no, please don’t
>>> tell me you can delete it and get an invalid pointer read...)
>>
>> (I tried triggering that, but, oh, it’s strdup’ed() in stream_start().
>> I’m a bit daft.)
>>
>
>
> If it's broken anyway, probably we can just revert c624b015bf and start
> to freeze base again?
Well, it’s only broken if you care about the backing filename string
that’s written to @top. So it isn’t broken altogether.
Though, well. If we all agree to just revert it and maybe add a @bottom
parameter instead, then I suppose we could do it.
(Maybe in a follow-up, though.)
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v7 14/47] stream: Deal with filters
20.08.2020 12:22, Max Reitz wrote:
On 20.08.20 10:31, Max Reitz wrote:
[...]
So all in all, I believe the biggest surprise about what’s written into
the top layer isn’t that it may be a json:{} filename, but the filename
of a node that maybe doesn’t even exist anymore? (Oh, no, please don’t
tell me you can delete it and get an invalid pointer read...)
(I tried triggering that, but, oh, it’s strdup’ed() in stream_start().
I’m a bit daft.)
If it's broken anyway, probably we can just revert c624b015bf and start to
freeze base again?
--
Best regards,
Vladimir
Re: [PATCH v7 14/47] stream: Deal with filters
On 20.08.20 10:31, Max Reitz wrote:
[...]
> So all in all, I believe the biggest surprise about what’s written into
> the top layer isn’t that it may be a json:{} filename, but the filename
> of a node that maybe doesn’t even exist anymore? (Oh, no, please don’t
> tell me you can delete it and get an invalid pointer read...)
(I tried triggering that, but, oh, it’s strdup’ed() in stream_start().
I’m a bit daft.)
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v7 14/47] stream: Deal with filters
On 19.08.20 17:16, Kevin Wolf wrote:
> Am 19.08.2020 um 16:47 hat Max Reitz geschrieben:
>> On 18.08.20 16:28, Kevin Wolf wrote:
>>> Am 25.06.2020 um 17:21 hat Max Reitz geschrieben:
Because of the (not so recent anymore) changes that make the stream job
independent of the base node and instead track the node above it, we
have to split that "bottom" node into two cases: The bottom COW node,
and the node directly above the base node (which may be an R/W filter
or the bottom COW node).
Signed-off-by: Max Reitz
---
qapi/block-core.json | 4 +++
block/stream.c | 63
blockdev.c | 4 ++-
3 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b20332e592..df87855429 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2486,6 +2486,10 @@
# On successful completion the image file is updated to drop the backing
file
# and the BLOCK_JOB_COMPLETED event is emitted.
#
+# In case @device is a filter node, block-stream modifies the first
non-filter
+# overlay node below it to point to base's backing node (or NULL if @base
was
+# not specified) instead of modifying @device itself.
>>>
>>> Not to @base's backing node, but to @base itself (or actually, to
>>> above_base's backing node, which is initially @base, but may have
>>> changed when the job is completed).
>>
>> Oh, yes.
>>
>> (I thought I had noticed that already at some point and fixed it
>> locally... But apparently not.)
>>
>>> Should we also document what using a filter node for @base means?
>>
>> Hm. What does it mean? I think the more interesting case is what it
>> means if above_base is a filter, right?
>>
>> Maybe we can put in somewhere in the “If a base file is specified then
>> sectors are not copied from that base file and its backing chain.” But
>> the more I think about it, the less I know what we could add to it.
>> What happens if there are filters above @base is that their data isn’t
>> copied, because that’s exactly the data in @base.
>
> The interesting part is probably the graph reconfiguration at the end of
> the job. Which is actually already documented:
>
> # When streaming completes the image file will have the base
> # file as its backing file.
>
> Of course, this is not entirely correct any more (because the base may
> have changed).
>
> If @base is a filter, what backing file path do we write into the top
> layer? A json: filename including the filter?
Yes.
Or, actually. Now that I read the code... It takes @base’s filename
before the stream job and then uses that. So if @base has changed
during the job, then it still uses the old filename.
But that’s not really due to this series.
> Is this worth mentioning
> or do you consider it obvious?
Hm. I consider it obvious, yes. @base becomes @top’s backing file (at
least without any graph changes while the job is running), so naturally
what’s written into the image header is @base’s filename – which is a
json:{} filename.
On second thought, @backing-file’s description mysteriously states that
“QEMU will automatically determine the backing file string to use”.
Which makes sense because it would clearly not make sense to describe
what’s actually happening, which is to use @base’s filename at job start
regardless of whether it’s still there at the end of the job.
So I suppose I have the choice of either documenting exactly what’s
happening, even though it doesn’t make much sense, or just not, keeping
it mysterious.
So all in all, I believe the biggest surprise about what’s written into
the top layer isn’t that it may be a json:{} filename, but the filename
of a node that maybe doesn’t even exist anymore? (Oh, no, please don’t
tell me you can delete it and get an invalid pointer read...)
The more I think about it, the more I think there are problems beyond
the scope of this series here. :/
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v7 14/47] stream: Deal with filters
Am 19.08.2020 um 16:47 hat Max Reitz geschrieben: > On 18.08.20 16:28, Kevin Wolf wrote: > > Am 25.06.2020 um 17:21 hat Max Reitz geschrieben: > >> Because of the (not so recent anymore) changes that make the stream job > >> independent of the base node and instead track the node above it, we > >> have to split that "bottom" node into two cases: The bottom COW node, > >> and the node directly above the base node (which may be an R/W filter > >> or the bottom COW node). > >> > >> Signed-off-by: Max Reitz > >> --- > >> qapi/block-core.json | 4 +++ > >> block/stream.c | 63 > >> blockdev.c | 4 ++- > >> 3 files changed, 53 insertions(+), 18 deletions(-) > >> > >> diff --git a/qapi/block-core.json b/qapi/block-core.json > >> index b20332e592..df87855429 100644 > >> --- a/qapi/block-core.json > >> +++ b/qapi/block-core.json > >> @@ -2486,6 +2486,10 @@ > >> # On successful completion the image file is updated to drop the backing > >> file > >> # and the BLOCK_JOB_COMPLETED event is emitted. > >> # > >> +# In case @device is a filter node, block-stream modifies the first > >> non-filter > >> +# overlay node below it to point to base's backing node (or NULL if @base > >> was > >> +# not specified) instead of modifying @device itself. > > > > Not to @base's backing node, but to @base itself (or actually, to > > above_base's backing node, which is initially @base, but may have > > changed when the job is completed). > > Oh, yes. > > (I thought I had noticed that already at some point and fixed it > locally... But apparently not.) > > > Should we also document what using a filter node for @base means? > > Hm. What does it mean? I think the more interesting case is what it > means if above_base is a filter, right? > > Maybe we can put in somewhere in the “If a base file is specified then > sectors are not copied from that base file and its backing chain.” But > the more I think about it, the less I know what we could add to it. > What happens if there are filters above @base is that their data isn’t > copied, because that’s exactly the data in @base. The interesting part is probably the graph reconfiguration at the end of the job. Which is actually already documented: # When streaming completes the image file will have the base # file as its backing file. Of course, this is not entirely correct any more (because the base may have changed). If @base is a filter, what backing file path do we write into the top layer? A json: filename including the filter? Is this worth mentioning or do you consider it obvious? Kevin signature.asc Description: PGP signature
Re: [PATCH v7 14/47] stream: Deal with filters
On 18.08.20 16:28, Kevin Wolf wrote: > Am 25.06.2020 um 17:21 hat Max Reitz geschrieben: >> Because of the (not so recent anymore) changes that make the stream job >> independent of the base node and instead track the node above it, we >> have to split that "bottom" node into two cases: The bottom COW node, >> and the node directly above the base node (which may be an R/W filter >> or the bottom COW node). >> >> Signed-off-by: Max Reitz >> --- >> qapi/block-core.json | 4 +++ >> block/stream.c | 63 >> blockdev.c | 4 ++- >> 3 files changed, 53 insertions(+), 18 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index b20332e592..df87855429 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -2486,6 +2486,10 @@ >> # On successful completion the image file is updated to drop the backing >> file >> # and the BLOCK_JOB_COMPLETED event is emitted. >> # >> +# In case @device is a filter node, block-stream modifies the first >> non-filter >> +# overlay node below it to point to base's backing node (or NULL if @base >> was >> +# not specified) instead of modifying @device itself. > > Not to @base's backing node, but to @base itself (or actually, to > above_base's backing node, which is initially @base, but may have > changed when the job is completed). Oh, yes. (I thought I had noticed that already at some point and fixed it locally... But apparently not.) > Should we also document what using a filter node for @base means? Hm. What does it mean? I think the more interesting case is what it means if above_base is a filter, right? Maybe we can put in somewhere in the “If a base file is specified then sectors are not copied from that base file and its backing chain.” But the more I think about it, the less I know what we could add to it. What happens if there are filters above @base is that their data isn’t copied, because that’s exactly the data in @base. Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v7 14/47] stream: Deal with filters
19.08.2020 15:39, Max Reitz wrote: On 10.08.20 13:04, Vladimir Sementsov-Ogievskiy wrote: 10.08.2020 11:12, Max Reitz wrote: On 07.08.20 12:29, Vladimir Sementsov-Ogievskiy wrote: [...] But, with our proposed way (freeze only chain up to base_overlay inclusively, and use backing(base_overlay) as final backing), all will work as expected, and two parallel jobs will work.. I don’t think it will work as expected because users can no longer specify which node should be the base node after streaming. And the QAPI schema says that base-node is to become the backing file of the top node after streaming. But this will never work with either way: base node may disappear during stream. Even with you way, they only stable thing is "above-base", which backing child may be completely another node at stream finish. Yeah, but after c624b015bf, that’s just how it is. I thought the best would be an approach where if there are no graph changes during the job, you would indeed end up with @base as the backing file afterwards. [...] Well, that still leaves the problem that users should be able to specify which node is to become the base after streaming, and that that node maybe shouldn’t be restricted to immediate children of COW images. And again, this is impossible even with your way. I have an idea: What about making the whole thing explicit? We add an optional parameter to stream-job: bottom-node, which is mutally exclusive with specifying base. Then, if user specified base node, we freeze base as well, so it can't disappear. User will not be able to start parallel stream with this base node as top (because new stream can not insert a filter into frozen chain), but for sure it's rare case, used only in iotest 30 :)). Benefit: user have guarantee of what would be final backing node. Sounds very nice to me, but... Otherwise, if user specified bottom-node, we use the way of this patch. So user can run parallel streams (iotest 30 will have to use bottom-node argument). No guarantee of final base-node, it would be backing of bottom-node at job finish. But, this is incompatible change, and we probably should wait for 3 releases for deprecation of old behavior.. ...yeah. Hm. What a pain, but right, we can just deprecate it. Unfortunately, I don’t think there’s any way we could issue a warning (we’d want to deprecate using the @base node in something outside of the stream job, and we can’t really detect this case to issue a warning). So it would be a deprecation found only in the deprecation notes and the QAPI spec... Hmm.. add "bool frozen_only_warn" field to BdrvChild and print warning instead of fail where c->frozen cause failure? I can make a patch, if you don't think that its too much. Anyway, I feel now, that you convinced me. I'm not sure that we will not have to change it make filter work, but not reason to change something now. Andrey, could you try to rebase your series on top of this and fix iotest 30 by just specifying exact node-names in it?.. Hmmm. My thought goes further. Seems, that in this way, introducing explicit filter would be incompatible change anyway: it will break scenario with parallel stream jobs, when user specifies filenames, not node names (user will have to specify filter-node name as base for another stream job, as you said). So, it's incompatible anyway. What do you think of it? Could we break this scenario in one release without deprecation and don't care? I don’t know, but I’m afraid I don’t think we can. Actually, we already done so once: Freezing of base was introduced in 4.0, when c624b015bf comes in 4.1 (there were no real user bugs or feature requests, it was just my [bad] idea, related to the problem around our stream-filter and parallel jobs in iotest 30). So, at least for 4.0 we had frozen base.. Were there any bugs? But now we live with not-frozen base for several releases.. Than I think my idea about base vs bottom-node arguments for stream job may be applied. Or what to do? If we can't break this scenario without a deprecation, we'll have to implement "implicit" filter, like for mirror, when filter-node-name is not specified. And for this implicit filter we'll need additional logic (closer to what I've proposed in a previous mail). Or, try to keep stream without a filter (not insert it at all and behave the old way), when filter-node-name is not specified. Than new features based on filter will be available only when filter-node-name is specified, but this is OK. The latter seems better for me. If that works... OK. So what I think we can do is first just take this patch as part of this series. Yes, let's start with it. Then, we add @bottom-node separately and deprecate @base not being frozen. I think we can just deprecate, and not add the bottom-node. If someone will come during deprecation period and say that he have such use-case, we'll add @bottom-node. Otherwise we'll just start to freeze base node again. If
Re: [PATCH v7 14/47] stream: Deal with filters
On 10.08.20 13:04, Vladimir Sementsov-Ogievskiy wrote: > 10.08.2020 11:12, Max Reitz wrote: >> On 07.08.20 12:29, Vladimir Sementsov-Ogievskiy wrote: [...] >>> But, with our proposed way (freeze only chain up to base_overlay >>> inclusively, and use backing(base_overlay) as final backing), all will >>> work as expected, and two parallel jobs will work.. >> >> I don’t think it will work as expected because users can no longer >> specify which node should be the base node after streaming. And the >> QAPI schema says that base-node is to become the backing file of the top >> node after streaming. > > But this will never work with either way: base node may disappear during > stream. Even with you way, they only stable thing is "above-base", which > backing child may be completely another node at stream finish. Yeah, but after c624b015bf, that’s just how it is. I thought the best would be an approach where if there are no graph changes during the job, you would indeed end up with @base as the backing file afterwards. [...] >> Well, that still leaves the problem that users should be able to specify >> which node is to become the base after streaming, and that that node >> maybe shouldn’t be restricted to immediate children of COW images. > > And again, this is impossible even with your way. I have an idea: > > What about making the whole thing explicit? > > We add an optional parameter to stream-job: bottom-node, which is > mutally exclusive with specifying base. > > Then, if user specified base node, we freeze base as well, so it can't > disappear. User will not be able to start parallel stream with this base > node as top (because new stream can not insert a filter into frozen > chain), but for sure it's rare case, used only in iotest 30 :)). > Benefit: user have guarantee of what would be final backing node. Sounds very nice to me, but... > Otherwise, if user specified bottom-node, we use the way of this patch. > So user can run parallel streams (iotest 30 will have to use bottom-node > argument). No guarantee of final base-node, it would be backing of > bottom-node at job finish. > > But, this is incompatible change, and we probably should wait for 3 > releases for deprecation of old behavior.. ...yeah. Hm. What a pain, but right, we can just deprecate it. Unfortunately, I don’t think there’s any way we could issue a warning (we’d want to deprecate using the @base node in something outside of the stream job, and we can’t really detect this case to issue a warning). So it would be a deprecation found only in the deprecation notes and the QAPI spec... > Anyway, I feel now, that you convinced me. I'm not sure that we will not > have to change it make filter work, but not reason to change something > now. Andrey, could you try to rebase your series on top of this and fix > iotest 30 by just specifying exact node-names in it?.. > > > Hmmm. My thought goes further. Seems, that in this way, introducing > explicit filter would be incompatible change anyway: it will break > scenario with parallel stream jobs, when user specifies filenames, not > node names (user will have to specify filter-node name as base for > another stream job, as you said). So, it's incompatible anyway. > > What do you think of it? Could we break this scenario in one release > without deprecation and don't care? I don’t know, but I’m afraid I don’t think we can. > Than I think my idea about base vs > bottom-node arguments for stream job may be applied. Or what to do? > > If we can't break this scenario without a deprecation, we'll have to > implement "implicit" filter, like for mirror, when filter-node-name is > not specified. And for this implicit filter we'll need additional logic > (closer to what I've proposed in a previous mail). Or, try to keep > stream without a filter (not insert it at all and behave the old way), > when filter-node-name is not specified. Than new features based on > filter will be available only when filter-node-name is specified, but > this is OK. The latter seems better for me. If that works... OK. So what I think we can do is first just take this patch as part of this series. Then, we add @bottom-node separately and deprecate @base not being frozen. If it’s feasible to not add a stream filter node until the deprecation period is over, then that should work. Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v7 14/47] stream: Deal with filters
Reviewed-by: Andrey Shinkevich
On 10.08.2020 14:04, Vladimir Sementsov-Ogievskiy wrote:
10.08.2020 11:12, Max Reitz wrote:
On 07.08.20 12:29, Vladimir Sementsov-Ogievskiy wrote:
16.07.2020 17:59, Max Reitz wrote:
On 10.07.20 19:41, Andrey Shinkevich wrote:
On 10.07.2020 18:24, Max Reitz wrote:
On 09.07.20 16:52, Andrey Shinkevich wrote:
On 25.06.2020 18:21, Max Reitz wrote:
Because of the (not so recent anymore) changes that make the
stream job
independent of the base node and instead track the node above
it, we
have to split that "bottom" node into two cases: The bottom COW
node,
and the node directly above the base node (which may be an R/W
filter
or the bottom COW node).
Signed-off-by: Max Reitz
---
qapi/block-core.json | 4 +++
block/stream.c | 63
blockdev.c | 4 ++-
3 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b20332e592..df87855429 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2486,6 +2486,10 @@
# On successful completion the image file is updated to
drop the
backing file
# and the BLOCK_JOB_COMPLETED event is emitted.
#
+# In case @device is a filter node, block-stream modifies the
first
non-filter
+# overlay node below it to point to base's backing node (or
NULL if
@base was
+# not specified) instead of modifying @device itself.
+#
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
diff --git a/block/stream.c b/block/stream.c
index aa2e7af98e..b9c1141656 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,7 +31,8 @@ enum {
typedef struct StreamBlockJob {
BlockJob common;
- BlockDriverState *bottom;
+ BlockDriverState *base_overlay; /* COW overlay (stream from
this) */
+ BlockDriverState *above_base; /* Node directly above the
base */
Keeping the base_overlay is enough to complete the stream job.
Depends on the definition. If we decide it isn’t enough, then it
isn’t
enough.
The above_base may disappear during the job and we can't rely on
it.
In this version of this series, it may not, because the chain is
frozen.
So the above_base cannot disappear.
Once we insert a filter above the top bs of the stream job, the
parallel
jobs in
the iotests #030 will fail with 'frozen link error'. It is because of
the
independent parallel stream or commit jobs that insert/remove their
filters
asynchroniously.
I’m not sure whether that’s a problem with this series specifically.
We can discuss whether we should allow it to disappear, but I think
not.
The problem is, we need something to set as the backing file after
streaming. How do we figure out what that should be? My proposal
is we
keep above_base and use its immediate child.
We can do the same with the base_overlay.
If the backing node turns out to be a filter, the proper backing
child will
be set after the filter is removed. So, we shouldn't care.
And what if the user manually added some filter above the base (i.e.
below base_overlay) that they want to keep after the job?
It's automatically kept, if we use base_overlay->backing->bs as final
backing node.
You mean, that they want it to be dropped?
Er, yes. Point is, the graph structure below with @base at the root may
be different than the one right below @base_overlay.
so, assuming the following:
top -(backing)-> manually-inserted-filter -(file)-> base
and user do stream with base=base, and expects filter to be removed by
stream job?
Hmm, yes, such use-case is broken with our proposed way...
Let me now clarify the problem we'll have with your way.
When stream don't have any filter, we can easily imagine two parallel
stream jobs:
top -(backing)-> mid1 -(backing)-> mid2 -(backing)-> base
stream1: top=top, base=mid2
stream2: top=mid2, base=NULL
final picture is obvious:
top (merged with mid1) -(backing)-> mid2 (merged with base)
Yes, and I don’t think this currently working case is broken by this
series.
But we want stream job has own filter, like mirror.
Which it does not have yet, right? Which is why I was saying that I
don’t think this is a problem with this series. We could try to address
it later.
Or do you think we can’t address it later because right now all filter
cases are broken anyway so now would be the time to make a breaking
change (which the suggestion to not use @base as the final backing
node is)?
I think, we can address it later, but it would be good to fit into one
release cycle with these series, to not make incompatible behavior
changes later.
So the picture becomes more complex.
Assume stream2 starts first.
top -(backing)-> mid1 -(backing)-> stream2-filter -(backing)-> mid2
-(backing)-> base
stream2-filter would be on top of mid2, right?
Right. In my picture, "-(backing)->" means backing link.
Re: [PATCH v7 14/47] stream: Deal with filters
Am 25.06.2020 um 17:21 hat Max Reitz geschrieben: > Because of the (not so recent anymore) changes that make the stream job > independent of the base node and instead track the node above it, we > have to split that "bottom" node into two cases: The bottom COW node, > and the node directly above the base node (which may be an R/W filter > or the bottom COW node). > > Signed-off-by: Max Reitz > --- > qapi/block-core.json | 4 +++ > block/stream.c | 63 > blockdev.c | 4 ++- > 3 files changed, 53 insertions(+), 18 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index b20332e592..df87855429 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2486,6 +2486,10 @@ > # On successful completion the image file is updated to drop the backing file > # and the BLOCK_JOB_COMPLETED event is emitted. > # > +# In case @device is a filter node, block-stream modifies the first > non-filter > +# overlay node below it to point to base's backing node (or NULL if @base was > +# not specified) instead of modifying @device itself. Not to @base's backing node, but to @base itself (or actually, to above_base's backing node, which is initially @base, but may have changed when the job is completed). Should we also document what using a filter node for @base means? The code changes look good. Kevin
Re: [PATCH v7 14/47] stream: Deal with filters
On 10.08.2020 14:04, Vladimir Sementsov-Ogievskiy wrote:
10.08.2020 11:12, Max Reitz wrote:
On 07.08.20 12:29, Vladimir Sementsov-Ogievskiy wrote:
16.07.2020 17:59, Max Reitz wrote:
On 10.07.20 19:41, Andrey Shinkevich wrote:
On 10.07.2020 18:24, Max Reitz wrote:
On 09.07.20 16:52, Andrey Shinkevich wrote:
On 25.06.2020 18:21, Max Reitz wrote:
Because of the (not so recent anymore) changes that make the
stream job
independent of the base node and instead track the node above
it, we
have to split that "bottom" node into two cases: The bottom COW
node,
and the node directly above the base node (which may be an R/W
filter
or the bottom COW node).
Signed-off-by: Max Reitz
---
qapi/block-core.json | 4 +++
block/stream.c | 63
blockdev.c | 4 ++-
3 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b20332e592..df87855429 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2486,6 +2486,10 @@
# On successful completion the image file is updated to
drop the
backing file
# and the BLOCK_JOB_COMPLETED event is emitted.
#
+# In case @device is a filter node, block-stream modifies the
first
non-filter
+# overlay node below it to point to base's backing node (or
NULL if
@base was
+# not specified) instead of modifying @device itself.
+#
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
diff --git a/block/stream.c b/block/stream.c
index aa2e7af98e..b9c1141656 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,7 +31,8 @@ enum {
typedef struct StreamBlockJob {
BlockJob common;
- BlockDriverState *bottom;
+ BlockDriverState *base_overlay; /* COW overlay (stream from
this) */
+ BlockDriverState *above_base; /* Node directly above the
base */
Keeping the base_overlay is enough to complete the stream job.
Depends on the definition. If we decide it isn’t enough, then it
isn’t
enough.
The above_base may disappear during the job and we can't rely on
it.
In this version of this series, it may not, because the chain is
frozen.
So the above_base cannot disappear.
Once we insert a filter above the top bs of the stream job, the
parallel
jobs in
the iotests #030 will fail with 'frozen link error'. It is because of
the
independent parallel stream or commit jobs that insert/remove their
filters
asynchroniously.
I’m not sure whether that’s a problem with this series specifically.
We can discuss whether we should allow it to disappear, but I think
not.
The problem is, we need something to set as the backing file after
streaming. How do we figure out what that should be? My proposal
is we
keep above_base and use its immediate child.
We can do the same with the base_overlay.
If the backing node turns out to be a filter, the proper backing
child will
be set after the filter is removed. So, we shouldn't care.
And what if the user manually added some filter above the base (i.e.
below base_overlay) that they want to keep after the job?
It's automatically kept, if we use base_overlay->backing->bs as final
backing node.
You mean, that they want it to be dropped?
Er, yes. Point is, the graph structure below with @base at the root may
be different than the one right below @base_overlay.
so, assuming the following:
top -(backing)-> manually-inserted-filter -(file)-> base
and user do stream with base=base, and expects filter to be removed by
stream job?
Hmm, yes, such use-case is broken with our proposed way...
Let me now clarify the problem we'll have with your way.
When stream don't have any filter, we can easily imagine two parallel
stream jobs:
top -(backing)-> mid1 -(backing)-> mid2 -(backing)-> base
stream1: top=top, base=mid2
stream2: top=mid2, base=NULL
final picture is obvious:
top (merged with mid1) -(backing)-> mid2 (merged with base)
Yes, and I don’t think this currently working case is broken by this
series.
But we want stream job has own filter, like mirror.
Which it does not have yet, right? Which is why I was saying that I
don’t think this is a problem with this series. We could try to address
it later.
Or do you think we can’t address it later because right now all filter
cases are broken anyway so now would be the time to make a breaking
change (which the suggestion to not use @base as the final backing
node is)?
I think, we can address it later, but it would be good to fit into one
release cycle with these series, to not make incompatible behavior
changes later.
So the picture becomes more complex.
Assume stream2 starts first.
top -(backing)-> mid1 -(backing)-> stream2-filter -(backing)-> mid2
-(backing)-> base
stream2-filter would be on top of mid2, right?
Right. In my picture, "-(backing)->" means backing link. Hmm, most
probably stream-filter
Re: [PATCH v7 14/47] stream: Deal with filters
10.08.2020 11:12, Max Reitz wrote:
On 07.08.20 12:29, Vladimir Sementsov-Ogievskiy wrote:
16.07.2020 17:59, Max Reitz wrote:
On 10.07.20 19:41, Andrey Shinkevich wrote:
On 10.07.2020 18:24, Max Reitz wrote:
On 09.07.20 16:52, Andrey Shinkevich wrote:
On 25.06.2020 18:21, Max Reitz wrote:
Because of the (not so recent anymore) changes that make the
stream job
independent of the base node and instead track the node above it, we
have to split that "bottom" node into two cases: The bottom COW node,
and the node directly above the base node (which may be an R/W filter
or the bottom COW node).
Signed-off-by: Max Reitz
---
qapi/block-core.json | 4 +++
block/stream.c | 63
blockdev.c | 4 ++-
3 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b20332e592..df87855429 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2486,6 +2486,10 @@
# On successful completion the image file is updated to drop the
backing file
# and the BLOCK_JOB_COMPLETED event is emitted.
#
+# In case @device is a filter node, block-stream modifies the first
non-filter
+# overlay node below it to point to base's backing node (or NULL if
@base was
+# not specified) instead of modifying @device itself.
+#
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
diff --git a/block/stream.c b/block/stream.c
index aa2e7af98e..b9c1141656 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,7 +31,8 @@ enum {
typedef struct StreamBlockJob {
BlockJob common;
- BlockDriverState *bottom;
+ BlockDriverState *base_overlay; /* COW overlay (stream from
this) */
+ BlockDriverState *above_base; /* Node directly above the
base */
Keeping the base_overlay is enough to complete the stream job.
Depends on the definition. If we decide it isn’t enough, then it isn’t
enough.
The above_base may disappear during the job and we can't rely on it.
In this version of this series, it may not, because the chain is
frozen.
So the above_base cannot disappear.
Once we insert a filter above the top bs of the stream job, the parallel
jobs in
the iotests #030 will fail with 'frozen link error'. It is because of
the
independent parallel stream or commit jobs that insert/remove their
filters
asynchroniously.
I’m not sure whether that’s a problem with this series specifically.
We can discuss whether we should allow it to disappear, but I think
not.
The problem is, we need something to set as the backing file after
streaming. How do we figure out what that should be? My proposal
is we
keep above_base and use its immediate child.
We can do the same with the base_overlay.
If the backing node turns out to be a filter, the proper backing
child will
be set after the filter is removed. So, we shouldn't care.
And what if the user manually added some filter above the base (i.e.
below base_overlay) that they want to keep after the job?
It's automatically kept, if we use base_overlay->backing->bs as final
backing node.
You mean, that they want it to be dropped?
Er, yes. Point is, the graph structure below with @base at the root may
be different than the one right below @base_overlay.
so, assuming the following:
top -(backing)-> manually-inserted-filter -(file)-> base
and user do stream with base=base, and expects filter to be removed by
stream job?
Hmm, yes, such use-case is broken with our proposed way...
Let me now clarify the problem we'll have with your way.
When stream don't have any filter, we can easily imagine two parallel
stream jobs:
top -(backing)-> mid1 -(backing)-> mid2 -(backing)-> base
stream1: top=top, base=mid2
stream2: top=mid2, base=NULL
final picture is obvious:
top (merged with mid1) -(backing)-> mid2 (merged with base)
Yes, and I don’t think this currently working case is broken by this series.
But we want stream job has own filter, like mirror.
Which it does not have yet, right? Which is why I was saying that I
don’t think this is a problem with this series. We could try to address
it later.
Or do you think we can’t address it later because right now all filter
cases are broken anyway so now would be the time to make a breaking
change (which the suggestion to not use @base as the final backing node is)?
I think, we can address it later, but it would be good to fit into one release
cycle with these series, to not make incompatible behavior changes later.
So the picture becomes more complex.
Assume stream2 starts first.
top -(backing)-> mid1 -(backing)-> stream2-filter -(backing)-> mid2
-(backing)-> base
stream2-filter would be on top of mid2, right?
Right. In my picture, "-(backing)->" means backing link. Hmm, most probably
stream-filter is COR, which actually have file child. It doesn't matter here.
Now,
Re: [PATCH v7 14/47] stream: Deal with filters
On 07.08.20 12:29, Vladimir Sementsov-Ogievskiy wrote:
> 16.07.2020 17:59, Max Reitz wrote:
>> On 10.07.20 19:41, Andrey Shinkevich wrote:
>>> On 10.07.2020 18:24, Max Reitz wrote:
On 09.07.20 16:52, Andrey Shinkevich wrote:
> On 25.06.2020 18:21, Max Reitz wrote:
>> Because of the (not so recent anymore) changes that make the
>> stream job
>> independent of the base node and instead track the node above it, we
>> have to split that "bottom" node into two cases: The bottom COW node,
>> and the node directly above the base node (which may be an R/W filter
>> or the bottom COW node).
>>
>> Signed-off-by: Max Reitz
>> ---
>> qapi/block-core.json | 4 +++
>> block/stream.c | 63
>>
>> blockdev.c | 4 ++-
>> 3 files changed, 53 insertions(+), 18 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index b20332e592..df87855429 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2486,6 +2486,10 @@
>> # On successful completion the image file is updated to drop the
>> backing file
>> # and the BLOCK_JOB_COMPLETED event is emitted.
>> #
>> +# In case @device is a filter node, block-stream modifies the first
>> non-filter
>> +# overlay node below it to point to base's backing node (or NULL if
>> @base was
>> +# not specified) instead of modifying @device itself.
>> +#
>> # @job-id: identifier for the newly-created block job. If
>> # omitted, the device name will be used. (Since 2.7)
>> #
>> diff --git a/block/stream.c b/block/stream.c
>> index aa2e7af98e..b9c1141656 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -31,7 +31,8 @@ enum {
>> typedef struct StreamBlockJob {
>> BlockJob common;
>> - BlockDriverState *bottom;
>> + BlockDriverState *base_overlay; /* COW overlay (stream from
>> this) */
>> + BlockDriverState *above_base; /* Node directly above the
>> base */
> Keeping the base_overlay is enough to complete the stream job.
Depends on the definition. If we decide it isn’t enough, then it isn’t
enough.
> The above_base may disappear during the job and we can't rely on it.
In this version of this series, it may not, because the chain is
frozen.
So the above_base cannot disappear.
>>>
>>> Once we insert a filter above the top bs of the stream job, the parallel
>>> jobs in
>>>
>>> the iotests #030 will fail with 'frozen link error'. It is because of
>>> the
>>>
>>> independent parallel stream or commit jobs that insert/remove their
>>> filters
>>>
>>> asynchroniously.
>>
>> I’m not sure whether that’s a problem with this series specifically.
>>
We can discuss whether we should allow it to disappear, but I think
not.
The problem is, we need something to set as the backing file after
streaming. How do we figure out what that should be? My proposal
is we
keep above_base and use its immediate child.
>>>
>>> We can do the same with the base_overlay.
>>>
>>> If the backing node turns out to be a filter, the proper backing
>>> child will
>>>
>>> be set after the filter is removed. So, we shouldn't care.
>>
>> And what if the user manually added some filter above the base (i.e.
>> below base_overlay) that they want to keep after the job?
>
>
> It's automatically kept, if we use base_overlay->backing->bs as final
> backing node.
>
> You mean, that they want it to be dropped?
Er, yes. Point is, the graph structure below with @base at the root may
be different than the one right below @base_overlay.
> so, assuming the following:
>
> top -(backing)-> manually-inserted-filter -(file)-> base
>
> and user do stream with base=base, and expects filter to be removed by
> stream job?
>
> Hmm, yes, such use-case is broken with our proposed way...
>
>
>
> Let me now clarify the problem we'll have with your way.
>
> When stream don't have any filter, we can easily imagine two parallel
> stream jobs:
>
> top -(backing)-> mid1 -(backing)-> mid2 -(backing)-> base
>
> stream1: top=top, base=mid2
> stream2: top=mid2, base=NULL
>
> final picture is obvious:
>
> top (merged with mid1) -(backing)-> mid2 (merged with base)
Yes, and I don’t think this currently working case is broken by this series.
> But we want stream job has own filter, like mirror.
Which it does not have yet, right? Which is why I was saying that I
don’t think this is a problem with this series. We could try to address
it later.
Or do you think we can’t address it later because right now all filter
cases are broken anyway so now would be the time to make a breaking
change (which the suggestion to not use @base as the final backing node is)?
> So the picture becomes more complex.
>
> Assume
Re: [PATCH v7 14/47] stream: Deal with filters
16.07.2020 17:59, Max Reitz wrote:
On 10.07.20 19:41, Andrey Shinkevich wrote:
On 10.07.2020 18:24, Max Reitz wrote:
On 09.07.20 16:52, Andrey Shinkevich wrote:
On 25.06.2020 18:21, Max Reitz wrote:
Because of the (not so recent anymore) changes that make the stream job
independent of the base node and instead track the node above it, we
have to split that "bottom" node into two cases: The bottom COW node,
and the node directly above the base node (which may be an R/W filter
or the bottom COW node).
Signed-off-by: Max Reitz
---
qapi/block-core.json | 4 +++
block/stream.c | 63
blockdev.c | 4 ++-
3 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b20332e592..df87855429 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2486,6 +2486,10 @@
# On successful completion the image file is updated to drop the
backing file
# and the BLOCK_JOB_COMPLETED event is emitted.
#
+# In case @device is a filter node, block-stream modifies the first
non-filter
+# overlay node below it to point to base's backing node (or NULL if
@base was
+# not specified) instead of modifying @device itself.
+#
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
diff --git a/block/stream.c b/block/stream.c
index aa2e7af98e..b9c1141656 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,7 +31,8 @@ enum {
typedef struct StreamBlockJob {
BlockJob common;
- BlockDriverState *bottom;
+ BlockDriverState *base_overlay; /* COW overlay (stream from
this) */
+ BlockDriverState *above_base; /* Node directly above the base */
Keeping the base_overlay is enough to complete the stream job.
Depends on the definition. If we decide it isn’t enough, then it isn’t
enough.
The above_base may disappear during the job and we can't rely on it.
In this version of this series, it may not, because the chain is frozen.
So the above_base cannot disappear.
Once we insert a filter above the top bs of the stream job, the parallel
jobs in
the iotests #030 will fail with 'frozen link error'. It is because of the
independent parallel stream or commit jobs that insert/remove their filters
asynchroniously.
I’m not sure whether that’s a problem with this series specifically.
We can discuss whether we should allow it to disappear, but I think not.
The problem is, we need something to set as the backing file after
streaming. How do we figure out what that should be? My proposal is we
keep above_base and use its immediate child.
We can do the same with the base_overlay.
If the backing node turns out to be a filter, the proper backing child will
be set after the filter is removed. So, we shouldn't care.
And what if the user manually added some filter above the base (i.e.
below base_overlay) that they want to keep after the job?
It's automatically kept, if we use base_overlay->backing->bs as final backing
node.
You mean, that they want it to be dropped?
so, assuming the following:
top -(backing)-> manually-inserted-filter -(file)-> base
and user do stream with base=base, and expects filter to be removed by stream
job?
Hmm, yes, such use-case is broken with our proposed way...
Let me now clarify the problem we'll have with your way.
When stream don't have any filter, we can easily imagine two parallel stream
jobs:
top -(backing)-> mid1 -(backing)-> mid2 -(backing)-> base
stream1: top=top, base=mid2
stream2: top=mid2, base=NULL
final picture is obvious:
top (merged with mid1) -(backing)-> mid2 (merged with base)
But we want stream job has own filter, like mirror. So the picture becomes more
complex.
Assume stream2 starts first.
top -(backing)-> mid1 -(backing)-> stream2-filter -(backing)-> mid2
-(backing)-> base
Now, when we run stream1, with your solution, stream1 will freeze stream2-filter
(wrong thing, stream2 will fail to remove it if it finished first), and stream1
will
remove stream2-filter on finish (which is wrong as well, stream2 is not
prepared to
removing of its filter)..
But, with our proposed way (freeze only chain up to base_overlay inclusively,
and use backing(base_overlay) as final backing), all will work as expected, and
two parallel jobs will work..
So, these are two mutually exclusive cases.. I vote for freezing up to
base_overlay, and use backing(base_overlay) as final backing, because:
1. I can't imaging other way to fix the case with parallel streams with filters
(it's not a problem of current master, but we have pending series which will
introduce stream job filter, and the problem will appear and even break iotest
30)
2. I don't think that removing filters above base node by stream job is so
important case to break parallel stream jobs in future:
- Stream job is not intended to remove filters, but to
Re: [PATCH v7 14/47] stream: Deal with filters
On 10.07.20 19:41, Andrey Shinkevich wrote:
> On 10.07.2020 18:24, Max Reitz wrote:
>> On 09.07.20 16:52, Andrey Shinkevich wrote:
>>> On 25.06.2020 18:21, Max Reitz wrote:
Because of the (not so recent anymore) changes that make the stream job
independent of the base node and instead track the node above it, we
have to split that "bottom" node into two cases: The bottom COW node,
and the node directly above the base node (which may be an R/W filter
or the bottom COW node).
Signed-off-by: Max Reitz
---
qapi/block-core.json | 4 +++
block/stream.c | 63
blockdev.c | 4 ++-
3 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b20332e592..df87855429 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2486,6 +2486,10 @@
# On successful completion the image file is updated to drop the
backing file
# and the BLOCK_JOB_COMPLETED event is emitted.
#
+# In case @device is a filter node, block-stream modifies the first
non-filter
+# overlay node below it to point to base's backing node (or NULL if
@base was
+# not specified) instead of modifying @device itself.
+#
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
diff --git a/block/stream.c b/block/stream.c
index aa2e7af98e..b9c1141656 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,7 +31,8 @@ enum {
typedef struct StreamBlockJob {
BlockJob common;
- BlockDriverState *bottom;
+ BlockDriverState *base_overlay; /* COW overlay (stream from
this) */
+ BlockDriverState *above_base; /* Node directly above the base */
>>> Keeping the base_overlay is enough to complete the stream job.
>> Depends on the definition. If we decide it isn’t enough, then it isn’t
>> enough.
>>
>>> The above_base may disappear during the job and we can't rely on it.
>> In this version of this series, it may not, because the chain is frozen.
>> So the above_base cannot disappear.
>
> Once we insert a filter above the top bs of the stream job, the parallel
> jobs in
>
> the iotests #030 will fail with 'frozen link error'. It is because of the
>
> independent parallel stream or commit jobs that insert/remove their filters
>
> asynchroniously.
I’m not sure whether that’s a problem with this series specifically.
>> We can discuss whether we should allow it to disappear, but I think not.
>>
>> The problem is, we need something to set as the backing file after
>> streaming. How do we figure out what that should be? My proposal is we
>> keep above_base and use its immediate child.
>
> We can do the same with the base_overlay.
>
> If the backing node turns out to be a filter, the proper backing child will
>
> be set after the filter is removed. So, we shouldn't care.
And what if the user manually added some filter above the base (i.e.
below base_overlay) that they want to keep after the job?
>> If we don’t keep above_base, then we’re basically left guessing as to
>> what should be the backing file after the stream job.
>>
BlockdevOnError on_error;
char *backing_file_str;
bool bs_read_only;
@@ -53,7 +54,7 @@ static void stream_abort(Job *job)
if (s->chain_frozen) {
BlockJob *bjob = >common;
- bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->bottom);
+ bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
}
}
@@ -62,14 +63,15 @@ static int stream_prepare(Job *job)
StreamBlockJob *s = container_of(job, StreamBlockJob,
common.job);
BlockJob *bjob = >common;
BlockDriverState *bs = blk_bs(bjob->blk);
- BlockDriverState *base = backing_bs(s->bottom);
+ BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+ BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
>>> The initial base node may be a top node for a concurrent commit job and
>>>
>>> may disappear.
>> Then it would just be replaced by another node, though, so above_base
>> keeps a child. The @base here is not necessarily the initial @base, and
>> that’s intentional.
>
> Not really. In my example, above_base becomes a dangling
>
> pointer because after the commit job finishes, its filter that should
> belong to the
>
> commit job frozen chain will be deleted. If we freeze the link to the
> above_base
>
> for this job, the iotests #30 will not pass.
So it doesn’t become a dangling pointer, because it’s frozen.
030 passes after this series, so I’m not sure whether I can consider
that problem part of this series.
I think if adding a filter node becomes a
Re: [PATCH v7 14/47] stream: Deal with filters
On 10.07.2020 18:24, Max Reitz wrote:
On 09.07.20 16:52, Andrey Shinkevich wrote:
On 25.06.2020 18:21, Max Reitz wrote:
Because of the (not so recent anymore) changes that make the stream job
independent of the base node and instead track the node above it, we
have to split that "bottom" node into two cases: The bottom COW node,
and the node directly above the base node (which may be an R/W filter
or the bottom COW node).
Signed-off-by: Max Reitz
---
qapi/block-core.json | 4 +++
block/stream.c | 63
blockdev.c | 4 ++-
3 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b20332e592..df87855429 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2486,6 +2486,10 @@
# On successful completion the image file is updated to drop the
backing file
# and the BLOCK_JOB_COMPLETED event is emitted.
#
+# In case @device is a filter node, block-stream modifies the first
non-filter
+# overlay node below it to point to base's backing node (or NULL if
@base was
+# not specified) instead of modifying @device itself.
+#
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
diff --git a/block/stream.c b/block/stream.c
index aa2e7af98e..b9c1141656 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,7 +31,8 @@ enum {
typedef struct StreamBlockJob {
BlockJob common;
- BlockDriverState *bottom;
+ BlockDriverState *base_overlay; /* COW overlay (stream from this) */
+ BlockDriverState *above_base; /* Node directly above the base */
Keeping the base_overlay is enough to complete the stream job.
Depends on the definition. If we decide it isn’t enough, then it isn’t
enough.
The above_base may disappear during the job and we can't rely on it.
In this version of this series, it may not, because the chain is frozen.
So the above_base cannot disappear.
Once we insert a filter above the top bs of the stream job, the parallel
jobs in
the iotests #030 will fail with 'frozen link error'. It is because of the
independent parallel stream or commit jobs that insert/remove their filters
asynchroniously.
We can discuss whether we should allow it to disappear, but I think not.
The problem is, we need something to set as the backing file after
streaming. How do we figure out what that should be? My proposal is we
keep above_base and use its immediate child.
We can do the same with the base_overlay.
If the backing node turns out to be a filter, the proper backing child will
be set after the filter is removed. So, we shouldn't care.
If we don’t keep above_base, then we’re basically left guessing as to
what should be the backing file after the stream job.
BlockdevOnError on_error;
char *backing_file_str;
bool bs_read_only;
@@ -53,7 +54,7 @@ static void stream_abort(Job *job)
if (s->chain_frozen) {
BlockJob *bjob = >common;
- bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->bottom);
+ bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
}
}
@@ -62,14 +63,15 @@ static int stream_prepare(Job *job)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockJob *bjob = >common;
BlockDriverState *bs = blk_bs(bjob->blk);
- BlockDriverState *base = backing_bs(s->bottom);
+ BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+ BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
The initial base node may be a top node for a concurrent commit job and
may disappear.
Then it would just be replaced by another node, though, so above_base
keeps a child. The @base here is not necessarily the initial @base, and
that’s intentional.
Not really. In my example, above_base becomes a dangling
pointer because after the commit job finishes, its filter that should
belong to the
commit job frozen chain will be deleted. If we freeze the link to the
above_base
for this job, the iotests #30 will not pass.
base = bdrv_filter_or_cow_bs(s->base_overlay) is more reliable.
But also wrong. The point of keeping above_base around is to get its
child here to use that child as the new backing child of the top node.
Error *local_err = NULL;
int ret = 0;
- bdrv_unfreeze_backing_chain(bs, s->bottom);
+ bdrv_unfreeze_backing_chain(bs, s->above_base);
s->chain_frozen = false;
- if (bs->backing) {
+ if (bdrv_cow_child(unfiltered_bs)) {
const char *base_id = NULL, *base_fmt = NULL;
if (base) {
base_id = s->backing_file_str;
@@ -77,8 +79,8 @@ static int stream_prepare(Job *job)
base_fmt = base->drv->format_name;
}
}
- bdrv_set_backing_hd(bs, base, _err);
- ret = bdrv_change_backing_file(bs, base_id, base_fmt);
+
Re: [PATCH v7 14/47] stream: Deal with filters
On 09.07.20 17:13, Andrey Shinkevich wrote: > On 25.06.2020 18:21, Max Reitz wrote: >> Because of the (not so recent anymore) changes that make the stream job >> independent of the base node and instead track the node above it, we >> have to split that "bottom" node into two cases: The bottom COW node, >> and the node directly above the base node (which may be an R/W filter >> or the bottom COW node). >> >> Signed-off-by: Max Reitz >> --- >> qapi/block-core.json | 4 +++ >> block/stream.c | 63 >> blockdev.c | 4 ++- >> 3 files changed, 53 insertions(+), 18 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index b20332e592..df87855429 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -2486,6 +2486,10 @@ >> # On successful completion the image file is updated to drop the >> backing file >> # and the BLOCK_JOB_COMPLETED event is emitted. >> # >> +# In case @device is a filter node, block-stream modifies the first >> non-filter >> +# overlay node below it to point to base's backing node (or NULL if >> @base was > > Forgot one thing. To me, it would be more understandable to read > > "...to point to the base as backing node..." because it may be thought > as a backing > > node of the base. This doesn’t sound like it’s about understandability; “point to the base as backing node” and “point to base’s backing node [as backing node]” are semantically different. Was my phrasing just wrong? @base should be the backing node, so yours seems correct. Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v7 14/47] stream: Deal with filters
On 09.07.20 16:52, Andrey Shinkevich wrote:
> On 25.06.2020 18:21, Max Reitz wrote:
>> Because of the (not so recent anymore) changes that make the stream job
>> independent of the base node and instead track the node above it, we
>> have to split that "bottom" node into two cases: The bottom COW node,
>> and the node directly above the base node (which may be an R/W filter
>> or the bottom COW node).
>>
>> Signed-off-by: Max Reitz
>> ---
>> qapi/block-core.json | 4 +++
>> block/stream.c | 63
>> blockdev.c | 4 ++-
>> 3 files changed, 53 insertions(+), 18 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index b20332e592..df87855429 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2486,6 +2486,10 @@
>> # On successful completion the image file is updated to drop the
>> backing file
>> # and the BLOCK_JOB_COMPLETED event is emitted.
>> #
>> +# In case @device is a filter node, block-stream modifies the first
>> non-filter
>> +# overlay node below it to point to base's backing node (or NULL if
>> @base was
>> +# not specified) instead of modifying @device itself.
>> +#
>> # @job-id: identifier for the newly-created block job. If
>> # omitted, the device name will be used. (Since 2.7)
>> #
>> diff --git a/block/stream.c b/block/stream.c
>> index aa2e7af98e..b9c1141656 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -31,7 +31,8 @@ enum {
>> typedef struct StreamBlockJob {
>> BlockJob common;
>> - BlockDriverState *bottom;
>> + BlockDriverState *base_overlay; /* COW overlay (stream from this) */
>> + BlockDriverState *above_base; /* Node directly above the base */
>
> Keeping the base_overlay is enough to complete the stream job.
Depends on the definition. If we decide it isn’t enough, then it isn’t
enough.
> The above_base may disappear during the job and we can't rely on it.
In this version of this series, it may not, because the chain is frozen.
So the above_base cannot disappear.
We can discuss whether we should allow it to disappear, but I think not.
The problem is, we need something to set as the backing file after
streaming. How do we figure out what that should be? My proposal is we
keep above_base and use its immediate child.
If we don’t keep above_base, then we’re basically left guessing as to
what should be the backing file after the stream job.
>> BlockdevOnError on_error;
>> char *backing_file_str;
>> bool bs_read_only;
>> @@ -53,7 +54,7 @@ static void stream_abort(Job *job)
>> if (s->chain_frozen) {
>> BlockJob *bjob = >common;
>> - bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->bottom);
>> + bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
>> }
>> }
>> @@ -62,14 +63,15 @@ static int stream_prepare(Job *job)
>> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>> BlockJob *bjob = >common;
>> BlockDriverState *bs = blk_bs(bjob->blk);
>> - BlockDriverState *base = backing_bs(s->bottom);
>> + BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
>> + BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
>
> The initial base node may be a top node for a concurrent commit job and
>
> may disappear.
Then it would just be replaced by another node, though, so above_base
keeps a child. The @base here is not necessarily the initial @base, and
that’s intentional.
> base = bdrv_filter_or_cow_bs(s->base_overlay) is more reliable.
But also wrong. The point of keeping above_base around is to get its
child here to use that child as the new backing child of the top node.
>> Error *local_err = NULL;
>> int ret = 0;
>> - bdrv_unfreeze_backing_chain(bs, s->bottom);
>> + bdrv_unfreeze_backing_chain(bs, s->above_base);
>> s->chain_frozen = false;
>> - if (bs->backing) {
>> + if (bdrv_cow_child(unfiltered_bs)) {
>> const char *base_id = NULL, *base_fmt = NULL;
>> if (base) {
>> base_id = s->backing_file_str;
>> @@ -77,8 +79,8 @@ static int stream_prepare(Job *job)
>> base_fmt = base->drv->format_name;
>> }
>> }
>> - bdrv_set_backing_hd(bs, base, _err);
>> - ret = bdrv_change_backing_file(bs, base_id, base_fmt);
>> + bdrv_set_backing_hd(unfiltered_bs, base, _err);
>> + ret = bdrv_change_backing_file(unfiltered_bs, base_id,
>> base_fmt);
>> if (local_err) {
>> error_report_err(local_err);
>> return -EPERM;
>> @@ -109,14 +111,15 @@ static int coroutine_fn stream_run(Job *job,
>> Error **errp)
>> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>> BlockBackend *blk = s->common.blk;
>> BlockDriverState *bs = blk_bs(blk);
>> - bool enable_cor = !backing_bs(s->bottom);
>> +
Re: [PATCH v7 14/47] stream: Deal with filters
On 09.07.2020 17:52, Andrey Shinkevich wrote:
On 25.06.2020 18:21, Max Reitz wrote:
Because of the (not so recent anymore) changes that make the stream job
independent of the base node and instead track the node above it, we
have to split that "bottom" node into two cases: The bottom COW node,
and the node directly above the base node (which may be an R/W filter
or the bottom COW node).
Signed-off-by: Max Reitz
---
qapi/block-core.json | 4 +++
block/stream.c | 63
blockdev.c | 4 ++-
3 files changed, 53 insertions(+), 18 deletions(-)
...
+ BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
+ BlockDriverState *above_base;
- if (bdrv_freeze_backing_chain(bs, bottom, errp) < 0) {
+ if (!base_overlay) {
+ error_setg(errp, "'%s' is not in the backing chain of '%s'",
+ base->node_name, bs->node_name);
Sorry, I am not clear with the error message.
In this case, there is no an intermediate COW node but the base, if
not NULL, is
in the backing chain of bs, isn't it?
I am discarding this question. No need to answer.
Andrey
+ return;
+ }
+
Re: [PATCH v7 14/47] stream: Deal with filters
On 25.06.2020 18:21, Max Reitz wrote: Because of the (not so recent anymore) changes that make the stream job independent of the base node and instead track the node above it, we have to split that "bottom" node into two cases: The bottom COW node, and the node directly above the base node (which may be an R/W filter or the bottom COW node). Signed-off-by: Max Reitz --- qapi/block-core.json | 4 +++ block/stream.c | 63 blockdev.c | 4 ++- 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index b20332e592..df87855429 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2486,6 +2486,10 @@ # On successful completion the image file is updated to drop the backing file # and the BLOCK_JOB_COMPLETED event is emitted. # +# In case @device is a filter node, block-stream modifies the first non-filter +# overlay node below it to point to base's backing node (or NULL if @base was Forgot one thing. To me, it would be more understandable to read "...to point to the base as backing node..." because it may be thought as a backing node of the base. Andrey +# not specified) instead of modifying @device itself. +# # @job-id: identifier for the newly-created block job. If # omitted, the device name will be used. (Since 2.7) #
Re: [PATCH v7 14/47] stream: Deal with filters
On 25.06.2020 18:21, Max Reitz wrote:
Because of the (not so recent anymore) changes that make the stream job
independent of the base node and instead track the node above it, we
have to split that "bottom" node into two cases: The bottom COW node,
and the node directly above the base node (which may be an R/W filter
or the bottom COW node).
Signed-off-by: Max Reitz
---
qapi/block-core.json | 4 +++
block/stream.c | 63
blockdev.c | 4 ++-
3 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b20332e592..df87855429 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2486,6 +2486,10 @@
# On successful completion the image file is updated to drop the backing file
# and the BLOCK_JOB_COMPLETED event is emitted.
#
+# In case @device is a filter node, block-stream modifies the first non-filter
+# overlay node below it to point to base's backing node (or NULL if @base was
+# not specified) instead of modifying @device itself.
+#
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
diff --git a/block/stream.c b/block/stream.c
index aa2e7af98e..b9c1141656 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,7 +31,8 @@ enum {
typedef struct StreamBlockJob {
BlockJob common;
-BlockDriverState *bottom;
+BlockDriverState *base_overlay; /* COW overlay (stream from this) */
+BlockDriverState *above_base; /* Node directly above the base */
Keeping the base_overlay is enough to complete the stream job.
The above_base may disappear during the job and we can't rely on it.
BlockdevOnError on_error;
char *backing_file_str;
bool bs_read_only;
@@ -53,7 +54,7 @@ static void stream_abort(Job *job)
if (s->chain_frozen) {
BlockJob *bjob = >common;
-bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->bottom);
+bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
}
}
@@ -62,14 +63,15 @@ static int stream_prepare(Job *job)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockJob *bjob = >common;
BlockDriverState *bs = blk_bs(bjob->blk);
-BlockDriverState *base = backing_bs(s->bottom);
+BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
The initial base node may be a top node for a concurrent commit job and
may disappear. It is true for the above_base as well.
base = bdrv_filter_or_cow_bs(s->base_overlay) is more reliable.
Error *local_err = NULL;
int ret = 0;
-bdrv_unfreeze_backing_chain(bs, s->bottom);
+bdrv_unfreeze_backing_chain(bs, s->above_base);
s->chain_frozen = false;
-if (bs->backing) {
+if (bdrv_cow_child(unfiltered_bs)) {
const char *base_id = NULL, *base_fmt = NULL;
if (base) {
base_id = s->backing_file_str;
@@ -77,8 +79,8 @@ static int stream_prepare(Job *job)
base_fmt = base->drv->format_name;
}
}
-bdrv_set_backing_hd(bs, base, _err);
-ret = bdrv_change_backing_file(bs, base_id, base_fmt);
+bdrv_set_backing_hd(unfiltered_bs, base, _err);
+ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt);
if (local_err) {
error_report_err(local_err);
return -EPERM;
@@ -109,14 +111,15 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockBackend *blk = s->common.blk;
BlockDriverState *bs = blk_bs(blk);
-bool enable_cor = !backing_bs(s->bottom);
+BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+bool enable_cor = !bdrv_cow_child(s->base_overlay);
int64_t len;
int64_t offset = 0;
uint64_t delay_ns = 0;
int error = 0;
int64_t n = 0; /* bytes */
-if (bs == s->bottom) {
+if (unfiltered_bs == s->base_overlay) {
/* Nothing to stream */
return 0;
}
@@ -150,13 +153,14 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
copy = false;
-ret = bdrv_is_allocated(bs, offset, STREAM_CHUNK, );
+ret = bdrv_is_allocated(unfiltered_bs, offset, STREAM_CHUNK, );
if (ret == 1) {
/* Allocated in the top, no need to copy. */
} else if (ret >= 0) {
/* Copy if allocated in the intermediate images. Limit to the
* known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE). */
-ret = bdrv_is_allocated_above(backing_bs(bs), s->bottom, true,
+ret = bdrv_is_allocated_above(bdrv_cow_bs(unfiltered_bs),
+ s->base_overlay, true,
[PATCH v7 14/47] stream: Deal with filters
Because of the (not so recent anymore) changes that make the stream job
independent of the base node and instead track the node above it, we
have to split that "bottom" node into two cases: The bottom COW node,
and the node directly above the base node (which may be an R/W filter
or the bottom COW node).
Signed-off-by: Max Reitz
---
qapi/block-core.json | 4 +++
block/stream.c | 63
blockdev.c | 4 ++-
3 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b20332e592..df87855429 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2486,6 +2486,10 @@
# On successful completion the image file is updated to drop the backing file
# and the BLOCK_JOB_COMPLETED event is emitted.
#
+# In case @device is a filter node, block-stream modifies the first non-filter
+# overlay node below it to point to base's backing node (or NULL if @base was
+# not specified) instead of modifying @device itself.
+#
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
diff --git a/block/stream.c b/block/stream.c
index aa2e7af98e..b9c1141656 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,7 +31,8 @@ enum {
typedef struct StreamBlockJob {
BlockJob common;
-BlockDriverState *bottom;
+BlockDriverState *base_overlay; /* COW overlay (stream from this) */
+BlockDriverState *above_base; /* Node directly above the base */
BlockdevOnError on_error;
char *backing_file_str;
bool bs_read_only;
@@ -53,7 +54,7 @@ static void stream_abort(Job *job)
if (s->chain_frozen) {
BlockJob *bjob = >common;
-bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->bottom);
+bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
}
}
@@ -62,14 +63,15 @@ static int stream_prepare(Job *job)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockJob *bjob = >common;
BlockDriverState *bs = blk_bs(bjob->blk);
-BlockDriverState *base = backing_bs(s->bottom);
+BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
Error *local_err = NULL;
int ret = 0;
-bdrv_unfreeze_backing_chain(bs, s->bottom);
+bdrv_unfreeze_backing_chain(bs, s->above_base);
s->chain_frozen = false;
-if (bs->backing) {
+if (bdrv_cow_child(unfiltered_bs)) {
const char *base_id = NULL, *base_fmt = NULL;
if (base) {
base_id = s->backing_file_str;
@@ -77,8 +79,8 @@ static int stream_prepare(Job *job)
base_fmt = base->drv->format_name;
}
}
-bdrv_set_backing_hd(bs, base, _err);
-ret = bdrv_change_backing_file(bs, base_id, base_fmt);
+bdrv_set_backing_hd(unfiltered_bs, base, _err);
+ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt);
if (local_err) {
error_report_err(local_err);
return -EPERM;
@@ -109,14 +111,15 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockBackend *blk = s->common.blk;
BlockDriverState *bs = blk_bs(blk);
-bool enable_cor = !backing_bs(s->bottom);
+BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+bool enable_cor = !bdrv_cow_child(s->base_overlay);
int64_t len;
int64_t offset = 0;
uint64_t delay_ns = 0;
int error = 0;
int64_t n = 0; /* bytes */
-if (bs == s->bottom) {
+if (unfiltered_bs == s->base_overlay) {
/* Nothing to stream */
return 0;
}
@@ -150,13 +153,14 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
copy = false;
-ret = bdrv_is_allocated(bs, offset, STREAM_CHUNK, );
+ret = bdrv_is_allocated(unfiltered_bs, offset, STREAM_CHUNK, );
if (ret == 1) {
/* Allocated in the top, no need to copy. */
} else if (ret >= 0) {
/* Copy if allocated in the intermediate images. Limit to the
* known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE). */
-ret = bdrv_is_allocated_above(backing_bs(bs), s->bottom, true,
+ret = bdrv_is_allocated_above(bdrv_cow_bs(unfiltered_bs),
+ s->base_overlay, true,
offset, n, );
/* Finish early if end of backing file has been reached */
if (ret == 0 && n == 0) {
@@ -223,9 +227,29 @@ void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *iter;
bool bs_read_only;
int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
-BlockDriverState *bottom = bdrv_find_overlay(bs, base);
+BlockDriverState *base_overlay =
