On 23.04.2016 08:26, 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 <[email protected]> > --- > qemu-img.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 06264d9..b771227 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) > +{ > + while (bs->backing) { > + int64_t ret; > + BlockDriverState *file; > + bs = bs->backing->bs; > + ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum, &file); > + if (ret < 0 || ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) { > + return ret; > + }
Denis' suggestion was to basically drop this function and call
bdrv_get_block_status_above() instead.
I wouldn't strictly oppose keeping this function because of the
BDRV_BLOCK_{DATA,ZERO} vs. BDRV_BLOCK_ALLOCATED issue I talked about before.
One thing we can definitely learn from bdrv_get_block_status_above(),
though, is that a
nb_sectors = MIN(nb_sectors, *pnum);
is missing here (see bdrv_co_get_block_status_above()).
(Sorry for not noticing this before)
> + }
> + return -1;
> +}
> +
> static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
> {
> int64_t ret;
> @@ -1477,10 +1493,21 @@ 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);
In any case, I think I agree with Denis that just calling
ret = bdrv_get_block_status_above(blk_bs(s->src[s->src_cur]), NULL,
sector_num - s->src_cur_offset, n, &n,
&file);
would be sufficient.
(Sorry for not noticing this either...)
Max
> + if (ret < 0) {
> + return ret;
> + }
> +
> + 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
