Re: [Qemu-block] [PATCH v2] qemu-img: check block status of backing file when converting.

[Max Reitz]

The patch looks pretty good now, just one small thing:
(Don't let the wall of text scare you)

On 15.04.2016 08:19, Ren Kimura wrote:
> When converting images, check the block status of its backing file chain
> to avoid needlessly reading zeros.
> 
> Signed-off-by: Ren Kimura <address@hidden>
> ---
>  qemu-img.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 06264d9..6330f2a 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1451,6 +1451,22 @@ static void convert_select_part(ImgConvertState *s, 
> int64_t sector_num)
>      }
>  }
>  
> +static int64_t get_backing_status(BlockDriverState *bs,
> +                                  int64_t sector_num,
> +                                  int nb_sectors, int *pnum)
> +{
> +    int64_t ret;
> +    BlockDriverState *file;
> +    ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum, &file);
> +    if (ret != BDRV_BLOCK_DATA && ret != BDRV_BLOCK_ZERO) {

The value returned by bdrv_get_block_status() is a bitmask;
BDRV_BLOCK_DATA and BDRV_BLOCK_ZERO are only two flags that may be set
in that mask.

For instance, BDRV_BLOCK_DATA and BDRV_BLOCK_ZERO may be set at the same
time, actually. According to the documentation in include/block/block.h,
this would mean that the image actually contains physical zeros in the
given range, but we know that it only contains zeros there.

(In contrast to that, BDRV_BLOCK_ZERO without BDRV_BLOCK_DATA means that
the image may contain either anything or nothing at all in the given
range; it's just some metadata structure that told us to interpret it as
if it was zero.)

Besides BDRV_BLOCK_DATA and BDRV_BLOCK_ZERO, there are some more flags
that may be set in the return value.

So the correct way to test this would be:

if (!(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO))

Or, shorter:

if (!(ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)))



Now comes optional stuff. Feel free to ignore it, if you don't really
care. I just wrote it down because it turns out that it's actually a
tricky question which flags we should query here.

When I looked all of this up, I came across another interesting flag:
BDRV_BLOCK_ALLOCATED. That flag is basically exactly what *I thought* we
want to know, i.e. whether the content of this range is determined by
this layer or by its backing file.

But it turns out that we actually don't care whether it's this layer or
the backing file that is responsible for that data we see. For instance,
if BDRV_BLOCK_ALLOCATED is not set but BDRV_BLOCK_ZERO is (this is a
valid combination), that means that it is not this layer that is
responsible for it, but we know for certain that anything we read from
it will be 0.

An example for this state is if the backing file is shorter than the
overlay. When reading something from the end of the image, the overlay
may tell you to fall through to the backing file; but the backing file
is not long enough, so you will read zeros, but those do not come from
the backing file, actually.

Another example is if you don't have a backing file at all. Then the
overlay may tell you to fall through to the backing file even if it
doesn't exist. Then, you will generally read zeros, too.

I originally said (in my reply to v1) that we may not fall through to
the backing file if the range is allocated in the overlay. That is
correct, but now it turns out that we can also skip falling through if
the range is not allocated in the overlay but the contents of the
backing file don't matter anyway.

(Note that it is impossible to have BDRV_BLOCK_ALLOCATED set without one
of BDRV_BLOCK_ZERO and BDRV_BLOCK_DATA set, too.)

Max

> +        if (bs->backing) {
> +            ret = get_backing_status(bs->backing->bs, sector_num,
> +                                     nb_sectors, pnum);
> +        }
> +    }
> +    return ret;
> +}
> +
>  static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>  {
>      int64_t ret;
> @@ -1477,10 +1493,17 @@ static int convert_iteration_sectors(ImgConvertState 
> *s, int64_t sector_num)
>          } else if (!s->target_has_backing) {
>              /* Without a target backing file we must copy over the contents 
> of
>               * the backing file as well. */
> -            /* TODO Check block status of the backing file chain to avoid
> +            /* Check block status of the backing file chain to avoid
>               * needlessly reading zeroes and limiting the iteration to the
>               * buffer size */
> -            s->status = BLK_DATA;
> +            ret = get_backing_status(blk_bs(s->src[s->src_cur]),
> +                                     sector_num - s->src_cur_offset,
> +                                     n, &n);
> +            if (ret & BDRV_BLOCK_ZERO) {
> +                s->status = BLK_ZERO;
> +            } else {
> +                s->status = BLK_DATA;
> +            }
>          } else {
>              s->status = BLK_BACKING_FILE;
>          }
>

signature.asc
Description: OpenPGP digital signature