From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36373) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TCHEV-00026W-W7 for qemu-devel@nongnu.org; Thu, 13 Sep 2012 17:46:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TCHEU-0005lf-So for qemu-devel@nongnu.org; Thu, 13 Sep 2012 17:45:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26837) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TCHEU-0005lT-KH for qemu-devel@nongnu.org; Thu, 13 Sep 2012 17:45:58 -0400 Message-ID: <5052540F.5050501@redhat.com> Date: Thu, 13 Sep 2012 17:45:51 -0400 From: Jeff Cody 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> In-Reply-To: <50524246.5080800@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 Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, supriyak@linux.vnet.ibm.com, eblake@redhat.com, qemu-devel@nongnu.org, stefanha@gmail.com 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(). While there are more changes this way, I think it cleans up the code a bit. The advantage is that bs->open_flags actually reflects the open flags that are currently in use. One disadvantage I see is that it seems a bit odd to have BDRV_O_CACHE_WCE cleared and BDRV_O_CACHE_WB set, until you think about them being intended for different layers. Maybe that is part of the underlying problem - there is one open_flags variable in the BDS, that has some flags intended for all layers in the block stack, and some flags specific to a layer.