Re: [Qemu-devel] [PATCH 2/4] block: add a helper to change writeback mode on the fly

[Stefan Hajnoczi]

On Thu, Mar 17, 2011 at 3:11 PM, Kevin Wolf <address@hidden> wrote:
> Am 17.03.2011 15:44, schrieb Daniel P. Berrange:
>> On Tue, Mar 15, 2011 at 03:11:32PM +0100, Christoph Hellwig wrote:
>>> Add a new bdrv_change_cache that can set/clear the writeback flag
>>> at runtime by stopping all I/O and closing/reopening the image file.
>>>
>>> All code is based on a patch from Prerna Saxena <address@hidden>
>>> with minimal refactoring.
>>>
>>> Signed-off-by: Christoph Hellwig <address@hidden>
>>>
>>>
>>> +static int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
>>> +{
>>> +    BlockDriver *drv = bs->drv;
>>> +    int ret;
>>> +
>>> +    if (bdrv_flags == bs->open_flags) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    /* Quiesce IO for the given block device */
>>> +    qemu_aio_flush();
>>> +    bdrv_flush(bs);
>>> +
>>> +    bdrv_close(bs);
>>> +    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
>>> +
>>> +    /*
>>> +     * A failed attempt to reopen the image file must lead to 'abort()'
>>> +     */
>>> +    if (ret != 0) {
>>> +        abort();
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>
>>
>>
>>> +
>>> +int bdrv_change_cache(BlockDriverState *bs, bool enable)
>>> +{
>>> +    int bdrv_flags = 0;
>>> +
>>> +    bdrv_flags = bs->open_flags & ~BDRV_O_CACHE_WB;
>>> +    if (enable) {
>>> +        bdrv_flags |= BDRV_O_CACHE_WB;
>>> +    }
>>> +
>>> +    return bdrv_reopen(bs, bdrv_flags);
>>> +}
>>
>> Is there any way we can manage todo this *without* closing and
>> re-opening the file descriptor ?
>>
>> One of the things we're considering for the future is to enable
>> QEMU to be passed open FD(s) for its drives, from the management
>> system, instead of having QEMU open the files itself.
>
> What about cache mode, read-only flags etc. that are set with the right
> flags during the open? Must qemu just assume that the management has
> done the right thing?
>
> And what about things like backing files or other information that
> depends on the content of the image file?
>
>> If QEMU expects to close+reopen any of its disks at any time the
>> guest requests, this will complicate life somewhat
>
> It does expect this. For example for making backing files temporarily
> read-write during a 'commit' monitor command, we already reopen the
> image. (Let's hope nobody uses -snapshot, live snapshots and commit, he
> would lose his disk...)

I think we need to use the most robust solution possible.  We don't
want to get into a situation that can be avoided.  Especially in cases
where human errors can (==will) be made.

I suggested using /proc/$pid/fd to reopen an existing file descriptor.
 That interface is only available on Linux and possibly some of
{Solaris, *BSD, OSX}.  So there needs to be a fallback, but in this
situation it feels right to take advantage of whatever means are
provided by the host OS to make safe transitions between image files.

Stefan