| Message ID | 20170803150301.10177-3-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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 > >
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"}}
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?
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;
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(-)