Re: [PATCH v7 39/47] blockdev: Fix active commit choice
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
> Reviewed-by: Vladimir Sementsov-Ogievskiy
> ---
> 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 valu
Re: [PATCH v7 39/47] blockdev: Fix active commit choice
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
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
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.
> 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.
Re: [PATCH v7 39/47] blockdev: Fix active commit choice
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
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy
> >> ---
> >> 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.
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.
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.
> 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..."?
> 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.
Kevin
signature.asc
Description: PGP signature
Re: [PATCH v7 39/47] blockdev: Fix active commit choice
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
>> Reviewed-by: Vladimir Sementsov-Ogievskiy
>> ---
>> 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.
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 [...]”?
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)”.)
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v7 39/47] blockdev: Fix active commit choice
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
> Reviewed-by: Vladimir Sementsov-Ogievskiy
> ---
> 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".
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.
> goto out;
> }
> -commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
> -job_flags, speed, on_error,
> +if (!has_job_id) {
> +/*
> + * Emulate here what block_job_create() does, because it
> + * is possible that @bs != @top_bs (the block job should
> + * be named after @bs, even if @top_bs is the actual
> + * source)
> + */
Should it? Oh, yes, looks like it. block-commit is weird. :-)
> +job_id = bdrv_get_device_name(bs);
> +}
> +commit_active_start(job_id, top_bs, base_bs, job_flags, speed,
> on_error,
> filter_node_name, NULL, NULL, false, &local_err);
> } else {
> BlockDriverState *overlay_bs = bdrv_find_overlay(bs, top_bs);
Kevin
[PATCH v7 39/47] blockdev: Fix active commit choice
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
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
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. Historically, we have always
+ * used active commit for top nodes, so continue that practice.
+ * (Active commit is never really wrong.)
+ */
+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");
goto out;
}
-commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
-job_flags, speed, on_error,
+if (!has_job_id) {
+/*
+ * Emulate here what block_job_create() does, because it
+ * is possible that @bs != @top_bs (the block job should
+ * be named after @bs, even if @top_bs is the actual
+ * source)
+ */
+job_id = bdrv_get_device_name(bs);
+}
+commit_active_start(job_id, top_bs, base_bs, job_flags, speed,
on_error,
filter_node_name, NULL, NULL, false, &local_err);
} else {
BlockDriverState *overlay_bs = bdrv_find_overlay(bs, top_bs);
--
2.26.2
