RBD reopen, read_only cleanup
cover RBD reopen, read_only cleanup
This series does two things:
1.) Cleans up some of the logic behind setting the read_only flag
for a BDS in the block layer, so that it is done consistently (and rules are applied consistently), and
2.) Adds .bdrv_reopen_prepare() implementation for RBD, so that block
jobs can be run on backing chains that have rbd protocol nodes.
Jeff Cody (9):
block: add bdrv_set_read_only() helper function
block: do not set BDS read_only if copy_on_read enabled
block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
block: code movement
block: introduce bdrv_try_set_read_only()
block: use bdrv_try_set_read_only() during reopen block/rbd - update variable names to more apt names block/rbd: do not blindly set bs->read_only block/rbd: Add support for reopen()
block.c | 56 +++++++++++++++++++++++++++++++++++--------- block/bochs.c | 5 +++- block/cloop.c | 5 +++- block/dmg.c | 6 ++++- block/rbd.c | 65 +++++++++++++++++++++++++++++++++++++-------------- block/vvfat.c | 15 +++++++++--- include/block/block.h | 2 ++ 7 files changed, 120 insertions(+), 34 deletions(-)
Signature · 1 line
2.9.3
PATCH 1/9 block: add bdrv_set_read_only() helper function
We have a helper wrapper for checking for the BDS read_only flag, add a helper wrapper to set the read_only flag as well.
--- block.c | 5 +++++ block/bochs.c | 2 +- block/cloop.c | 2 +- block/dmg.c | 2 +- block/rbd.c | 2 +- block/vvfat.c | 4 ++-- include/block/block.h | 1 + 7 files changed, 12 insertions(+), 6 deletions(-)
block.c +5 -0
@@ -192,6 +192,11 @@ void path_combine(char *dest, int dest_size, } } +void bdrv_set_read_only (BlockDriverState *bs,bool read_only) +{ + bs->read_only = read_only; +} + void bdrv_get_full_backing_filename_from_filename(const char *backed, const char *backing, char *dest, size_t sz,
block/bochs.c +1 -1
@@ -110,7 +110,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,return -EINVAL ; } - bs->read_only = true;/* no write support yet */ +bdrv_set_read_only (bs, true);/* no write support yet */ ret =bdrv_pread (bs->file,0 , &bochs,sizeof (bochs));if (ret <0 ) {
block/cloop.c +1 -1
@@ -72,7 +72,7 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,return -EINVAL ; } - bs->read_only = true; +bdrv_set_read_only (bs, true);/* read header */ ret =bdrv_pread (bs->file,128 , &s->block_size,4 );
block/dmg.c +1 -1
@@ -420,7 +420,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, }block_module_load_one ("dmg-bz2" ); - bs->read_only = true; +bdrv_set_read_only (bs, true); s->n_chunks =0 ; s->offsets = s->lengths = s->sectors = s->sectorcounts =NULL ;
block/rbd.c +1 -1
@@ -641,7 +641,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, goto failed_open; } - bs->read_only = (s->snap !=NULL ); +bdrv_set_read_only (bs, (s->snap !=NULL ));qemu_opts_del (opts);return 0 ;
block/vvfat.c +2 -2
@@ -1157,7 +1157,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, s->current_cluster=0xffffffff ;/* read only is the default for safety */ - bs->read_only = true; +bdrv_set_read_only (bs, true); s->qcow =NULL ; s->qcow_filename =NULL ; s->fat2 =NULL ;
@@ -1173,7 +1173,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,if (ret <0 ) { goto fail; } - bs->read_only = false; +bdrv_set_read_only (bs, false); } bs->total_sectors = cyls * heads * secs;
include/block/block.h +1 -0
@@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, int64_t sector_num, int nb_sectors, int *pnum);bool bdrv_is_read_only (BlockDriverState *bs); +void bdrv_set_read_only (BlockDriverState *bs,bool read_only);bool bdrv_is_sg (BlockDriverState *bs);bool bdrv_is_inserted (BlockDriverState *bs);int bdrv_media_changed (BlockDriverState *bs);
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH for-2.10 1/9] block: add bdrv_set_read_only() helper function
Quoted 4 lines · Jeff Cody
We have a helper wrapper for checking for the BDS
read_onlyflag, add a helper wrapper to set theread_onlyflag as well.Signed-off-by Jeff Cody <[email protected]>
Quoted 9 lines
--- block.c | 5 +++++ block/bochs.c | 2 +- block/cloop.c | 2 +- block/dmg.c | 2 +- block/rbd.c | 2 +- block/vvfat.c | 4 ++-- include/block/block.h | 1 + 7 files changed, 12 insertions(+), 6 deletions(-)
PATCH 2/9 block: do not set BDS read_only if copy_on_read enabled
In reply to Jeff CodyA few block drivers will set the BDS read_only flag from their .bdrv_open() function. This means the bs->read_only flag could be set after we enable copy_on_read, as the BDRV_O_COPY_ON_READ flag check occurs prior to the call to bdrv->bdrv_open().
This adds an error return to bdrv_set_read_only(), and an error will be return if we try to set the BDS to read_only while copy_on_read is enabled.
--- block.c | 10 +++++++++- block/bochs.c | 5 ++++- block/cloop.c | 5 ++++- block/dmg.c | 6 +++++- block/rbd.c | 6 +++++- block/vvfat.c | 15 ++++++++++++--- include/block/block.h | 2 +- 7 files changed, 40 insertions(+), 9 deletions(-)
block.c +9 -1
@@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size, } } -void bdrv_set_read_only (BlockDriverState *bs,bool read_only) +int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) { +/* Do not set read_only if copy_on_read is enabled */ +if (bs->copy_on_read && read_only) { +error_setg (errp,"Cannot set node '%s' to r/o while COW enabled" , +bdrv_get_device_or_node_name (bs)); +return -EINVAL ; + } + bs->read_only = read_only; +return 0 ; } void bdrv_get_full_backing_filename_from_filename(const char *backed,
block/bochs.c +4 -1
@@ -110,7 +110,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,return -EINVAL ; } -bdrv_set_read_only (bs, true);/* no write support yet */ + ret =bdrv_set_read_only (bs, true, errp);/* no write support yet */ +if (ret <0 ) { +return ret; + } ret =bdrv_pread (bs->file,0 , &bochs,sizeof (bochs));if (ret <0 ) {
block/cloop.c +4 -1
@@ -72,7 +72,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,return -EINVAL ; } -bdrv_set_read_only (bs, true); + ret =bdrv_set_read_only (bs, true, errp); +if (ret <0 ) { +return ret; + }/* read header */ ret =bdrv_pread (bs->file,128 , &s->block_size,4 );
block/dmg.c +5 -1
@@ -419,8 +419,12 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,return -EINVAL ; } + ret =bdrv_set_read_only (bs, true, errp); +if (ret <0 ) { +return ret; + } +block_module_load_one ("dmg-bz2" ); -bdrv_set_read_only (bs, true); s->n_chunks =0 ; s->offsets = s->lengths = s->sectors = s->sectorcounts =NULL ;
block/rbd.c +5 -1
@@ -641,7 +641,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, goto failed_open; } -bdrv_set_read_only (bs, (s->snap !=NULL )); + r =bdrv_set_read_only (bs, (s->snap !=NULL ), &local_err); +if (r <0 ) { +error_propagate (errp, local_err); + goto failed_open; + }qemu_opts_del (opts);return 0 ;
block/vvfat.c +12 -3
@@ -1156,8 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, s->current_cluster=0xffffffff ; -/* read only is the default for safety */ -bdrv_set_read_only (bs, true); s->qcow =NULL ; s->qcow_filename =NULL ; s->fat2 =NULL ;
@@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,if (ret <0 ) { goto fail; } -bdrv_set_read_only (bs, false); + ret =bdrv_set_read_only (bs, false, &local_err); +if (ret <0 ) { +error_propagate (errp, local_err); + goto fail; + } + }else { +/* read only is the default for safety */ + ret =bdrv_set_read_only (bs, true, &local_err); +if (ret <0 ) { +error_propagate (errp, local_err); + goto fail; + } } bs->total_sectors = cyls * heads * secs;
include/block/block.h +1 -1
@@ -426,7 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, int64_t sector_num, int nb_sectors, int *pnum);bool bdrv_is_read_only (BlockDriverState *bs); -void bdrv_set_read_only (BlockDriverState *bs,bool read_only); +int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp);bool bdrv_is_sg (BlockDriverState *bs);bool bdrv_is_inserted (BlockDriverState *bs);int bdrv_media_changed (BlockDriverState *bs);
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled
Quoted 25 lines · Jeff Cody
A few block drivers will set the BDS
read_onlyflag from their .bdrv_open() function. This means thebs->read_onlyflag could be set after we enablecopy_on_read, as theBDRV_O_COPY_ON_READflag check occurs prior to the call tobdrv->bdrv_open().This adds an error return to
bdrv_set_read_only(), and an error will be return if we try to set the BDS toread_onlywhilecopy_on_readis enabled.Signed-off-by Jeff Cody <[email protected]>--- block.c | 10 +++++++++- block/bochs.c | 5 ++++- block/cloop.c | 5 ++++- block/dmg.c | 6 +++++- block/rbd.c | 6 +++++- block/vvfat.c | 15 ++++++++++++--- include/block/block.h | 2 +- 7 files changed, 40 insertions(+), 9 deletions(-)block.c +0
-0
block.c void path_combine(char *dest, int dest_size, · line 199
Quoted 9 lines
block.c +4
-1@@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size, } } -void bdrv_set_read_only (BlockDriverState *bs,bool read_only) +int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) { +/* Do not set read_only if copy_on_read is enabled */ +if (bs->copy_on_read && read_only) { + error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",
COW?
Quoted 123 lines
block.c +5
-0+ bdrv_get_device_or_node_name(bs)); +return -EINVAL ; + } + bs->read_only = read_only; +return 0 ; } void bdrv_get_full_backing_filename_from_filename(const char *backed,block/bochs.c +4
-1@@ -110,7 +110,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,return -EINVAL ; } -bdrv_set_read_only (bs, true);/* no write support yet */ + ret =bdrv_set_read_only (bs, true, errp);/* no write support yet */ +if (ret <0 ) { +return ret; + } ret =bdrv_pread (bs->file,0 , &bochs,sizeof (bochs));if (ret <0 ) {block/cloop.c +4
-1@@ -72,7 +72,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,return -EINVAL ; } -bdrv_set_read_only (bs, true); + ret =bdrv_set_read_only (bs, true, errp); +if (ret <0 ) { +return ret; + }/* read header */ ret =bdrv_pread (bs->file,128 , &s->block_size,4 );block/dmg.c +5
-1@@ -419,8 +419,12 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,return -EINVAL ; } + ret =bdrv_set_read_only (bs, true, errp); +if (ret <0 ) { +return ret; + } +block_module_load_one ("dmg-bz2" ); -bdrv_set_read_only (bs, true); s->n_chunks =0 ; s->offsets = s->lengths = s->sectors = s->sectorcounts =NULL ;block/rbd.c +5
-1@@ -641,7 +641,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, goto failed_open; } -bdrv_set_read_only (bs, (s->snap !=NULL )); + r =bdrv_set_read_only (bs, (s->snap !=NULL ), &local_err); +if (r <0 ) { +error_propagate (errp, local_err); + goto failed_open; + }qemu_opts_del (opts);return 0 ;block/vvfat.c +12
-3@@ -1156,8 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, s->current_cluster=0xffffffff ; -/* read only is the default for safety */ -bdrv_set_read_only (bs, true); s->qcow =NULL ; s->qcow_filename =NULL ; s->fat2 =NULL ;@@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,if (ret <0 ) { goto fail; } -bdrv_set_read_only (bs, false); + ret =bdrv_set_read_only (bs, false, &local_err); +if (ret <0 ) { +error_propagate (errp, local_err); + goto fail; + } + }else { +/* read only is the default for safety */ + ret =bdrv_set_read_only (bs, true, &local_err); +if (ret <0 ) { +error_propagate (errp, local_err); + goto fail; + } } bs->total_sectors = cyls * heads * secs;include/block/block.h +1
-1@@ -426,7 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, int64_t sector_num, int nb_sectors, int *pnum);bool bdrv_is_read_only (BlockDriverState *bs); -void bdrv_set_read_only (BlockDriverState *bs,bool read_only); +int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp);bool bdrv_is_sg (BlockDriverState *bs);bool bdrv_is_inserted (BlockDriverState *bs);int bdrv_media_changed (BlockDriverState *bs);
Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled
Quoted 31 lines · John Snow
Quoted 28 lines · Jeff Cody
A few block drivers will set the BDS
read_onlyflag from their .bdrv_open() function. This means thebs->read_onlyflag could be set after we enablecopy_on_read, as theBDRV_O_COPY_ON_READflag check occurs prior to the call tobdrv->bdrv_open().This adds an error return to
bdrv_set_read_only(), and an error will be return if we try to set the BDS toread_onlywhilecopy_on_readis enabled.Signed-off-by Jeff Cody <[email protected]>--- block.c | 10 +++++++++- block/bochs.c | 5 ++++- block/cloop.c | 5 ++++- block/dmg.c | 6 +++++- block/rbd.c | 6 +++++- block/vvfat.c | 15 ++++++++++++--- include/block/block.h | 2 +- 7 files changed, 40 insertions(+), 9 deletions(-)block.c +0
-0@@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size, } }
Quoted 9 lines
block.c void path_combine(char *dest, int dest_size, · line 199
Quoted 6 lines
block.c +4
-1-void bdrv_set_read_only (BlockDriverState *bs,bool read_only) +int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) { +/* Do not set read_only if copy_on_read is enabled */ +if (bs->copy_on_read && read_only) { + error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",COW?
Mooo! You are right, that should be COR (or better yet, I should just write it out - copy on read).
Quoted 123 lines
Quoted 123 lines
block.c +5
-0+ bdrv_get_device_or_node_name(bs)); +return -EINVAL ; + } + bs->read_only = read_only; +return 0 ; } void bdrv_get_full_backing_filename_from_filename(const char *backed,block/bochs.c +4
-1@@ -110,7 +110,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,return -EINVAL ; } -bdrv_set_read_only (bs, true);/* no write support yet */ + ret =bdrv_set_read_only (bs, true, errp);/* no write support yet */ +if (ret <0 ) { +return ret; + } ret =bdrv_pread (bs->file,0 , &bochs,sizeof (bochs));if (ret <0 ) {block/cloop.c +4
-1@@ -72,7 +72,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,return -EINVAL ; } -bdrv_set_read_only (bs, true); + ret =bdrv_set_read_only (bs, true, errp); +if (ret <0 ) { +return ret; + }/* read header */ ret =bdrv_pread (bs->file,128 , &s->block_size,4 );block/dmg.c +5
-1@@ -419,8 +419,12 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,return -EINVAL ; } + ret =bdrv_set_read_only (bs, true, errp); +if (ret <0 ) { +return ret; + } +block_module_load_one ("dmg-bz2" ); -bdrv_set_read_only (bs, true); s->n_chunks =0 ; s->offsets = s->lengths = s->sectors = s->sectorcounts =NULL ;block/rbd.c +5
-1@@ -641,7 +641,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, goto failed_open; } -bdrv_set_read_only (bs, (s->snap !=NULL )); + r =bdrv_set_read_only (bs, (s->snap !=NULL ), &local_err); +if (r <0 ) { +error_propagate (errp, local_err); + goto failed_open; + }qemu_opts_del (opts);return 0 ;block/vvfat.c +12
-3@@ -1156,8 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, s->current_cluster=0xffffffff ; -/* read only is the default for safety */ -bdrv_set_read_only (bs, true); s->qcow =NULL ; s->qcow_filename =NULL ; s->fat2 =NULL ;@@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,if (ret <0 ) { goto fail; } -bdrv_set_read_only (bs, false); + ret =bdrv_set_read_only (bs, false, &local_err); +if (ret <0 ) { +error_propagate (errp, local_err); + goto fail; + } + }else { +/* read only is the default for safety */ + ret =bdrv_set_read_only (bs, true, &local_err); +if (ret <0 ) { +error_propagate (errp, local_err); + goto fail; + } } bs->total_sectors = cyls * heads * secs;include/block/block.h +1
-1@@ -426,7 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, int64_t sector_num, int nb_sectors, int *pnum);bool bdrv_is_read_only (BlockDriverState *bs); -void bdrv_set_read_only (BlockDriverState *bs,bool read_only); +int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp);bool bdrv_is_sg (BlockDriverState *bs);bool bdrv_is_inserted (BlockDriverState *bs);int bdrv_media_changed (BlockDriverState *bs);
Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled
In reply to Jeff CodyOn Wed, Apr 05, 2017 at 02:28:44PM -0400, Jeff Cody wrote:
Minor comments but:
Quoted 5 lines
block.c +0
-0
block.c void path_combine(char *dest, int dest_size, · line 199
Quoted 9 lines
block.c +4
-1@@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size, } } -void bdrv_set_read_only (BlockDriverState *bs,bool read_only) +int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) { +/* Do not set read_only if copy_on_read is enabled */ +if (bs->copy_on_read && read_only) { + error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",
Users might be puzzled by "COR". The -drive option is called "copy-on-read" so spelling it out is clearer than using an acronym.
block.c static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, · line 1178
Quoted 10 lines
block.c +5
-1@@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,if (ret <0 ) { goto fail; } -bdrv_set_read_only (bs, false); + ret =bdrv_set_read_only (bs, false, &local_err); +if (ret <0 ) { +error_propagate (errp, local_err); + goto fail; + }
read_only = false by default. There's no need to set it now that you've moved the bdrv_set_read_only(bs, true) call.
Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled
In reply to Jeff Codyblock/vvfat.c static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, · line 1178
Quoted 10 lines · Jeff Cody
block/vvfat.c +5
-1@@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,if (ret <0 ) { goto fail; } -bdrv_set_read_only (bs, false); + ret =bdrv_set_read_only (bs, false, &local_err); +if (ret <0 ) { +error_propagate (errp, local_err); + goto fail; + }
I realized later in the series why you are doing this.
The error code path introduces a resource leak: enable_write_target() has already been called and isn't cleaned up by the fail label.
It would be cleaner to check that bs is writable before calling enable_write_target().
Stefan
PATCH 3/9 block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
In reply to Jeff CodyThe BDRV_O_ALLOW_RDWR flag allows / prohibits the changing of the BDS 'read_only' state, but there are a few places where it is ignored. In the bdrv_set_read_only() helper, make sure to honor the flag.
--- block.c | 7 +++++++ 1 file changed, 7 insertions(+)
block.c +7 -0
@@ -201,6 +201,13 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)return -EINVAL ; } +/* Do not clear read_only if it is prohibited */ +if (!read_only && !(bs->open_flags &BDRV_O_ALLOW_RDWR )) { +error_setg (errp,"Node '%s' is read only" , +bdrv_get_device_or_node_name (bs)); +return -EPERM ; + } + bs->read_only = read_only;return 0 ; }
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
Quoted 19 lines · Jeff Cody
The
BDRV_O_ALLOW_RDWRflag allows / prohibits the changing of the BDS 'read_only' state, but there are a few places where it is ignored. In thebdrv_set_read_only()helper, make sure to honor the flag.Signed-off-by Jeff Cody <[email protected]>--- block.c | 7 +++++++ 1 file changed, 7 insertions(+)block.c +0
-0@@ -201,6 +201,13 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)return -EINVAL ; }
block.c int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp) · line 213
Quoted 10 lines
block.c +7
-0+/* Do not clear read_only if it is prohibited */ +if (!read_only && !(bs->open_flags &BDRV_O_ALLOW_RDWR )) { +error_setg (errp,"Node '%s' is read only" , +bdrv_get_device_or_node_name (bs)); +return -EPERM ; + } + bs->read_only = read_only;return 0 ; }
Conceptually straightforward.
looks like this might change behavior for... RBD and vvfat, right? RBD is the subject of this series so we'll just assume that was broken and stupid.
What's vvfat's story? It always set the read-only property to false regardless of what you asked for?
Re: [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
Quoted 8 lines · John Snow
Conceptually straightforward.
looks like this might change behavior for... RBD and vvfat, right? RBD is the subject of this series so we'll just assume that was broken and stupid.
What's vvfat's story? It always set the read-only property to false regardless of what you asked for?
vvfat is even stupider than that - it has its own independent property 'rw' that determines whether to allow write operations, separate from the inherited BDS readonly property.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
Quoted 8 lines · Eric Blake
Quoted 6 lines · John Snow
Conceptually straightforward.
looks like this might change behavior for... RBD and vvfat, right? RBD is the subject of this series so we'll just assume that was broken and stupid.
Yes on RBD, and that change is intentional.
Quoted 7 lines
Quoted 2 lines
What's vvfat's story? It always set the read-only property to false regardless of what you asked for?
vvfat is even stupider than that - it has its own independent property 'rw' that determines whether to allow write operations, separate from the inherited BDS readonly property.
Yes, it is very odd. But if we have copy_on_read enabled, or explicitly set the block device to read-only via QAPI or -drive, I think that those should take precedence.
Re: [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
Quoted 15 lines · Jeff Cody
Quoted 8 lines · Eric Blake
Quoted 6 lines · John Snow
Conceptually straightforward.
looks like this might change behavior for... RBD and vvfat, right? RBD is the subject of this series so we'll just assume that was broken and stupid.
Yes on RBD, and that change is intentional.
Quoted 3 lines
Quoted 2 lines
What's vvfat's story? It always set the read-only property to false regardless of what you asked for?
Quoted 9 lines
Quoted 4 lines
vvfat is even stupider than that - it has its own independent property 'rw' that determines whether to allow write operations, separate from the inherited BDS readonly property.
Yes, it is very odd. But if we have
copy_on_readenabled, or explicitly set the block device to read-only via QAPI or -drive, I think that those should take precedence.
I meant to add an example - currently, with this drive commandline:
"-drive format=vvfat,dir=/tmp/vvfat,rw,if=virtio,readonly=on"
The drive is not read-only, but writable. That seems broken.
After this patch, this ends up throwing an error now (which I think is a logical thing to do):
qemu-system-x86_64: -drive format=vvfat,dir=/tmp/vvfat,rw,if=virtio,readonly=on: Node '#block238' is read only
How this affects vvfat (and rbd) should be documented in the commit message, however, so I should ammend that if we keep this behavior.
One other possible option is to treat the vvfat 'rw' option as meaning "enable writes, iff the block device is writable". With that interpretation, we could do something different in the above scenario:
silently fail to set bs->read_only to 'true' in the vvfat driver, and keep it r/o.
PATCH 4/9 block: code movement
In reply to Jeff CodyMove bdrv_is_read_only() up with its friends.
--- block.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
block.c +5 -5
@@ -192,6 +192,11 @@ void path_combine(char *dest, int dest_size, } } +bool bdrv_is_read_only (BlockDriverState *bs) +{ +return bs->read_only; +} +int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) {/* Do not set read_only if copy_on_read is enabled */
@@ -3367,11 +3372,6 @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr) *nb_sectors_ptr = nb_sectors <0 ?0 : nb_sectors; } -bool bdrv_is_read_only (BlockDriverState *bs) -{ -return bs->read_only; -} -bool bdrv_is_sg (BlockDriverState *bs) {return bs->sg;
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH for-2.10 4/9] block: code movement
Quoted 26 lines · Jeff Cody
Move
bdrv_is_read_only()up with its friends.Signed-off-by Jeff Cody <[email protected]>--- block.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)block.c +5
-0@@ -192,6 +192,11 @@ void path_combine(char *dest, int dest_size, } } +bool bdrv_is_read_only (BlockDriverState *bs) +{ +return bs->read_only; +} +int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) {/* Do not set read_only if copy_on_read is enabled */ @@ -3367,11 +3372,6 @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr) *nb_sectors_ptr = nb_sectors <0 ?0 : nb_sectors;
block.c void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr) · line 3377
Quoted 10 lines
block.c +0
-5} -bool bdrv_is_read_only (BlockDriverState *bs) -{ -return bs->read_only; -} -bool bdrv_is_sg (BlockDriverState *bs) {return bs->sg;
Re: [Qemu-devel] [PATCH for-2.10 4/9] block: code movement
In reply to Jeff CodyQuoted 6 lines · Jeff Cody
Move
bdrv_is_read_only()up with its friends.Signed-off-by Jeff Cody <[email protected]>--- block.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
PATCH 5/9 block: introduce bdrv_try_set_read_only()
In reply to Jeff CodyIntroduce try function for setting read_only flags. Will return < 0 on error, with appropriate Error value set. Does not alter any flags.
--- block.c | 14 +++++++++++++- include/block/block.h | 1 + 2 files changed, 14 insertions(+), 1 deletion(-)
block.c +13 -1
@@ -197,7 +197,7 @@ bool bdrv_is_read_only(BlockDriverState *bs)return bs->read_only; } -int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) +int bdrv_try_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) {/* Do not set read_only if copy_on_read is enabled */ if (bs->copy_on_read && read_only) {
@@ -213,6 +213,18 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)return -EPERM ; } +return 0 ; +} + +int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) +{ +int ret =0 ; + + ret =bdrv_try_set_read_only (bs, read_only, errp); +if (ret <0 ) { +return ret; + } + bs->read_only = read_only;return 0 ; }
include/block/block.h +1 -0
@@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, int64_t sector_num, int nb_sectors, int *pnum);bool bdrv_is_read_only (BlockDriverState *bs); +int bdrv_try_set_read_only (BlockDriverState *bs,bool read_only,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);
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only()
Quoted 47 lines · Jeff Cody
Introduce try function for setting
read_onlyflags. Will return < 0 on error, with appropriate Error value set. Does not alter any flags.Signed-off-by Jeff Cody <[email protected]>--- block.c | 14 +++++++++++++- include/block/block.h | 1 + 2 files changed, 14 insertions(+), 1 deletion(-)block.c +13
-1@@ -197,7 +197,7 @@ bool bdrv_is_read_only(BlockDriverState *bs)return bs->read_only; } -int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) +int bdrv_try_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) {/* Do not set read_only if copy_on_read is enabled */ if (bs->copy_on_read && read_only) {@@ -213,6 +213,18 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)return -EPERM ; } +return 0 ; +} + +int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) +{ +int ret =0 ; + + ret =bdrv_try_set_read_only (bs, read_only, errp); +if (ret <0 ) { +return ret; + } + bs->read_only = read_only;return 0 ; }include/block/block.h +0
-0
include/block/block.h int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, · line 432
Quoted 7 lines
include/block/block.h +1
-0@@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, int64_t sector_num, int nb_sectors, int *pnum);bool bdrv_is_read_only (BlockDriverState *bs); +int bdrv_try_set_read_only (BlockDriverState *bs,bool read_only,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);
Maybe I'm just green, but I find it weird that the function is named try_x when we aren't actually trying to do anything, we're checking to see if we can.
Re: [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only()
Quoted 51 lines · John Snow
Quoted 48 lines · Jeff Cody
Introduce try function for setting
read_onlyflags. Will return < 0 on error, with appropriate Error value set. Does not alter any flags.Signed-off-by Jeff Cody <[email protected]>--- block.c | 14 +++++++++++++- include/block/block.h | 1 + 2 files changed, 14 insertions(+), 1 deletion(-)block.c +13
-1@@ -197,7 +197,7 @@ bool bdrv_is_read_only(BlockDriverState *bs)return bs->read_only; } -int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) +int bdrv_try_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) {/* Do not set read_only if copy_on_read is enabled */ if (bs->copy_on_read && read_only) {@@ -213,6 +213,18 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)return -EPERM ; } +return 0 ; +} + +int bdrv_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) +{ +int ret =0 ; + + ret =bdrv_try_set_read_only (bs, read_only, errp); +if (ret <0 ) { +return ret; + } + bs->read_only = read_only;return 0 ; }include/block/block.h +0
-0@@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, int64_t sector_num, int nb_sectors, int *pnum);
Quoted 10 lines
include/block/block.h int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, · line 432
Quoted 6 lines
include/block/block.h +1
-0bool bdrv_is_read_only (BlockDriverState *bs); +int bdrv_try_set_read_only (BlockDriverState *bs,bool read_only,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);Maybe I'm just green, but I find it weird that the function is named
try_xwhen we aren't actually trying to do anything, we're checking to see if we can.
Yeah, not a good name. Maybe bdrv_set_read_only_test() or bdrv_test_set_read_only()?
Re: [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only()
Quoted 20 lines · Jeff Cody
Quoted 8 lines · John Snow
Quoted 7 lines · Jeff Cody
include/block/block.h +0
-0@@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, int64_t sector_num, int nb_sectors, int *pnum);Quoted 10 lines
include/block/block.h int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, · line 432
Quoted 6 lines
include/block/block.h +1
-0bool bdrv_is_read_only (BlockDriverState *bs); +int bdrv_try_set_read_only (BlockDriverState *bs,bool read_only,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);Maybe I'm just green, but I find it weird that the function is named
try_xwhen we aren't actually trying to do anything, we're checking to see if we can.
Quoted 2 lines
Yeah, not a good name. Maybe
bdrv_set_read_only_test()orbdrv_test_set_read_only()?
This one reads nicely:
if (bdrv_can_set_read_only(...)) {
...
}PATCH 6/9 block: use bdrv_try_set_read_only() during reopen
In reply to Jeff Cody--- block.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
block.c +8 -6
@@ -2785,6 +2785,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,BlockDriver *drv;QemuOpts *opts;const char *value; +bool read_only;assert (reopen_state !=NULL );assert (reopen_state->bs->drv !=NULL );
@@ -2813,12 +2814,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,qdict_put (reopen_state->options,"driver" ,qstring_from_str (value)); } -/* if we are to stay read-only, do not allow permission change -* to r/w */ -if (!(reopen_state->bs->open_flags &BDRV_O_ALLOW_RDWR ) && - reopen_state->flags &BDRV_O_RDWR ) { -error_setg (errp,"Node '%s' is read only" , -bdrv_get_device_or_node_name (reopen_state->bs)); +/* If we are to stay read-only, do not allow permission change +* 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->bs->open_flags &BDRV_O_RDWR ); + ret =bdrv_try_set_read_only (reopen_state->bs, read_only, &local_err); +if (local_err) { +error_propagate (errp, local_err); goto error; }
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen
Quoted 18 lines · Jeff Cody
Signed-off-by Jeff Cody <[email protected]>--- block.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)block.c +1
-0@@ -2785,6 +2785,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,BlockDriver *drv;QemuOpts *opts;const char *value; +bool read_only;assert (reopen_state !=NULL );assert (reopen_state->bs->drv !=NULL );
block.c int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, · line 2819
Quoted 7 lines
block.c +0
-4@@ -2813,12 +2814,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,qdict_put (reopen_state->options,"driver" ,qstring_from_str (value)); } -/* if we are to stay read-only, do not allow permission change -* to r/w */ -if (!(reopen_state->bs->open_flags &BDRV_O_ALLOW_RDWR ) && - reopen_state->flags &BDRV_O_RDWR ) {
So the current code checks reopen_state->flags & BDRV_O_RDWR;
block.c int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, · line 2820
Quoted 6 lines
block.c +4
-2-error_setg (errp,"Node '%s' is read only" , -bdrv_get_device_or_node_name (reopen_state->bs)); +/* If we are to stay read-only, do not allow permission change +* 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->bs->open_flags &BDRV_O_RDWR );
And the proposed change checks reopen_state->bs->open_flags & BDRV_O_RDWR. (It's negated again inside of bdrv_try_set_read_only.)
Both check against !(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR).
What's the functional difference of doing so, and is it intentional?
Quoted 7 lines
block.c +3
-0+ ret =bdrv_try_set_read_only (reopen_state->bs, read_only, &local_err); +if (local_err) { +error_propagate (errp, local_err); goto error; }
Re: [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen
Quoted 38 lines · John Snow
Quoted 18 lines · Jeff Cody
Signed-off-by Jeff Cody <[email protected]>--- block.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)block.c +1
-0@@ -2785,6 +2785,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,BlockDriver *drv;QemuOpts *opts;const char *value; +bool read_only;assert (reopen_state !=NULL );assert (reopen_state->bs->drv !=NULL );block.c int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, · line 2819
Quoted 7 lines
block.c +0
-4@@ -2813,12 +2814,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,qdict_put (reopen_state->options,"driver" ,qstring_from_str (value)); } -/* if we are to stay read-only, do not allow permission change -* to r/w */ -if (!(reopen_state->bs->open_flags &BDRV_O_ALLOW_RDWR ) && - reopen_state->flags &BDRV_O_RDWR ) {So the current code checks
reopen_state->flags&BDRV_O_RDWR;block.c int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, · line 2820
Quoted 6 lines
block.c +4
-2-error_setg (errp,"Node '%s' is read only" , -bdrv_get_device_or_node_name (reopen_state->bs)); +/* If we are to stay read-only, do not allow permission change +* 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->bs->open_flags &BDRV_O_RDWR );
Quoted 7 lines
And the proposed change checks
reopen_state->bs->open_flags&BDRV_O_RDWR. (It's negated again inside ofbdrv_try_set_read_only.)Both check against !(reopen_state->bs->open_flags &
BDRV_O_ALLOW_RDWR).What's the functional difference of doing so, and is it intentional?
Good catch; unintentional. That line should read:
+ read_only = !(reopen_state->flags & BDRV_O_RDWR);
The functional difference is we would be testing against the open_flags of the current BS to see if we are requesting r/w or r/o, rather than checking against the reopen requested flags.
Quoted 7 lines
Quoted 7 lines
block.c +3
-0+ ret =bdrv_try_set_read_only (reopen_state->bs, read_only, &local_err); +if (local_err) { +error_propagate (errp, local_err); goto error; }
PATCH 7/9 block/rbd - update variable names to more apt names
In reply to Jeff CodyUpdate 'clientname' to be 'user', which tracks better with both the QAPI and rados viarable naming.
Update 'name' to be 'image_name', as it indicates the rbd image. Naming it 'image' would have been ideal, but we are using that for the rados_image_t value returned by rbd_open().
--- block/rbd.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)
block/rbd.c +17 -16
@@ -94,7 +94,7 @@ typedef struct BDRVRBDState {rados_t cluster;rados_ioctx_t io_ctx;rbd_image_t image; -char *name; +char *image_name;char *snap; } BDRVRBDState;
@@ -350,7 +350,7 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)int64_t bytes =0 ;int64_t objsize;int obj_order =0 ; -const char *pool, *name, *conf, *clientname, *keypairs; +const char *pool, *image_name, *conf, *user, *keypairs;const char *secretid;rados_t cluster;rados_ioctx_t io_ctx;
@@ -393,11 +393,11 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)*/ pool =qdict_get_try_str (options,"pool" ); conf =qdict_get_try_str (options,"conf" ); - clientname =qdict_get_try_str (options,"user" ); - name =qdict_get_try_str (options,"image" ); + user =qdict_get_try_str (options,"user" ); + image_name =qdict_get_try_str (options,"image" ); keypairs =qdict_get_try_str (options,"=keyvalue-pairs" ); - ret =rados_create (&cluster, clientname); + ret =rados_create (&cluster, user);if (ret <0 ) {error_setg_errno (errp, -ret,"error initializing" ); goto exit;
@@ -434,7 +434,7 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp) goto shutdown; } - ret =rbd_create (io_ctx, name, bytes, &obj_order); + ret =rbd_create (io_ctx, image_name, bytes, &obj_order);if (ret <0 ) {error_setg_errno (errp, -ret,"error rbd create" ); }
@@ -540,7 +540,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) {BDRVRBDState *s = bs->opaque; -const char *pool, *snap, *conf, *clientname, *name, *keypairs; +const char *pool, *snap, *conf, *user, *image_name, *keypairs;const char *secretid;QemuOpts *opts;Error *local_err =NULL ;
@@ -567,24 +567,24 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, pool =qemu_opt_get (opts,"pool" ); conf =qemu_opt_get (opts,"conf" ); snap =qemu_opt_get (opts,"snapshot" ); - clientname =qemu_opt_get (opts,"user" ); - name =qemu_opt_get (opts,"image" ); + user =qemu_opt_get (opts,"user" ); + image_name =qemu_opt_get (opts,"image" ); keypairs =qemu_opt_get (opts,"=keyvalue-pairs" ); -if (!pool || !name) { +if (!pool || !image_name) {error_setg (errp,"Parameters 'pool' and 'image' are required" ); r = -EINVAL ; goto failed_opts; } - r =rados_create (&s->cluster, clientname); + r =rados_create (&s->cluster, user);if (r <0 ) {error_setg_errno (errp, -r,"error initializing" ); goto failed_opts; } s->snap =g_strdup (snap); - s->name =g_strdup (name); + s->image_name =g_strdup (image_name);/* try default location when conf=NULL, but ignore failure */ r =rados_conf_read_file (s->cluster, conf);
@@ -635,9 +635,10 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, goto failed_shutdown; } - r =rbd_open (s->io_ctx, s->name, &s->image, s->snap); + r =rbd_open (s->io_ctx, s->image_name, &s->image, s->snap);if (r <0 ) { -error_setg_errno (errp, -r,"error reading header from %s" , s->name); +error_setg_errno (errp, -r,"error reading header from %s" , + s->image_name); goto failed_open; }
@@ -655,7 +656,7 @@ failed_open: failed_shutdown:rados_shutdown (s->cluster);g_free (s->snap); -g_free (s->name); +g_free (s->image_name); failed_opts:qemu_opts_del (opts);g_free (mon_host);
@@ -669,7 +670,7 @@ static void qemu_rbd_close(BlockDriverState *bs)rbd_close (s->image);rados_ioctx_destroy (s->io_ctx);g_free (s->snap); -g_free (s->name); +g_free (s->image_name);rados_shutdown (s->cluster); }
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH for-2.10 7/9] block/rbd - update variable names to more apt names
Quoted 11 lines · Jeff Cody
Update 'clientname' to be 'user', which tracks better with both the QAPI and rados viarable naming.
Update 'name' to be '
image_name', as it indicates the rbd image. Naming it 'image' would have been ideal, but we are using that for therados_image_tvalue returned byrbd_open().Signed-off-by Jeff Cody <[email protected]>--- block/rbd.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)
PATCH 8/9 block/rbd: do not blindly set bs->read_only
In reply to Jeff CodyWe do not want to just blindly set bs->read_only. The only time we need to set it with the current rbd driver, is if we are using an rbd snapshot. In that case, we must set it to read-only.
--- block/rbd.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
block/rbd.c +9 -4
@@ -635,6 +635,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, goto failed_shutdown; } +/* rbd_open is always r/w */ r =rbd_open (s->io_ctx, s->image_name, &s->image, s->snap);if (r <0 ) { error_setg_errno(errp, -r, "error reading header from %s",
@@ -642,10 +643,14 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, goto failed_open; } - r =bdrv_set_read_only (bs, (s->snap !=NULL ), &local_err); -if (r <0 ) { -error_propagate (errp, local_err); - goto failed_open; +/* If we are using an rbd snapshot, we must be r/o, otherwise +* leave as-is */ +if (s->snap !=NULL ) { + r =bdrv_set_read_only (bs, true, &local_err); +if (r <0 ) { +error_propagate (errp, local_err); + goto failed_open; + } }qemu_opts_del (opts);
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH for-2.10 8/9] block/rbd: do not blindly set bs->read_only
Quoted 27 lines · Jeff Cody
We do not want to just blindly set
bs->read_only. The only time we need to set it with the current rbd driver, is if we are using an rbd snapshot. In that case, we must set it to read-only.Signed-off-by Jeff Cody <[email protected]>--- block/rbd.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)block/rbd.c +1
-2@@ -635,6 +635,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, goto failed_shutdown; } +/* rbd_open is always r/w */ r =rbd_open (s->io_ctx, s->image_name, &s->image, s->snap);if (r <0 ) { error_setg_errno(errp, -r, "error reading header from %s",@@ -642,10 +643,14 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, goto failed_open; } - r =bdrv_set_read_only (bs, (s->snap !=NULL ), &local_err); -if (r <0 ) {
block/rbd.c static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, · line 653
Quoted 10 lines
block/rbd.c +8
-2-error_propagate (errp, local_err); - goto failed_open; +/* If we are using an rbd snapshot, we must be r/o, otherwise +* leave as-is */ +if (s->snap !=NULL ) { + r =bdrv_set_read_only (bs, true, &local_err); +if (r <0 ) { +error_propagate (errp, local_err); + goto failed_open; + }
This patch series breaks git-bisect(1) for -drive file.driver=rbd,file.read-only=on since bdrv_set_read_only(bs, false, &local_err) fails.
Please squash this patch where you added the bdrv_set_read_only() errp argument.
PATCH 9/9 block/rbd: Add support for reopen()
In reply to Jeff CodyThis adds support for reopen in rbd, for changing between r/w and r/o.
Note, that this is only a flag change, but we will block a change from r/o to r/w if we are using an RBD internal snapshot.
--- block/rbd.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
block/rbd.c +21 -0
@@ -668,6 +668,26 @@ failed_opts:return r; } + +/* Since RBD is currently always opened R/W via the API, +* we just need to check if we are using a snapshot or not, in +* order to determine if we will allow it to be R/W */ +static int qemu_rbd_reopen_prepare (BDRVReopenState *state, +BlockReopenQueue *queue,Error **errp) +{ +BDRVRBDState *s = state->bs->opaque; +int ret =0 ; + +if (s->snap && state->flags &BDRV_O_RDWR ) { +error_setg (errp, +"Cannot change node '%s' to r/w when using RBD snapshot" , +bdrv_get_device_or_node_name (state->bs)); + ret = -EINVAL ; + } + +return ret; +} +static void qemu_rbd_close (BlockDriverState *bs) {BDRVRBDState *s = bs->opaque;
@@ -1074,6 +1094,7 @@ static BlockDriver bdrv_rbd = { .bdrv_parse_filename = qemu_rbd_parse_filename, .bdrv_file_open = qemu_rbd_open, .bdrv_close = qemu_rbd_close, + .bdrv_reopen_prepare = qemu_rbd_reopen_prepare, .bdrv_create = qemu_rbd_create, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_get_info = qemu_rbd_getinfo,
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH for-2.10 9/9] block/rbd: Add support for reopen()
Quoted 9 lines · Jeff Cody
This adds support for reopen in rbd, for changing between r/w and r/o.
Note, that this is only a flag change, but we will block a change from r/o to r/w if we are using an RBD internal snapshot.
Signed-off-by Jeff Cody <[email protected]>--- block/rbd.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)