| Message ID | 1441183880-26993-5-git-send-email-wency@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 09/02/2015 02:51 AM, Wen Congyang wrote: > Usage: > -drive file=xxx,id=Y, \ > -drive file=xxxx,id=X,backing.backing_reference=Y > > It will create such backing chain: > {virtio-blk dev 'Y'} {virtio-blk dev 'X'} > | | > | | > v v > > [base] <- [mid] <- ( Y ) <----------------- ( X ) This makes any changes to 'Y' have unspecified effects on 'X'. While we may have a valid reason to use a backing BDS in more than one chain, I seriously doubt anyone will ever want to have two guest-visible -drive's that are both read-write where one can corrupt the other. I can totally see the point of having BDS 'Y' exist for checkpoints or some other non-guest-visible action, so I'm not saying this patch is wrong, just that the commit message is picking a poor example of how it would be used. > > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > block.c | 39 +++++++++++++++++++++++++++++++++++---- > include/block/block.h | 1 + > 2 files changed, 36 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index d02d9e9..f93c50d 100644 > --- a/block.c > +++ b/block.c > @@ -1179,6 +1179,7 @@ out: > } > > #define ALLOW_WRITE_BACKING_FILE "allow-write-backing-file" > +#define BACKING_REFERENCE "backing_reference" Why the inconsistency in '-' vs. '_'? I'd stick with dash here. > static QemuOptsList backing_file_opts = { > .name = "backing_file", > .head = QTAILQ_HEAD_INITIALIZER(backing_file_opts.head), > @@ -1188,6 +1189,11 @@ static QemuOptsList backing_file_opts = { > .type = QEMU_OPT_BOOL, > .help = "allow write to backing file", > }, > + { > + .name = BACKING_REFERENCE, > + .type = QEMU_OPT_STRING, > + .help = "reference to the exsiting BDS", s/exsiting/existing/ But why do we need this? In qapi, BlockdevOptionsGenericCOWFormat already has '*backing':'BlockdevRef', and BlockdevRef already has a choice between 'definition' (object) and 'reference' (string). Or is this just a matter of teaching the command line to do what QMP can already do? In which case, wouldn't: -drive file=xxx,id=Y, -drive file=xxxx,id=X,backing=Y be the natural mapping of 'backing' being a string rather than a dictionary?
On 09/03/2015 02:50 AM, Eric Blake wrote: > On 09/02/2015 02:51 AM, Wen Congyang wrote: >> Usage: >> -drive file=xxx,id=Y, \ >> -drive file=xxxx,id=X,backing.backing_reference=Y >> >> It will create such backing chain: >> {virtio-blk dev 'Y'} {virtio-blk dev 'X'} >> | | >> | | >> v v >> >> [base] <- [mid] <- ( Y ) <----------------- ( X ) > > This makes any changes to 'Y' have unspecified effects on 'X'. While we > may have a valid reason to use a backing BDS in more than one chain, I > seriously doubt anyone will ever want to have two guest-visible -drive's > that are both read-write where one can corrupt the other. I can totally > see the point of having BDS 'Y' exist for checkpoints or some other > non-guest-visible action, so I'm not saying this patch is wrong, just > that the commit message is picking a poor example of how it would be used. Sorry, this graph is wrong. virtio-blk dev 'Y' can not use the BDS Y if it is referenced by X. That is why I need patch 1 and 2. Thanks Wen Congyang > >> >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> --- >> block.c | 39 +++++++++++++++++++++++++++++++++++---- >> include/block/block.h | 1 + >> 2 files changed, 36 insertions(+), 4 deletions(-) >> >> diff --git a/block.c b/block.c >> index d02d9e9..f93c50d 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1179,6 +1179,7 @@ out: >> } >> >> #define ALLOW_WRITE_BACKING_FILE "allow-write-backing-file" >> +#define BACKING_REFERENCE "backing_reference" > > Why the inconsistency in '-' vs. '_'? I'd stick with dash here. > >> static QemuOptsList backing_file_opts = { >> .name = "backing_file", >> .head = QTAILQ_HEAD_INITIALIZER(backing_file_opts.head), >> @@ -1188,6 +1189,11 @@ static QemuOptsList backing_file_opts = { >> .type = QEMU_OPT_BOOL, >> .help = "allow write to backing file", >> }, >> + { >> + .name = BACKING_REFERENCE, >> + .type = QEMU_OPT_STRING, >> + .help = "reference to the exsiting BDS", > > s/exsiting/existing/ > > But why do we need this? In qapi, BlockdevOptionsGenericCOWFormat > already has '*backing':'BlockdevRef', and BlockdevRef already has a > choice between 'definition' (object) and 'reference' (string). Or is > this just a matter of teaching the command line to do what QMP can > already do? In which case, wouldn't: > > -drive file=xxx,id=Y, -drive file=xxxx,id=X,backing=Y > > be the natural mapping of 'backing' being a string rather than a dictionary? >
diff --git a/block.c b/block.c index d02d9e9..f93c50d 100644 --- a/block.c +++ b/block.c @@ -1179,6 +1179,7 @@ out: } #define ALLOW_WRITE_BACKING_FILE "allow-write-backing-file" +#define BACKING_REFERENCE "backing_reference" static QemuOptsList backing_file_opts = { .name = "backing_file", .head = QTAILQ_HEAD_INITIALIZER(backing_file_opts.head), @@ -1188,6 +1189,11 @@ static QemuOptsList backing_file_opts = { .type = QEMU_OPT_BOOL, .help = "allow write to backing file", }, + { + .name = BACKING_REFERENCE, + .type = QEMU_OPT_STRING, + .help = "reference to the exsiting BDS", + }, { /* end of list */ } }, }; @@ -1204,11 +1210,12 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) { char *backing_filename = g_malloc0(PATH_MAX); int ret = 0; - BlockDriverState *backing_hd; + BlockDriverState *backing_hd = NULL; Error *local_err = NULL; QemuOpts *opts = NULL; bool child_rw = false; const BdrvChildRole *child_role = NULL; + const char *reference = NULL; if (bs->backing_hd != NULL) { QDECREF(options); @@ -1231,9 +1238,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) goto free_exit; } child_rw = qemu_opt_get_bool(opts, ALLOW_WRITE_BACKING_FILE, false); + reference = qemu_opt_get(opts, BACKING_REFERENCE); child_role = child_rw ? &child_backing_rw : &child_backing; - if (qdict_haskey(options, "file.filename")) { + if (qdict_haskey(options, "file.filename") || reference) { backing_filename[0] = '\0'; } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) { QDECREF(options); @@ -1256,7 +1264,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) goto free_exit; } - backing_hd = bdrv_new(); + if (!reference) { + backing_hd = bdrv_new(); + } if (bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) { qdict_put(options, "driver", qstring_from_str(bs->backing_format)); @@ -1265,7 +1275,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) assert(bs->backing_hd == NULL); ret = bdrv_open_inherit(&backing_hd, *backing_filename ? backing_filename : NULL, - NULL, options, 0, bs, child_role, + reference, options, 0, bs, child_role, NULL, &local_err); if (ret < 0) { bdrv_unref(backing_hd); @@ -1276,6 +1286,19 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) error_free(local_err); goto free_exit; } + if (reference) { + if (bdrv_op_is_blocked(backing_hd, BLOCK_OP_TYPE_BACKING_REFERENCE, + errp)) { + ret = -EBUSY; + goto free_reference_exit; + } + if (backing_hd->blk && blk_disable_attach_dev(backing_hd->blk)) { + error_setg(errp, "backing_hd %s is used by the other device model", + reference); + ret = -EBUSY; + goto free_reference_exit; + } + } bdrv_set_backing_hd(bs, backing_hd); @@ -1283,6 +1306,11 @@ free_exit: qemu_opts_del(opts); g_free(backing_filename); return ret; + +free_reference_exit: + bdrv_unref(backing_hd); + bs->open_flags |= BDRV_O_NO_BACKING; + goto free_exit; } /* @@ -1947,6 +1975,9 @@ void bdrv_close(BlockDriverState *bs) if (bs->backing_hd) { BlockDriverState *backing_hd = bs->backing_hd; bdrv_set_backing_hd(bs, NULL); + if (backing_hd->blk) { + blk_enable_attach_dev(backing_hd->blk); + } bdrv_unref(backing_hd); } diff --git a/include/block/block.h b/include/block/block.h index 612dcad..5812716 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -170,6 +170,7 @@ typedef enum BlockOpType { BLOCK_OP_TYPE_RESIZE, BLOCK_OP_TYPE_STREAM, BLOCK_OP_TYPE_REPLACE, + BLOCK_OP_TYPE_BACKING_REFERENCE, BLOCK_OP_TYPE_MAX, } BlockOpType;
Usage: -drive file=xxx,id=Y, \ -drive file=xxxx,id=X,backing.backing_reference=Y It will create such backing chain: {virtio-blk dev 'Y'} {virtio-blk dev 'X'} | | | | v v [base] <- [mid] <- ( Y ) <----------------- ( X ) Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> Signed-off-by: Gonglei <arei.gonglei@huawei.com> --- block.c | 39 +++++++++++++++++++++++++++++++++++---- include/block/block.h | 1 + 2 files changed, 36 insertions(+), 4 deletions(-)