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.
