| Message ID | 1441183880-26993-4-git-send-email-wency@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 09/02/2015 02:51 AM, Wen Congyang wrote: > 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> Not much description in the commit message. I really want an explanation of why this patch is necessary. After all, with 'block-commit', we were able to turn on read-write mode of backing files on an as-needed basis, without having to expose that to the end user. Giving the end user a knob that they must tune feels a bit awkward, and probably means we don't have the design right. > --- > block.c | 41 ++++++++++++++++++++++++++++++++++++++++- > qapi/block-core.json | 7 ++++++- > 2 files changed, 46 insertions(+), 2 deletions(-) > > +#define ALLOW_WRITE_BACKING_FILE "allow-write-backing-file" > +static QemuOptsList backing_file_opts = { > + .name = "backing_file", > + .head = QTAILQ_HEAD_INITIALIZER(backing_file_opts.head), > + .desc = { > + { > + .name = ALLOW_WRITE_BACKING_FILE, > + .type = QEMU_OPT_BOOL, > + .help = "allow write to backing file", If you do add more justification for why this patch is necessary, then, s/write/writes/ > +++ b/qapi/block-core.json > @@ -1408,6 +1408,10 @@ > # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1) > # (default: off) > # > +# @allow-write-backing-file: #optional whether the backing file is opened in > +# read-write mode. It is only for backing file > +# (Since 2.5 default: false) > +# The name feels a bit long. It sounds like it is an error to pass allow-write-backing-file for a top-level BDS (that is, the BDS associated with a BB). Meanwhile, the default for any backing chain BDS is to open it read-only, regardless of the 'read-only' setting of the parent. But can we just allow 'read-only':false on a backing BDS to mean that the BDS starts life as read-write, without having to add a new parameter?
On 09/03/2015 12:06 AM, Eric Blake wrote: > On 09/02/2015 02:51 AM, Wen Congyang wrote: >> 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> > > Not much description in the commit message. I really want an > explanation of why this patch is necessary. After all, with For COLO, we have such backing chain: secondary disk <-- hidden disk <-- active disk secondary disk is top BDS(use bacing reference), so it can be opened in read-write mode. But hidden disk is read only, and we need to write to hidden disk(backup job will write data to it). > 'block-commit', we were able to turn on read-write mode of backing files > on an as-needed basis, without having to expose that to the end user. > Giving the end user a knob that they must tune feels a bit awkward, and > probably means we don't have the design right. > >> --- >> block.c | 41 ++++++++++++++++++++++++++++++++++++++++- >> qapi/block-core.json | 7 ++++++- >> 2 files changed, 46 insertions(+), 2 deletions(-) >> > >> +#define ALLOW_WRITE_BACKING_FILE "allow-write-backing-file" >> +static QemuOptsList backing_file_opts = { >> + .name = "backing_file", >> + .head = QTAILQ_HEAD_INITIALIZER(backing_file_opts.head), >> + .desc = { >> + { >> + .name = ALLOW_WRITE_BACKING_FILE, >> + .type = QEMU_OPT_BOOL, >> + .help = "allow write to backing file", > > If you do add more justification for why this patch is necessary, then, > > s/write/writes/ > >> +++ b/qapi/block-core.json >> @@ -1408,6 +1408,10 @@ >> # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1) >> # (default: off) >> # >> +# @allow-write-backing-file: #optional whether the backing file is opened in >> +# read-write mode. It is only for backing file >> +# (Since 2.5 default: false) >> +# > > The name feels a bit long. > > It sounds like it is an error to pass allow-write-backing-file for a > top-level BDS (that is, the BDS associated with a BB). Meanwhile, the > default for any backing chain BDS is to open it read-only, regardless of > the 'read-only' setting of the parent. But can we just allow > 'read-only':false on a backing BDS to mean that the BDS starts life as > read-write, without having to add a new parameter? > We have discussed it before: http://lists.nongnu.org/archive/html/qemu-devel/2015-02/msg04468.html Thanks Wen Congyang
diff --git a/block.c b/block.c index aeb365b..d02d9e9 100644 --- a/block.c +++ b/block.c @@ -747,6 +747,15 @@ static const BdrvChildRole child_backing = { .inherit_flags = bdrv_backing_flags, }; +static int bdrv_backing_rw_flags(int flags) +{ + return bdrv_backing_flags(flags) | BDRV_O_RDWR; +} + +static const BdrvChildRole child_backing_rw = { + .inherit_flags = bdrv_backing_rw_flags, +}; + static int bdrv_open_flags(BlockDriverState *bs, int flags) { int open_flags = flags | BDRV_O_CACHE_WB; @@ -1169,6 +1178,20 @@ out: bdrv_refresh_limits(bs, NULL); } +#define ALLOW_WRITE_BACKING_FILE "allow-write-backing-file" +static QemuOptsList backing_file_opts = { + .name = "backing_file", + .head = QTAILQ_HEAD_INITIALIZER(backing_file_opts.head), + .desc = { + { + .name = ALLOW_WRITE_BACKING_FILE, + .type = QEMU_OPT_BOOL, + .help = "allow write to backing file", + }, + { /* end of list */ } + }, +}; + /* * Opens the backing file for a BlockDriverState if not yet open * @@ -1183,6 +1206,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) int ret = 0; BlockDriverState *backing_hd; Error *local_err = NULL; + QemuOpts *opts = NULL; + bool child_rw = false; + const BdrvChildRole *child_role = NULL; if (bs->backing_hd != NULL) { QDECREF(options); @@ -1195,6 +1221,18 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) } bs->open_flags &= ~BDRV_O_NO_BACKING; + + opts = qemu_opts_create(&backing_file_opts, NULL, 0, &error_abort); + qemu_opts_absorb_qdict(opts, options, &local_err); + if (local_err) { + ret = -EINVAL; + error_propagate(errp, local_err); + QDECREF(options); + goto free_exit; + } + child_rw = qemu_opt_get_bool(opts, ALLOW_WRITE_BACKING_FILE, false); + child_role = child_rw ? &child_backing_rw : &child_backing; + if (qdict_haskey(options, "file.filename")) { backing_filename[0] = '\0'; } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) { @@ -1227,7 +1265,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_backing, + NULL, options, 0, bs, child_role, NULL, &local_err); if (ret < 0) { bdrv_unref(backing_hd); @@ -1242,6 +1280,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) bdrv_set_backing_hd(bs, backing_hd); free_exit: + qemu_opts_del(opts); g_free(backing_filename); return ret; } diff --git a/qapi/block-core.json b/qapi/block-core.json index 50292b9..bf141a2 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1408,6 +1408,10 @@ # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1) # (default: off) # +# @allow-write-backing-file: #optional whether the backing file is opened in +# read-write mode. It is only for backing file +# (Since 2.5 default: false) +# # Since: 1.7 ## { 'struct': 'BlockdevOptionsBase', @@ -1420,7 +1424,8 @@ '*rerror': 'BlockdevOnError', '*werror': 'BlockdevOnError', '*read-only': 'bool', - '*detect-zeroes': 'BlockdevDetectZeroesOptions' } } + '*detect-zeroes': 'BlockdevDetectZeroesOptions', + '*allow-write-backing-file': 'bool' } } ## # @BlockdevOptionsFile