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

[Blue Swirl]

On Thu, Mar 17, 2011 at 6:44 PM, Stefan Hajnoczi <address@hidden> wrote:
> 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.

No need for such hacks. There could be an explicit reopen method,
which could be implemented differently for each backing file type
(file descriptor, file, socket, CDROM devices etc).