Re: [PATCH 1/4] block/io: fix bdrv_co_block_status_above

[Kevin Wolf]

Am 16.11.2019 um 17:34 hat Vladimir Sementsov-Ogievskiy geschrieben:
> bdrv_co_block_status_above has several problems with handling short
> backing files:
> 
> 1. With want_zeros=true, it may return ret with BDRV_BLOCK_ZERO but
> without BDRV_BLOCK_ALLOCATED flag, when actually short backing file
> which produces these after-EOF zeros is inside requested backing
> sequesnce.

s/sequesnce/sequence/

> 
> 2. With want_zeros=false, it will just stop inside requested region, if
> we have unallocated region in top node when underlying backing is
> short.

I honestly don't understand this one. Can you rephrase/explain in more
detail what you mean by "stop inside [the] requested region"?

> Fix these things, making logic about short backing files clearer.
> 
> Note that 154 output changed, because now bdrv_block_status_above don't
> merge unallocated zeros with zeros after EOF (which are actually
> "allocated" in POV of read from backing-chain top) and is_zero() just
> don't understand that the whole head or tail is zero. We may update
> is_zero to call bdrv_block_status_above several times, or add flag to
> bdrv_block_status_above that we are not interested in ALLOCATED flag,
> so ranges with different ALLOCATED status may be merged, but actually,
> it seems that we'd better don't care about this corner case.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/io.c                 | 41 ++++++++++++++++++++++++++++----------
>  tests/qemu-iotests/154.out |  4 ++--
>  2 files changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index f75777f5ea..4d7fa99bd2 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2434,25 +2434,44 @@ static int coroutine_fn 
> bdrv_co_block_status_above(BlockDriverState *bs,
>          ret = bdrv_co_block_status(p, want_zero, offset, bytes, pnum, map,
>                                     file);
>          if (ret < 0) {
> -            break;
> +            return ret;
>          }
> -        if (ret & BDRV_BLOCK_ZERO && ret & BDRV_BLOCK_EOF && !first) {
> +        if (*pnum == 0) {
> +            if (first) {
> +                return ret;
> +            }
> +
>              /*
> -             * Reading beyond the end of the file continues to read
> -             * zeroes, but we can only widen the result to the
> -             * unallocated length we learned from an earlier
> -             * iteration.
> +             * Reads from bs for selected region will return zeroes, produced
> +             * because current level is short. We should consider it as
> +             * allocated.

"the selected region"
"the current level"

> +             * TODO: Should we report p as file here?

I think that would make sense.

>               */
> +            assert(ret & BDRV_BLOCK_EOF);

Can this assertion be moved above the if (first)?

>              *pnum = bytes;
> +            return BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED;
>          }
> -        if (ret & (BDRV_BLOCK_ZERO | BDRV_BLOCK_DATA)) {
> -            break;
> +        if (ret & BDRV_BLOCK_ALLOCATED) {
> +            /* We've found the node and the status, we must return. */
> +
> +            if (ret & BDRV_BLOCK_ZERO && ret & BDRV_BLOCK_EOF && !first) {
> +                /*
> +                 * This level also responsible for reads after EOF inside
> +                 * unallocated region in previous level.

"is also responsible"
"the unallocated region in the previous level"

> +                 */
> +                *pnum = bytes;
> +            }
> +
> +            return ret;
>          }
> -        /* [offset, pnum] unallocated on this layer, which could be only
> -         * the first part of [offset, bytes].  */

Any reason for deleting this comment? I think it's still valid.

> -        bytes = MIN(bytes, *pnum);
> +
> +        /* Proceed to backing */
> +        assert(*pnum <= bytes);
> +        bytes = *pnum;
>          first = false;
>      }
> +
>      return ret;
>  }

Kevin