Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Jeff Cody <jcody@redhat.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.