Re: [Qemu-devel] [PATCH 03/16] allow writing to the backing file

[Wen Congyang <wency@cn.fujitsu.com>]

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