From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42465) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QZPns-000633-5r for qemu-devel@nongnu.org; Wed, 22 Jun 2011 11:57:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QZPnq-0007Oq-N2 for qemu-devel@nongnu.org; Wed, 22 Jun 2011 11:57:20 -0400 Received: from e28smtp07.in.ibm.com ([122.248.162.7]:39955) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QZPnp-0007OM-SJ for qemu-devel@nongnu.org; Wed, 22 Jun 2011 11:57:18 -0400 Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60]) by e28smtp07.in.ibm.com (8.14.4/8.13.1) with ESMTP id p5MFvCTM004214 for ; Wed, 22 Jun 2011 21:27:12 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p5MFvCH34530392 for ; Wed, 22 Jun 2011 21:27:12 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p5MFvBsx028468 for ; Wed, 22 Jun 2011 21:27:11 +0530 Message-ID: <4E0213A8.2030303@in.ibm.com> Date: Wed, 22 Jun 2011 21:39:12 +0530 From: Supriya Kannery MIME-Version: 1.0 References: <20110617163710.2933.89020.sendpatchset@skannery> <20110617163806.2933.39799.sendpatchset@skannery> <4DFF5A6C.8060301@redhat.com> In-Reply-To: <4DFF5A6C.8060301@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [V2 3/3] Command "block_set" for dynamic block params change List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Christoph Hellwig On 06/20/2011 08:04 PM, Kevin Wolf wrote: > Am 17.06.2011 18:38, schrieb Supriya Kannery: >> + if (bdrv_is_inserted(bs)) { >> + /* cache change applicable only if device inserted */ >> + return bdrv_change_hostcache(bs, enable); >> + } else { >> + qerror_report(QERR_DEVICE_NOT_INSERTED, device); >> + return -1; >> + } > > I'm not so sure about this one. Why shouldn't I change the cache mode > for a device which is currently? The next thing I want to do could be > inserting a medium and using it with the new cache mode. Uninserted device has no file to be re-opened. So such a check was used to avoid calling bdrv_reopen without a filename. I agree with your point. Hostcache value change is applicable for devices that are not inserted as well. If device is inserted, request for hostcache setting change can lead to change in open_flags and then reopen of file. Otherwise, the request can result in just open_flags updation, which can be used for opening a file when a device gets inserted. Will make the above modification in V3 > >> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags) >> +{ >> + BlockDriver *drv = bs->drv; >> + int ret = 0; >> + >> + /* No need to reopen as no change in flags */ >> + if (bdrv_flags == bs->open_flags) >> + return 0; > > There could be other reasons for reopening besides changing flags, e.g. > invalidating cached metadata. > >> + >> + /* Quiesce IO for the given block device */ >> + qemu_aio_flush(); >> + bdrv_flush(bs); > > Missing error handling. > will add in V3 >> + ret = bdrv_open(bs, bs->filename, bdrv_flags, drv); >> + >> + /* >> + * A failed attempt to reopen the image file must lead to 'abort()' >> + */ >> + if (ret != 0) { >> + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); >> + abort(); >> + } > > Maybe we can retry with the old flags at least before aborting? > > Also I would like to see a (Linux specific) version that uses the old fd > for the reopen, so that we can handle files that aren't accessible with > their old name any more. This would mean adding a .bdrv_reopen callback > in raw-posix. > Will work on to implement version of re-open using fd. Since for block_set command processing, re-open using filename already works, is it ok to have the 're-open using fd' done as a separate patchset ? >> + /* Reopen file with changed set of flags */ >> + return bdrv_reopen(bs, bdrv_flags); >> +} > > Hm, interesting. Now we can get a O_DIRECT | O_SYNC mode with the > monitor. We should probably expose the same functionality for the > command line, too. > hostcache is indirectly set/reset from qemu commandline, using "cache=" (none/writeback/writethrough/unsafe) option. So should we be having "hostcache=xx" option added to -drive in addition to "cache=" ? > Kevin