Re: [Qemu-devel] [RFC Patch 3/7]Qemu: Cmd "block_set_hostcache" for dynamic cache change

[Kevin Wolf]

Am 02.02.2012 01:09, schrieb Michael Roth:
> On 01/31/2012 09:06 PM, Supriya Kannery wrote:
>> New command "block_set_hostcache" added for dynamically changing
>> host pagecache setting of a block device.
>>
>> Usage:
>>   block_set_hostcache<device>  <option>
>>     <device>  = block device
>>     <option>  = on/off
>>
>> Example:
>>   (qemu) block_set_hostcache ide0-hd0 off
>>
>> Signed-off-by: Supriya Kannery<address@hidden>
>>
>> ---
>>   block.c         |   54 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block.h         |    2 ++
>>   blockdev.c      |   26 ++++++++++++++++++++++++++
>>   blockdev.h      |    2 ++
>>   hmp-commands.hx |   14 ++++++++++++++
>>   qmp-commands.hx |   27 +++++++++++++++++++++++++++
>>   6 files changed, 125 insertions(+)
>>
>> Index: qemu/block.c
>> ===================================================================
>> --- qemu.orig/block.c
>> +++ qemu/block.c
>> @@ -808,6 +808,35 @@ 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();
>> +    ret = bdrv_flush(bs);
>> +    if (ret != 0) {
>> +        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();
>> +        }
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   void bdrv_close(BlockDriverState *bs)
>>   {
>>       if (bs->drv) {
>> @@ -870,6 +899,33 @@ void bdrv_drain_all(void)
>>       }
>>   }
>>
>> +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)
>> +{
>> +    int bdrv_flags = bs->open_flags;
>> +
>> +    /* set hostcache flags (without changing WCE/flush bits) */
>> +    if (enable_host_cache) {
>> +        bdrv_flags&= ~BDRV_O_NOCACHE;
>> +    } else {
>> +        bdrv_flags |= BDRV_O_NOCACHE;
>> +    }
>> +
>> +    /* If no change in flags, no need to reopen */
>> +    if (bdrv_flags == bs->open_flags) {
>> +        return 0;
>> +    }
>> +
>> +    if (bdrv_is_inserted(bs)) {
>> +        /* Reopen file with changed set of flags */
>> +        bdrv_flags&= ~BDRV_O_CACHE_WB;
>> +        return bdrv_reopen(bs, bdrv_flags);
> 
> It seems like the real interface we're wanting here is bdrv_set_flags(), 
> or something along that line, with the re-opening being more of an 
> implementation detail.
> 
> For instance, with raw-posix.c:raw_reopen_prepare() we'll end up 
> skipping the re-opening completely if fcntl() is sufficient.

It's reopening a BlockDriverState, not necessarily reopening the image
file that backs it.

bdrv_set_flags() would be good name for what this series is doing, but
I've been thinking about adding a way to actually switch the image file
as well. We could need this for implementing external snapshots of
multiple images atomically.

Kevin