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

[Kevin Wolf]

Am 27.07.2011 16:51, schrieb 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.

In general I really like understatements, but opening an image file
twice isn't only "not ideal", but should be considered verboten.

We're still doing it during migration and it means that in upstream qemu
any non-raw images will be corrupted.

> 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.

Changing the format drivers doesn't really look like the right solution.

Keeping the fd around looks okay, we can probably achieve this by
introducing a bdrv_reopen function. It means that we may need to reopen
the format layer, but it can't really fail.

Kevin