Re: [Qemu-block] [PATCH] block: Don't lose FUA flag during ZERO_WRITE fallback

[Kevin Wolf]

Am 30.04.2016 um 23:48 hat Eric Blake geschrieben:
> NBD has situations where it can support FUA but not ZERO_WRITE;
> when that happens, the generic block layer fallback was losing
> the FUA flag.  The problem of losing flags unrelated to
> ZERO_WRITE has been latent in bdrv_co_do_write_zeroes() since
> aa7bfbff, but back then, it did not matter because there was no
> FUA flag.  But ever since 93f5e6d8 added bdrv_co_writev_flags(),
> the loss of flags can impact correctness.
> 
> Compare to commit 9eeb6dd, which got it right in
> bdrv_co_do_zero_pwritev().
> 
> Symptoms: I tested with qemu-io with default writethrough cache
> (which is supposed to use FUA semantics on every write), and
> targetted an NBD client connected to a server that intentionally
> did not advertise NBD_FLAG_SEND_FUA.  When doing 'write 0 512',
> the NBD client sent two operations (NBD_CMD_WRITE then
> NBD_CMD_FLUSH) to get the fallback FUA semantics; but when doing
> 'write -z 0 512', the NBD client sent only NBD_CMD_WRITE; the
> missing flush meant that an ill-timed disconnect could leave
> the zeroes unflushed.
> 
> CC: address@hidden
> Signed-off-by: Eric Blake <address@hidden>
> ---
> 
> As written, this patch applies to 2.7 on top of Kevin's block-next
> branch.  Since it's (probably) too late for 2.6, we'll need to
> backport it to there, but the backport will have to use
> bdrv_co_writev_flags since 2.6 lacks bdrv_driver_pwritev().
> 
>  block/io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 0db1146..bd46e47 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1213,7 +1213,8 @@ static int coroutine_fn 
> bdrv_co_do_write_zeroes(BlockDriverState *bs,
>              qemu_iovec_init_external(&qiov, &iov, 1);
> 
>              ret = bdrv_driver_pwritev(bs, sector_num * BDRV_SECTOR_SIZE,
> -                                      num * BDRV_SECTOR_SIZE, &qiov, 0);
> +                                      num * BDRV_SECTOR_SIZE, &qiov,
> +                                      flags & ~BDRV_REQ_ZERO_WRITE);

This is a good change, but it's only in the fallback code. If we succeed
here:

    if (drv->bdrv_co_write_zeroes) {
        ret = drv->bdrv_co_write_zeroes(bs, sector_num, num, flags);
    }

then we still don't get the necessary flush unless the driver's
.bdrv_co_write_zeroes() implementation explicitly handles FUA. As far as
I know, we don't have any driver that implements FUA there.

Kevin