Re: [Qemu-devel] [PATCH 10/20] block: Move enable_write_cache to BB level

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2301 bytes --]

On 18.03.2016 19:21, Kevin Wolf wrote:
> Whether a write cache is used or not is a decision that concerns the
> user (e.g. the guest device) rather than the backend. It was already
> logically part of the BB level as bdrv_move_feature_fields() always kept
> it on top of the BDS tree; with this patch, the core of it (the actual
> flag and the additional flushes) is also implemented there.
> 
> Direct callers of bdrv_open() must pass BDRV_O_CACHE_WB now if bs
> doesn't have a BlockBackend attached.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                    | 26 +++++++++++++++++---------
>  block/block-backend.c      | 42 +++++++++++++++++++++++++++---------------
>  block/io.c                 |  2 +-
>  block/iscsi.c              |  2 +-
>  include/block/block.h      |  1 +
>  include/block/block_int.h  |  3 ---
>  tests/qemu-iotests/142     |  4 ++--
>  tests/qemu-iotests/142.out |  8 ++++----
>  8 files changed, 53 insertions(+), 35 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

I'm not so sure about the state bdrv_{set_,}enable_write_cache() are in
after this patch (e.g. the NBD client will always think the write cache
is enabled; and bdrv_set_enable_write_cache() can be used to unset
BDRV_O_CACHE_WB on BDSs), but looking at the following patches' titles,
they'll clear that up.

It appears to me that multiwrite will ignore the writethrough status,
but then again, qemu-io seems to be the only multiwrite user.

> diff --git a/block.c b/block.c
> index 172f865..9271dbb 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -3618,8 +3626,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
>              }
>  
>              /* backing files always opened read-only */
> -            back_flags =
> -                flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
> +            back_flags = flags | BDRV_O_CACHE_WB;
> +            back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);

Actually, this is the only thing the @flags parameter of this function
is used for. Maybe it can be dropped since we already regulate the
back_flags pretty strictly.

>  
>              if (backing_fmt) {
>                  backing_options = qdict_new();


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]