From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([140.186.70.92]:54928)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1RstdN-0005LB-W0
	for qemu-devel@nongnu.org; Thu, 02 Feb 2012 05:11:23 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1RstdI-0005HA-7S
	for qemu-devel@nongnu.org; Thu, 02 Feb 2012 05:11:17 -0500
Received: from mx1.redhat.com ([209.132.183.28]:30145)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1RstdH-0005GT-Pb
	for qemu-devel@nongnu.org; Thu, 02 Feb 2012 05:11:12 -0500
Message-ID: <4F2A6206.60309@redhat.com>
Date: Thu, 02 Feb 2012 11:14:30 +0100
From: Kevin Wolf <kwolf@redhat.com>
MIME-Version: 1.0
References: <20120201030557.2990.74150.sendpatchset@skannery.in.ibm.com>
	<20120201030641.2990.46503.sendpatchset@skannery.in.ibm.com>
	<4F29D42C.8090203@linux.vnet.ibm.com>
In-Reply-To: <4F29D42C.8090203@linux.vnet.ibm.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [RFC Patch 3/7]Qemu: Cmd "block_set_hostcache" for
 dynamic cache change
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Michael Roth <mdroth@linux.vnet.ibm.com>
Cc: Supriya Kannery <supriyak@linux.vnet.ibm.com>, Stefan Hajnoczi <stefanha@gmail.com>, Jeff Cody <jcody@redhat.com>, qemu-devel@nongnu.org, Luiz Capitulino <lcapitulino@redhat.com>, Christoph Hellwig <hch@lst.de>

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<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(+)
>>
>> 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