On Thu, Jul 28, 2011 at 11:23 AM, Kevin Wolf <kw...@redhat.com> wrote:
> Am 27.07.2011 16:51, schrieb Stefan Hajnoczi:
>> 2011/7/27 Michael Tokarev <m...@tls.msk.ru>:
>>> 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 <supri...@in.ibm.com>
>>>>
>>>> ---
>>>> 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
