BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
reopen a node read-write temporarily because the user requested
read-write for the top-level image, but qemu decided that read-only is
enough for this node (a backing file).
bdrv_reopen() is different, it is also used for cases where the user
changed their mind and wants to update the options. There is no reason
to forbid making a node read-write in that case.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block.h | 3 ++-
block.c | 11 +++++++----
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 34770bb33a..ab80195378 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -436,7 +436,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
bool bdrv_is_read_only(BlockDriverState *bs);
bool bdrv_is_writable(BlockDriverState *bs);
-int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
+ bool ignore_allow_rdw, Error **errp);
int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
bool bdrv_is_sg(BlockDriverState *bs);
bool bdrv_is_inserted(BlockDriverState *bs);
diff --git a/block.c b/block.c
index ab908cdc50..2711c3dd3b 100644
--- a/block.c
+++ b/block.c
@@ -246,7 +246,8 @@ bool bdrv_is_writable(BlockDriverState *bs)
return !bdrv_is_read_only(bs) && !(bs->open_flags & BDRV_O_INACTIVE);
}
-int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
+ bool ignore_allow_rdw, Error **errp)
{
/* Do not set read_only if copy_on_read is enabled */
if (bs->copy_on_read && read_only) {
@@ -256,7 +257,9 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
}
/* Do not clear read_only if it is prohibited */
- if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
+ if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR) &&
+ !ignore_allow_rdw)
+ {
error_setg(errp, "Node '%s' is read only",
bdrv_get_device_or_node_name(bs));
return -EPERM;
@@ -269,7 +272,7 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
{
int ret = 0;
- ret = bdrv_can_set_read_only(bs, read_only, errp);
+ ret = bdrv_can_set_read_only(bs, read_only, false, errp);
if (ret < 0) {
return ret;
}
@@ -2907,7 +2910,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
* to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
* not set, or if the BDS still has copy_on_read enabled */
read_only = !(reopen_state->flags & BDRV_O_RDWR);
- ret = bdrv_can_set_read_only(reopen_state->bs, read_only, &local_err);
+ ret = bdrv_can_set_read_only(reopen_state->bs, read_only, true, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto error;
--
2.13.3
On 08/03/2017 10:02 AM, Kevin Wolf wrote: > BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally > reopen a node read-write temporarily because the user requested > read-write for the top-level image, but qemu decided that read-only is > enough for this node (a backing file). > > bdrv_reopen() is different, it is also used for cases where the user > changed their mind and wants to update the options. There is no reason > to forbid making a node read-write in that case. Hmm, I wonder. https://bugzilla.redhat.com/show_bug.cgi?id=1465320 details a failure when starting qemu with a read-write NBD disk, then taking several snapshots (nbd <- snap1 <- snap2 <- snap3), then where intermediate commit (snap2 into nbd) works but live commit (snap3 into nbd) fails with a message that nbd does not support reopening. I'm presuming that your series may help to address that; I'll give it a spin and see what happens. But first, the code review: > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/block/block.h | 3 ++- > block.c | 11 +++++++---- > 2 files changed, 9 insertions(+), 5 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 08/03/2017 10:21 AM, Eric Blake wrote:
> On 08/03/2017 10:02 AM, Kevin Wolf wrote:
>> BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
>> reopen a node read-write temporarily because the user requested
>> read-write for the top-level image, but qemu decided that read-only is
>> enough for this node (a backing file).
>>
>> bdrv_reopen() is different, it is also used for cases where the user
>> changed their mind and wants to update the options. There is no reason
>> to forbid making a node read-write in that case.
>
> Hmm, I wonder. https://bugzilla.redhat.com/show_bug.cgi?id=1465320
> details a failure when starting qemu with a read-write NBD disk, then
> taking several snapshots (nbd <- snap1 <- snap2 <- snap3), then where
> intermediate commit (snap2 into nbd) works but live commit (snap3 into
> nbd) fails with a message that nbd does not support reopening. I'm
> presuming that your series may help to address that; I'll give it a spin
> and see what happens.
Nope, even with your patches, I'm still getting:
{'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar2'}}
{"return": {}}
{"timestamp": {"seconds": 1501811285, "microseconds": 439748}, "event":
"BLOCK_JOB_COMPLETED", "data": {"device": "drive-image1", "len":
2097152, "offset": 2097152, "speed": 0, "type": "commit"}}
{'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar3'}}
{"error": {"class": "GenericError", "desc": "Block format 'nbd' used by
node '#block048' does not support reopening files"}}
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Am 04.08.2017 um 03:49 hat Eric Blake geschrieben:
> On 08/03/2017 10:21 AM, Eric Blake wrote:
> > On 08/03/2017 10:02 AM, Kevin Wolf wrote:
> >> BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
> >> reopen a node read-write temporarily because the user requested
> >> read-write for the top-level image, but qemu decided that read-only is
> >> enough for this node (a backing file).
> >>
> >> bdrv_reopen() is different, it is also used for cases where the user
> >> changed their mind and wants to update the options. There is no reason
> >> to forbid making a node read-write in that case.
> >
> > Hmm, I wonder. https://bugzilla.redhat.com/show_bug.cgi?id=1465320
> > details a failure when starting qemu with a read-write NBD disk, then
> > taking several snapshots (nbd <- snap1 <- snap2 <- snap3), then where
> > intermediate commit (snap2 into nbd) works but live commit (snap3 into
> > nbd) fails with a message that nbd does not support reopening. I'm
> > presuming that your series may help to address that; I'll give it a spin
> > and see what happens.
>
> Nope, even with your patches, I'm still getting:
>
> {'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar2'}}
> {"return": {}}
> {"timestamp": {"seconds": 1501811285, "microseconds": 439748}, "event":
> "BLOCK_JOB_COMPLETED", "data": {"device": "drive-image1", "len":
> 2097152, "offset": 2097152, "speed": 0, "type": "commit"}}
>
> {'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar3'}}
> {"error": {"class": "GenericError", "desc": "Block format 'nbd' used by
> node '#block048' does not support reopening files"}}
That's simply NBD not implementing .bdrv_reopen_*. In other words, not a
bug, but just a missing feature.
Kevin
On 08/04/2017 05:20 AM, Kevin Wolf wrote:
>>>
>>> Hmm, I wonder. https://bugzilla.redhat.com/show_bug.cgi?id=1465320
>>> details a failure when starting qemu with a read-write NBD disk, then
>>> taking several snapshots (nbd <- snap1 <- snap2 <- snap3), then where
>>> intermediate commit (snap2 into nbd) works but live commit (snap3 into
>>> nbd) fails with a message that nbd does not support reopening. I'm
>>> presuming that your series may help to address that; I'll give it a spin
>>> and see what happens.
>>
>> Nope, even with your patches, I'm still getting:
>>
>> {'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar2'}}
>> {"return": {}}
>> {"timestamp": {"seconds": 1501811285, "microseconds": 439748}, "event":
>> "BLOCK_JOB_COMPLETED", "data": {"device": "drive-image1", "len":
>> 2097152, "offset": 2097152, "speed": 0, "type": "commit"}}
>>
>> {'execute':'block-commit','arguments':{'device':'drive-image1','top':'bar3'}}
>> {"error": {"class": "GenericError", "desc": "Block format 'nbd' used by
>> node '#block048' does not support reopening files"}}
>
> That's simply NBD not implementing .bdrv_reopen_*. In other words, not a
> bug, but just a missing feature.
But why does intermediate commit work, while live commit fails? Both
require that the image being committed into be read-write, and the NBD
image was opened read-write (even if it can be treated as read-only for
the duration of time that it is a backing image). I understand missing
.bdrv_reopen making it impossible to change read-only to read-write, but
when missing it means you can never change read-write down to the
tighter read-only originally, then what is making live commit different
from intermediate in not being able to handle it?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On Thu, Aug 03, 2017 at 05:02:58PM +0200, Kevin Wolf wrote:
> BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
> reopen a node read-write temporarily because the user requested
> read-write for the top-level image, but qemu decided that read-only is
> enough for this node (a backing file).
>
> bdrv_reopen() is different, it is also used for cases where the user
> changed their mind and wants to update the options. There is no reason
> to forbid making a node read-write in that case.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> include/block/block.h | 3 ++-
> block.c | 11 +++++++----
> 2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 34770bb33a..ab80195378 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -436,7 +436,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
>
> bool bdrv_is_read_only(BlockDriverState *bs);
> bool bdrv_is_writable(BlockDriverState *bs);
> -int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> +int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
> + bool ignore_allow_rdw, Error **errp);
> int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> bool bdrv_is_sg(BlockDriverState *bs);
> bool bdrv_is_inserted(BlockDriverState *bs);
> diff --git a/block.c b/block.c
> index ab908cdc50..2711c3dd3b 100644
> --- a/block.c
> +++ b/block.c
> @@ -246,7 +246,8 @@ bool bdrv_is_writable(BlockDriverState *bs)
> return !bdrv_is_read_only(bs) && !(bs->open_flags & BDRV_O_INACTIVE);
> }
>
> -int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> +int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
> + bool ignore_allow_rdw, Error **errp)
I think 'override_allow_rdw' may be a bit clearer, but that is just
bikeshedding.
Reviewed-by: Jeff Cody <jcody@redhat.com>
> {
> /* Do not set read_only if copy_on_read is enabled */
> if (bs->copy_on_read && read_only) {
> @@ -256,7 +257,9 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> }
>
> /* Do not clear read_only if it is prohibited */
> - if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
> + if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR) &&
> + !ignore_allow_rdw)
> + {
> error_setg(errp, "Node '%s' is read only",
> bdrv_get_device_or_node_name(bs));
> return -EPERM;
> @@ -269,7 +272,7 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> {
> int ret = 0;
>
> - ret = bdrv_can_set_read_only(bs, read_only, errp);
> + ret = bdrv_can_set_read_only(bs, read_only, false, errp);
> if (ret < 0) {
> return ret;
> }
> @@ -2907,7 +2910,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
> * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
> * not set, or if the BDS still has copy_on_read enabled */
> read_only = !(reopen_state->flags & BDRV_O_RDWR);
> - ret = bdrv_can_set_read_only(reopen_state->bs, read_only, &local_err);
> + ret = bdrv_can_set_read_only(reopen_state->bs, read_only, true, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> goto error;
> --
> 2.13.3
>
>
© 2016 - 2026 Red Hat, Inc.