From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56750) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TCQjp-0008Ik-Mt for qemu-devel@nongnu.org; Fri, 14 Sep 2012 03:54:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TCQjl-0007jQ-GB for qemu-devel@nongnu.org; Fri, 14 Sep 2012 03:54:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16482) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TCQjl-0007jM-7G for qemu-devel@nongnu.org; Fri, 14 Sep 2012 03:54:53 -0400 Message-ID: <5052E2B1.1000101@redhat.com> Date: Fri, 14 Sep 2012 09:54:25 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <01df3140216ac0398bfb3a295c553c42cdf31e5b.1347548248.git.jcody@redhat.com> <5052060A.5010304@redhat.com> <50521531.6010308@redhat.com> <50522C73.5030100@redhat.com> <50522E32.4030406@redhat.com> <505237A6.7090202@redhat.com> <50524246.5080800@redhat.com> <5052540F.5050501@redhat.com> In-Reply-To: <5052540F.5050501@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: jcody@redhat.com Cc: stefanha@gmail.com, Paolo Bonzini , eblake@redhat.com, qemu-devel@nongnu.org, supriyak@linux.vnet.ibm.com Am 13.09.2012 23:45, schrieb Jeff Cody: > On 09/13/2012 04:29 PM, Paolo Bonzini wrote: >> Il 13/09/2012 21:44, Paolo Bonzini ha scritto: >>> Il 13/09/2012 21:04, Jeff Cody ha scritto: >>>>>> Perhaps we _should_ preserve that in bs->open_flags, while still using >>>>>> the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache. >>>> That would work. Part of the problem is that BDRV_O_CACHE_WB seems >>>> overloaded; maybe bdrv_parse_cache_flags() should set a new flag, called >>>> BDRV_O_CACHE_WCE, which can be used in lieu of enable_write_cache >>>> (similar to getting rid of keep_read_only in favor of >>>> BDRV_O_ALLOW_RDWR). And then bdrv_parse_cache_flags() would just not >>>> set BDRV_O_CACHE_WB, which can then be used exclusively by the lower >>>> layers for their parsing (and bdrv_open_common would just set >>>> bs->open_flags to always have BDRV_O_CACHE_WB). >>>> >>>> Then patch 2/16 would change to having bdrv_set_enable_write_cache() >>>> toggle BDRV_O_CACHE_WCE. >>>> >>> >>> Yeah, that would work. >>> >>> Alternatively, we can keep this patch and leave bdrv_open_common as is; >>> but then I would also prefer if raw-{posix,win32} took care of setting >>> BDRV_O_CACHE_WB in bs->open_flags, so that the flags are consistent. >>> This setting of BDRV_O_CACHE_WB can be extended later to other formats. >> >> Hmm, no, what was I thinking... >> >> But there is a very simple thing we can do: >> >> - patch 2 can be left as is >> >> - in patch 3 bdrv_reopen_queue, always add BDRV_O_CACHE_WB to >> bs_entry->state.flags >> >> - in patch 3 bdrv_reopen_commit, always leave BDRV_O_CACHE_WB aside: >> >> reopen_state->bs->open_flags = >> (bs->open_flags & BDRV_O_CACHE_WB) | >> (reopen_state->flags & ~BDRV_O_CACHE_WB); >> >> - this patch can be dropped completely. >> > > Yes, I think that would work. The only thing I don't like is that > BDRV_O_CACHE_WB still remains a bit confusing when looking through the > code... bs->open_flags does not actually represent the open flags. > > With what I proposed above, here are the steps needed: > > - bdrv_parse_cache_flags() sets BDRV_O_CACHE_WCE instead of > BDRV_O_CACHE_WB > > - BDRV_O_CACHE_WCE is used everywhere enable_write_cache was used > > - bs->enable_write_cache is removed > > - this patch is dropped > > - bdrv_open_common() sets bs->open_flags to have BDRV_O_CACHE_WB enabled > by default. > > - The only way BDRV_O_CACHE_WB would get cleared from bs->open_flags now > would be if someone explicitly cleared it during a bdrv_reopen(). This feels totally wrong. Your BDRV_O_CACHE_WCE is what BDRV_O_CACHE_WB should be; removing bs->enable_write_cache in favour of a fix BDRV_O_CACHE_WB makes sense, though (maybe we need to consider renaming bs->open_flags then, it really hasn't anything to do with open and more). All block drivers, including raw-{posix,win32}, should completely ignore the flag in their .bdrv_open implementation, this flag is purely for the generic block layer. Drivers always open everything writeback and provide a flush function. They already do today, because today's broken BDRV_O_CACHE_WB is always set. Kevin