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