Message ID | 95addb261af316f49517069e1cce4d9c601d274b.1537367701.git.berto@igalia.com |
---|---|
State | New |
Headers | show |
Series | Don't pass flags to bdrv_reopen_queue() | expand |
On 19.09.18 16:47, Alberto Garcia wrote: > This function is used to put the hidden and secondary disks in > read-write mode before launching the backup job, and back in read-only > mode afterwards. > > This patch does the following changes: > > - Use an options QDict with the "read-only" option instead of > passing the changes as flags only. > > - Simplify the code (it was unnecessarily complicated and verbose). > > - Fix a bug due to which the secondary disk was not being put back > in read-only mode when writable=false (because in this case > orig_secondary_flags always had the BDRV_O_RDWR flag set). > > - Stop clearing the BDRV_O_INACTIVE flag. Why? Sorry, but I don't know much about replication. Do you know this change is OK? (Since the code deliberately clears it right now.) (Well, it makes sense not to re-set it afterwards...) > The flags parameter to bdrv_reopen_queue() becomes redundant and we'll > be able to get rid of it in a subsequent patch. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/replication.c | 42 ++++++++++++++++++------------------------ > 1 file changed, 18 insertions(+), 24 deletions(-) > > diff --git a/block/replication.c b/block/replication.c > index 6349d6958e..1ad0ef2ef8 100644 > --- a/block/replication.c > +++ b/block/replication.c > @@ -20,6 +20,7 @@ > #include "block/block_backup.h" > #include "sysemu/block-backend.h" > #include "qapi/error.h" > +#include "qapi/qmp/qdict.h" > #include "replication.h" > > typedef enum { > @@ -39,8 +40,8 @@ typedef struct BDRVReplicationState { > char *top_id; > ReplicationState *rs; > Error *blocker; > - int orig_hidden_flags; > - int orig_secondary_flags; > + bool orig_hidden_read_only; > + bool orig_secondary_read_only; > int error; > } BDRVReplicationState; > > @@ -376,39 +377,32 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable, > { > BDRVReplicationState *s = bs->opaque; > BlockReopenQueue *reopen_queue = NULL; > - int orig_hidden_flags, orig_secondary_flags; > - int new_hidden_flags, new_secondary_flags; > Error *local_err = NULL; > > if (writable) { I'd like to request a comment that "writable" is true the first time this function is called, and false the second time. That'd make it clear why this rewrite makes sense. (And that writable=false means reverting to the original state.) ((And no, it doesn't make more sense before this patch.)) > - orig_hidden_flags = s->orig_hidden_flags = > - bdrv_get_flags(s->hidden_disk->bs); > - new_hidden_flags = (orig_hidden_flags | BDRV_O_RDWR) & > - ~BDRV_O_INACTIVE; > - orig_secondary_flags = s->orig_secondary_flags = > - bdrv_get_flags(s->secondary_disk->bs); > - new_secondary_flags = (orig_secondary_flags | BDRV_O_RDWR) & > - ~BDRV_O_INACTIVE; > - } else { > - orig_hidden_flags = (s->orig_hidden_flags | BDRV_O_RDWR) & > - ~BDRV_O_INACTIVE; > - new_hidden_flags = s->orig_hidden_flags; > - orig_secondary_flags = (s->orig_secondary_flags | BDRV_O_RDWR) & > - ~BDRV_O_INACTIVE; > - new_secondary_flags = s->orig_secondary_flags; > + s->orig_hidden_read_only = > + !(bdrv_get_flags(s->hidden_disk->bs) & BDRV_O_RDWR); > + s->orig_secondary_read_only = > + !(bdrv_get_flags(s->secondary_disk->bs) & BDRV_O_RDWR); Why not use bdrv_is_read_only()? Max > } > > bdrv_subtree_drained_begin(s->hidden_disk->bs); > bdrv_subtree_drained_begin(s->secondary_disk->bs); > > - if (orig_hidden_flags != new_hidden_flags) { > - reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs, NULL, > - new_hidden_flags); > + if (s->orig_hidden_read_only) { > + int flags = bdrv_get_flags(s->hidden_disk->bs); > + QDict *opts = qdict_new(); > + qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable); > + reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs, > + opts, flags); > } > > - if (!(orig_secondary_flags & BDRV_O_RDWR)) { > + if (s->orig_secondary_read_only) { > + int flags = bdrv_get_flags(s->secondary_disk->bs); > + QDict *opts = qdict_new(); > + qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable); > reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs, > - NULL, new_secondary_flags); > + opts, flags); > } > > if (reopen_queue) { >
On Mon 08 Oct 2018 04:34:24 AM CEST, Max Reitz wrote: >> This patch does the following changes: >> >> - Use an options QDict with the "read-only" option instead of >> passing the changes as flags only. >> >> - Simplify the code (it was unnecessarily complicated and verbose). >> >> - Fix a bug due to which the secondary disk was not being put back >> in read-only mode when writable=false (because in this case >> orig_secondary_flags always had the BDRV_O_RDWR flag set). >> >> - Stop clearing the BDRV_O_INACTIVE flag. > > Why? Sorry, but I don't know much about replication. Do you know > this change is OK? (Since the code deliberately clears it right now.) I don't think the current code is ok, the BDRV_O_INACTIVE flag should be cleared with bdrv_co_invalidate_cache() so I'm inclined to think that it's a bug. >> @@ -376,39 +377,32 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable, >> { >> BDRVReplicationState *s = bs->opaque; >> BlockReopenQueue *reopen_queue = NULL; >> - int orig_hidden_flags, orig_secondary_flags; >> - int new_hidden_flags, new_secondary_flags; >> Error *local_err = NULL; >> >> if (writable) { > > I'd like to request a comment that "writable" is true the first time > this function is called, and false the second time. That'd make it > clear why this rewrite makes sense. Yes, I think it's probably a good idea to do that. >> + s->orig_hidden_read_only = >> + !(bdrv_get_flags(s->hidden_disk->bs) & BDRV_O_RDWR); >> + s->orig_secondary_read_only = >> + !(bdrv_get_flags(s->secondary_disk->bs) & BDRV_O_RDWR); > > Why not use bdrv_is_read_only()? No reason, I overlooked this. I'll fix it. Berto
diff --git a/block/replication.c b/block/replication.c index 6349d6958e..1ad0ef2ef8 100644 --- a/block/replication.c +++ b/block/replication.c @@ -20,6 +20,7 @@ #include "block/block_backup.h" #include "sysemu/block-backend.h" #include "qapi/error.h" +#include "qapi/qmp/qdict.h" #include "replication.h" typedef enum { @@ -39,8 +40,8 @@ typedef struct BDRVReplicationState { char *top_id; ReplicationState *rs; Error *blocker; - int orig_hidden_flags; - int orig_secondary_flags; + bool orig_hidden_read_only; + bool orig_secondary_read_only; int error; } BDRVReplicationState; @@ -376,39 +377,32 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable, { BDRVReplicationState *s = bs->opaque; BlockReopenQueue *reopen_queue = NULL; - int orig_hidden_flags, orig_secondary_flags; - int new_hidden_flags, new_secondary_flags; Error *local_err = NULL; if (writable) { - orig_hidden_flags = s->orig_hidden_flags = - bdrv_get_flags(s->hidden_disk->bs); - new_hidden_flags = (orig_hidden_flags | BDRV_O_RDWR) & - ~BDRV_O_INACTIVE; - orig_secondary_flags = s->orig_secondary_flags = - bdrv_get_flags(s->secondary_disk->bs); - new_secondary_flags = (orig_secondary_flags | BDRV_O_RDWR) & - ~BDRV_O_INACTIVE; - } else { - orig_hidden_flags = (s->orig_hidden_flags | BDRV_O_RDWR) & - ~BDRV_O_INACTIVE; - new_hidden_flags = s->orig_hidden_flags; - orig_secondary_flags = (s->orig_secondary_flags | BDRV_O_RDWR) & - ~BDRV_O_INACTIVE; - new_secondary_flags = s->orig_secondary_flags; + s->orig_hidden_read_only = + !(bdrv_get_flags(s->hidden_disk->bs) & BDRV_O_RDWR); + s->orig_secondary_read_only = + !(bdrv_get_flags(s->secondary_disk->bs) & BDRV_O_RDWR); } bdrv_subtree_drained_begin(s->hidden_disk->bs); bdrv_subtree_drained_begin(s->secondary_disk->bs); - if (orig_hidden_flags != new_hidden_flags) { - reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs, NULL, - new_hidden_flags); + if (s->orig_hidden_read_only) { + int flags = bdrv_get_flags(s->hidden_disk->bs); + QDict *opts = qdict_new(); + qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable); + reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs, + opts, flags); } - if (!(orig_secondary_flags & BDRV_O_RDWR)) { + if (s->orig_secondary_read_only) { + int flags = bdrv_get_flags(s->secondary_disk->bs); + QDict *opts = qdict_new(); + qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable); reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs, - NULL, new_secondary_flags); + opts, flags); } if (reopen_queue) {
This function is used to put the hidden and secondary disks in read-write mode before launching the backup job, and back in read-only mode afterwards. This patch does the following changes: - Use an options QDict with the "read-only" option instead of passing the changes as flags only. - Simplify the code (it was unnecessarily complicated and verbose). - Fix a bug due to which the secondary disk was not being put back in read-only mode when writable=false (because in this case orig_secondary_flags always had the BDRV_O_RDWR flag set). - Stop clearing the BDRV_O_INACTIVE flag. The flags parameter to bdrv_reopen_queue() becomes redundant and we'll be able to get rid of it in a subsequent patch. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/replication.c | 42 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 24 deletions(-)