Re: [PATCH 4/5] block/io: fix bdrv_co_do_pwrite_zeroes head calculation

[Eric Blake]

On 3/2/20 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> It's wrong to update head using num in this place, as num may be
> reduced during the iteration, and we'll have wrong head value on next
> iteration.
>
> Instead update head at iteration end.
>
> Cc: address@hidden
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>   block/io.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Offhand, I don't see how this fixes any bug....
/me reads on

> diff --git a/block/io.c b/block/io.c
> index 75fd5600c2..c64566b4cf 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1785,7 +1785,6 @@ static int coroutine_fn 
> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>                * convenience, limit this request to max_transfer even if
>                * we don't need to fall back to writes.  */
>               num = MIN(MIN(bytes, max_transfer), alignment - head);
> -            head = (head + num) % alignment;
>               assert(num < max_write_zeroes);

Here, we've asserted that if head was non-zero, num was already smaller 
than max_write_zeroes.  The rest of the loop does indeed have code that 
appears like it can reduce num, but that code is guarded:

        /* limit request size */
        if (num > max_write_zeroes) {
            num = max_write_zeroes;
        }
...
        if (ret == -ENOTSUP && !(flags & BDRV_REQ_NO_FALLBACK)) {
            /* Fall back to bounce buffer if write zeroes is unsupported */
            BdrvRequestFlags write_flags = flags & ~BDRV_REQ_ZERO_WRITE;

            if ((flags & BDRV_REQ_FUA) &&
                !(bs->supported_write_flags & BDRV_REQ_FUA)) {
                /* No need for bdrv_driver_pwrite() to do a fallback
                 * flush on each chunk; use just one at the end */
                write_flags &= ~BDRV_REQ_FUA;
                need_flush = true;
            }
            num = MIN(num, max_transfer);

Oh. Now I see.  If max_write_zeroes is > max_transfer, but we fall back 
to a bounce buffer, it is indeed possible that a misaligned request that 
forces fallbacks to writes may indeed require more than one write to get 
to the point where it is then using a buffer aligned to max_write_zeroes.

Do we have an iotest provoking this, or is it theoretical?  With an 
iotest, this one is material for 5.0 even if the rest of the series 
misses soft freeze.

>           } else if (tail && num > alignment) {
>               /* Shorten the request to the last aligned sector.  */
> @@ -1844,6 +1843,9 @@ static int coroutine_fn 
> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>   
>           offset += num;
>           bytes -= num;
> +        if (head) {
> +            head = offset % alignment;
> +        }

Reviewed-by: Eric Blake <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org