Re: [Qemu-devel] [PATCH 13/20] iscsi: Support BDRV_REQ_FUA

From: Max Reitz <mreitz@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>, qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 13/20] iscsi: Support BDRV_REQ_FUA
Date: Sat, 26 Mar 2016 21:44:39 +0100	[thread overview]
Message-ID: <56F6F4B7.8010909@redhat.com> (raw)
In-Reply-To: <1458325289-17848-14-git-send-email-kwolf@redhat.com>

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

On 18.03.2016 19:21, Kevin Wolf wrote:
> This replaces the existing hack in the iscsi driver that sent the FUA
> bit in writethrough mode and ignored the following flush in order to
> optimise the number of roundtrips (see commit 73b5394e).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/iscsi.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 3b54536..4f75204 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c

[...]

> @@ -1851,7 +1840,8 @@ static BlockDriver bdrv_iscsi = {
>      .bdrv_co_discard      = iscsi_co_discard,
>      .bdrv_co_write_zeroes = iscsi_co_write_zeroes,
>      .bdrv_co_readv         = iscsi_co_readv,
> -    .bdrv_co_writev        = iscsi_co_writev,
> +    .bdrv_co_writev_flags  = iscsi_co_writev_flags,
> +    .supported_write_flags = BDRV_REQ_FUA,
>      .bdrv_co_flush_to_disk = iscsi_co_flush,
>  
>  #ifdef __linux__
> 

Hm, wait, maybe not R-b. I can see three places in block/io.c which call
bdrv_co_writev(), and only one of them diverts to bdrv_co_writev_flags()
if that is available. Maybe we don't need to care about the
bounce-buffer case for write_zeroes, but I do think we need to care
about the COR case.

Of course bdrv_co_writev() can trivially be forwarded to
bdrv_co_writev_flags(), but I'm not sure who is supposed to do this
forwarding. I can imagine three ways:

(1) Keep a wrapper per block driver. Simple, but not so elegant.
(2) Make all bdrv_co_writev() callers call bdrv_co_writev_flags() if
    the former is not available but the latter is.
(3) Introduce a generic function replacing every drv->bdrv_co_writev()
    call which then decides which driver function to invoke.

Max

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