Re: [Qemu-block] [PATCH v2 15/16] block/mirror: Add copy mode QAPI interface

[Max Reitz]

On 2018-02-27 10:38, Fam Zheng wrote:
> On Mon, 01/22 23:08, Max Reitz wrote:
>> This patch allows the user to specify whether to use active or only
>> passive mode for mirror block jobs.  Currently, this setting will remain
> 
> I think you want s/passive/background/ in the whole patch.

Errr, yes.

Max

>> constant for the duration of the entire block job.
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  qapi/block-core.json      | 11 +++++++++--
>>  include/block/block_int.h |  4 +++-
>>  block/mirror.c            | 12 +++++++-----
>>  blockdev.c                |  9 ++++++++-
>>  4 files changed, 27 insertions(+), 9 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index ba1fd736f5..5fafa5fcac 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1573,6 +1573,9 @@
>>  #         written. Both will result in identical contents.
>>  #         Default is true. (Since 2.4)
>>  #
>> +# @copy-mode: when to copy data to the destination; defaults to 'passive'
>> +#             (Since: 2.12)
>> +#
>>  # Since: 1.3
>>  ##
>>  { 'struct': 'DriveMirror',
>> @@ -1582,7 +1585,7 @@
>>              '*speed': 'int', '*granularity': 'uint32',
>>              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
>>              '*on-target-error': 'BlockdevOnError',
>> -            '*unmap': 'bool' } }
>> +            '*unmap': 'bool', '*copy-mode': 'MirrorCopyMode' } }
>>  
>>  ##
>>  # @BlockDirtyBitmap:
>> @@ -1761,6 +1764,9 @@
>>  #                    above @device. If this option is not given, a node 
>> name is
>>  #                    autogenerated. (Since: 2.9)
>>  #
>> +# @copy-mode: when to copy data to the destination; defaults to 'passive'
>> +#             (Since: 2.12)
>> +#
>>  # Returns: nothing on success.
>>  #
>>  # Since: 2.6
>> @@ -1781,7 +1787,8 @@
>>              '*speed': 'int', '*granularity': 'uint32',
>>              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
>>              '*on-target-error': 'BlockdevOnError',
>> -            '*filter-node-name': 'str' } }
>> +            '*filter-node-name': 'str',
>> +            '*copy-mode': 'MirrorCopyMode' } }
>>  
>>  ##
>>  # @block_set_io_throttle:
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 03f3fdd129..1fda4d3d43 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -948,6 +948,7 @@ void commit_active_start(const char *job_id, 
>> BlockDriverState *bs,
>>   * @filter_node_name: The node name that should be assigned to the filter
>>   * driver that the mirror job inserts into the graph above @bs. NULL means 
>> that
>>   * a node name should be autogenerated.
>> + * @copy_mode: When to trigger writes to the target.
>>   * @errp: Error object.
>>   *
>>   * Start a mirroring operation on @bs.  Clusters that are allocated
>> @@ -961,7 +962,8 @@ void mirror_start(const char *job_id, BlockDriverState 
>> *bs,
>>                    MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
>>                    BlockdevOnError on_source_error,
>>                    BlockdevOnError on_target_error,
>> -                  bool unmap, const char *filter_node_name, Error **errp);
>> +                  bool unmap, const char *filter_node_name,
>> +                  MirrorCopyMode copy_mode, Error **errp);
>>  
>>  /*
>>   * backup_job_create:
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 83082adb64..3b23886a5a 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -1409,7 +1409,7 @@ static void mirror_start_job(const char *job_id, 
>> BlockDriverState *bs,
>>                               const BlockJobDriver *driver,
>>                               bool is_none_mode, BlockDriverState *base,
>>                               bool auto_complete, const char 
>> *filter_node_name,
>> -                             bool is_mirror,
>> +                             bool is_mirror, MirrorCopyMode copy_mode,
>>                               Error **errp)
>>  {
>>      MirrorBlockJob *s;
>> @@ -1515,7 +1515,7 @@ static void mirror_start_job(const char *job_id, 
>> BlockDriverState *bs,
>>      s->on_target_error = on_target_error;
>>      s->is_none_mode = is_none_mode;
>>      s->backing_mode = backing_mode;
>> -    s->copy_mode = MIRROR_COPY_MODE_BACKGROUND;
>> +    s->copy_mode = copy_mode;
>>      s->base = base;
>>      s->granularity = granularity;
>>      s->buf_size = ROUND_UP(buf_size, granularity);
>> @@ -1582,7 +1582,8 @@ void mirror_start(const char *job_id, BlockDriverState 
>> *bs,
>>                    MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
>>                    BlockdevOnError on_source_error,
>>                    BlockdevOnError on_target_error,
>> -                  bool unmap, const char *filter_node_name, Error **errp)
>> +                  bool unmap, const char *filter_node_name,
>> +                  MirrorCopyMode copy_mode, Error **errp)
>>  {
>>      bool is_none_mode;
>>      BlockDriverState *base;
>> @@ -1597,7 +1598,7 @@ void mirror_start(const char *job_id, BlockDriverState 
>> *bs,
>>                       speed, granularity, buf_size, backing_mode,
>>                       on_source_error, on_target_error, unmap, NULL, NULL,
>>                       &mirror_job_driver, is_none_mode, base, false,
>> -                     filter_node_name, true, errp);
>> +                     filter_node_name, true, copy_mode, errp);
>>  }
>>  
>>  void commit_active_start(const char *job_id, BlockDriverState *bs,
>> @@ -1620,7 +1621,8 @@ void commit_active_start(const char *job_id, 
>> BlockDriverState *bs,
>>                       MIRROR_LEAVE_BACKING_CHAIN,
>>                       on_error, on_error, true, cb, opaque,
>>                       &commit_active_job_driver, false, base, auto_complete,
>> -                     filter_node_name, false, &local_err);
>> +                     filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
> 
> I think for active commit, MIRROR_COPY_MODE_WRITE_BLOCKING is more appealing?
> Can be a task for another day, though.

Hmm...  In theory a good point.  But in practice we currently still do
the write to both the overlay and the backing file, so it has the same
issues as normal mirroring.

But for active commit, we have the interesting property that one write
would actually be sufficient: We could only do the write to the backing
file, and then punch a hole into the overlay so that the backing file
data shines through.  So if we did that, we should really do
MIRROR_COPY_MODE_WRITE_BLOCKING.

But we currently don't, so I'd rather keep it in background mode until
we do that. :-)

Max

signature.asc
Description: OpenPGP digital signature