| Message ID | 1461392787-26897-2-git-send-email-rkx1209dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <rkx1209dev@gmail.com> > --- > 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; > } >
Aha. I realized what bdrv_get_block_status_above(bs, NULL...) meant. Sorry for many times resendings. I'll fix it next time then. Ren
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; + } + } + 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); + 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; }
When converting images, check the block status of its backing file chain to avoid needlessly reading zeros. Signed-off-by: Ren Kimura <rkx1209dev@gmail.com> --- qemu-img.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)