From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:39961) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qb6zX-0006Qa-KY for qemu-devel@nongnu.org; Mon, 27 Jun 2011 04:16:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qb6zI-0000ji-Lr for qemu-devel@nongnu.org; Mon, 27 Jun 2011 04:16:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10612) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qb6zI-0000je-36 for qemu-devel@nongnu.org; Mon, 27 Jun 2011 04:16:08 -0400 Message-ID: <4E083CED.5030106@redhat.com> Date: Mon, 27 Jun 2011 10:18:53 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <20110617163710.2933.89020.sendpatchset@skannery> <20110617163806.2933.39799.sendpatchset@skannery> <4DFF5A6C.8060301@redhat.com> <4E0213A8.2030303@in.ibm.com> In-Reply-To: <4E0213A8.2030303@in.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 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: Supriya Kannery Cc: qemu-devel@nongnu.org, Christoph Hellwig Am 22.06.2011 18:09, schrieb Supriya Kannery: > 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 ? Sure, it's just an obvious improvement. >>> + /* 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=" ? I think that's the right thing to do in the long run. Christoph? Kevin