Message ID | 1460701151-5984-1-git-send-email-rkx1209dev@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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 <rkx1209dev@gmail.com> > --- > 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; > } >
> 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))) Oops! It's very trivial mistake. I'll fix it. > 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. Aha. It's totally true. But I'd like to keep these operations simple and easy to read. So I'll fix only first one. Thank you for reviewing. Ren
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) { + 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; }
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 | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)