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

[Stefan Hajnoczi]

On Thu, Jul 28, 2011 at 11:23 AM, Kevin Wolf <address@hidden> wrote:
> 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.

Point taken.

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

My concern is that this assumes a single file descriptor.  For vmdk
there may be multiple split files.

I'm almost starting to think bdrv_reopen() should be recursive down
the BlockDriverState tree.

Stefan