Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change

[Stefan Hajnoczi]

2011/7/27 Michael Tokarev <address@hidden>:
> 27.07.2011 15:30, Supriya Kannery wrote:
>> New command "block_set" added for dynamically changing any of the block
>> device parameters. For now, dynamic setting of hostcache params using this
>> command is implemented. Other block device parameter changes, can be
>> integrated in similar lines.
>>
>> Signed-off-by: Supriya Kannery <address@hidden>
>>
>> ---
>>  block.c         |   54 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  block.h         |    2 +
>>  blockdev.c      |   61 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  blockdev.h      |    1
>>  hmp-commands.hx |   14 ++++++++++++
>>  qemu-config.c   |   13 +++++++++++
>>  qemu-option.c   |   25 ++++++++++++++++++++++
>>  qemu-option.h   |    2 +
>>  qmp-commands.hx |   28 +++++++++++++++++++++++++
>>  9 files changed, 200 insertions(+)
>>
>> Index: qemu/block.c
>> ===================================================================
>> --- qemu.orig/block.c
>> +++ qemu/block.c
>> @@ -651,6 +651,34 @@ unlink_and_fail:
>>      return ret;
>>  }
>>
>> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    int ret = 0, open_flags;
>> +
>> +    /* Quiesce IO for the given block device */
>> +    qemu_aio_flush();
>> +    if (bdrv_flush(bs)) {
>> +        qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name);
>> +        return ret;
>> +    }
>> +    open_flags = bs->open_flags;
>> +    bdrv_close(bs);
>> +
>> +    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
>> +    if (ret < 0) {
>> +        /* Reopen failed. Try to open with original flags */
>> +        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
>> +        ret = bdrv_open(bs, bs->filename, open_flags, drv);
>> +        if (ret < 0) {
>> +            /* Reopen failed with orig and modified flags */
>> +            abort();
>> +        }
>
> Can we please avoid this stuff completely?  Just keep the
> old device open still, until you're sure new one is ok.
>
> Or else it will be quite dangerous command in many cases.
> For example, after -runas/-chroot, or additional selinux
> settings or whatnot.  And in this case, instead of merely
> returning an error, we'll see abort().  Boom.

Slight complication for image formats that use a dirty bit here.  QED
has a dirty bit.  VMDK also specifies one but we don't implement it
today.

If the image file is dirty then all its metadata will be scanned for
consistency when it is opened.  This increases the bdrv_open() time
and hence the downtime of the VM.  So it is not ideal to open the
image file twice, even though there is no consistency problem.

I'll think about this some more, there are a couple of solutions like
keeping only the file descriptor around, introducing a flush command
that makes sure the file is in a clean state, or changing QED to not
do this.

Stefan