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

[Supriya Kannery <supriyak@linux.vnet.ibm.com>]

On 06/16/2012 03:26 AM, Eric Blake wrote:
> On 06/15/2012 02:47 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<supriyak@linux.vnet.ibm.com>
>>
>> ---
>>   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(+)
>
> Doesn't this also need to touch qapi-schema.json?
> [/me reads full patch]
> Oh, you did - but your diffstat is stale.  It might be worth figuring
> out what in your workflow leads to stale diffstats.
>
>>
>> Index: qemu/block.c
>> ===================================================================
>> --- qemu.orig/block.c
>> +++ qemu/block.c
>> @@ -858,6 +858,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);
>
> Yuck.  This is bad, and why 'transaction' was invented.  Any time you
> close() before open() you risk completely losing the file...
>
>> +    if (ret<  0) {
>> +        /* Reopen failed. Try to open with original flags */
>> +        qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
>> +        ret = bdrv_open(bs, bs->filename, open_flags, drv);
>> +        if (ret<  0) {
>> +            /* Reopen failed with orig and modified flags */
>> +            abort();
>
> and an abort() is not a nice reaction to that.
>
> I think we should rebase the series to do the safe reopen prior to
> adding this command (at least, just judging by the title of 4/10), to
> avoid intermediate bad code.
>
>

  Yes, will reorder the patches to have safe reopen done first and
then block_set_hostcache use it.

-thanks, Supriya