| Message ID | 20190809161407.11920-31-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Deal with filters | expand |
09.08.2019 19:13, Max Reitz wrote: > This changes iotest 204's output, because blkdebug on top of a COW node > used to make qemu-img map disregard the rest of the backing chain (the > backing chain was broken by the filter). With this patch, the > allocation in the base image is reported correctly. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qemu-img.c | 33 ++++++++++++++++++++------------- > tests/qemu-iotests/204.out | 1 + > 2 files changed, 21 insertions(+), 13 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 79983772de..3b30c5ae70 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1012,7 +1012,7 @@ static int img_commit(int argc, char **argv) > /* This is different from QMP, which by default uses the deepest file in > * the backing chain (i.e., the very base); however, the traditional > * behavior of qemu-img commit is using the immediate backing file. */ > - base_bs = backing_bs(bs); > + base_bs = bdrv_backing_chain_next(bs); > if (!base_bs) { > error_setg(&local_err, "Image does not have a backing file"); > goto done; > @@ -1632,18 +1632,20 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) > if (s->sector_next_status <= sector_num) { > uint64_t offset = (sector_num - src_cur_offset) * BDRV_SECTOR_SIZE; > int64_t count; > + BlockDriverState *src_bs = blk_bs(s->src[src_cur]); > + BlockDriverState *base; > + > + if (s->target_has_backing) { > + base = bdrv_filtered_cow_bs(bdrv_skip_rw_filters(src_bs)); > + } else { > + base = NULL; > + } > > do { > count = n * BDRV_SECTOR_SIZE; > > - if (s->target_has_backing) { > - ret = bdrv_block_status(blk_bs(s->src[src_cur]), offset, > - count, &count, NULL, NULL); > - } else { > - ret = bdrv_block_status_above(blk_bs(s->src[src_cur]), NULL, > - offset, count, &count, NULL, > - NULL); > - } > + ret = bdrv_block_status_above(src_bs, base, offset, count, &count, > + NULL, NULL); > > if (ret < 0) { > if (s->salvage) { > @@ -2490,7 +2492,8 @@ static int img_convert(int argc, char **argv) > * s.target_backing_sectors has to be negative, which it will > * be automatically). The backing file length is used only > * for optimizations, so such a case is not fatal. */ > - s.target_backing_sectors = bdrv_nb_sectors(out_bs->backing->bs); > + s.target_backing_sectors = > + bdrv_nb_sectors(bdrv_filtered_cow_bs(out_bs)); why not skip_rw_filters? It will fail if out_bs is filter.. > } else { > s.target_backing_sectors = -1; > } > @@ -2853,6 +2856,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset, > > depth = 0; > for (;;) { > + bs = bdrv_skip_rw_filters(bs); > ret = bdrv_block_status(bs, offset, bytes, &bytes, &map, &file); > if (ret < 0) { > return ret; > @@ -2861,7 +2865,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset, > if (ret & (BDRV_BLOCK_ZERO|BDRV_BLOCK_DATA)) { > break; > } > - bs = backing_bs(bs); > + bs = bdrv_filtered_cow_bs(bs); > if (bs == NULL) { > ret = 0; > break; > @@ -3216,6 +3220,7 @@ static int img_rebase(int argc, char **argv) > uint8_t *buf_old = NULL; > uint8_t *buf_new = NULL; > BlockDriverState *bs = NULL, *prefix_chain_bs = NULL; > + BlockDriverState *unfiltered_bs; > char *filename; > const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg; > int c, flags, src_flags, ret; > @@ -3350,6 +3355,8 @@ static int img_rebase(int argc, char **argv) > } > bs = blk_bs(blk); > > + unfiltered_bs = bdrv_skip_rw_filters(bs); > + > if (out_basefmt != NULL) { > if (bdrv_find_format(out_basefmt) == NULL) { > error_report("Invalid format name: '%s'", out_basefmt); > @@ -3361,7 +3368,7 @@ static int img_rebase(int argc, char **argv) > /* For safe rebasing we need to compare old and new backing file */ > if (!unsafe) { > QDict *options = NULL; > - BlockDriverState *base_bs = backing_bs(bs); > + BlockDriverState *base_bs = bdrv_filtered_cow_bs(unfiltered_bs); > > if (base_bs) { > blk_old_backing = blk_new(qemu_get_aio_context(), > @@ -3517,7 +3524,7 @@ static int img_rebase(int argc, char **argv) > * If cluster wasn't changed since prefix_chain, we don't need > * to take action > */ > - ret = bdrv_is_allocated_above(backing_bs(bs), prefix_chain_bs, > + ret = bdrv_is_allocated_above(unfiltered_bs, prefix_chain_bs, > false, offset, n, &n); > if (ret < 0) { > error_report("error while reading image metadata: %s", > diff --git a/tests/qemu-iotests/204.out b/tests/qemu-iotests/204.out > index f3a10fbe90..684774d763 100644 > --- a/tests/qemu-iotests/204.out > +++ b/tests/qemu-iotests/204.out > @@ -59,5 +59,6 @@ Offset Length File > 0x900000 0x2400000 TEST_DIR/t.IMGFMT > 0x3c00000 0x1100000 TEST_DIR/t.IMGFMT > 0x6a00000 0x400000 TEST_DIR/t.IMGFMT > +0x6e00000 0x1200000 TEST_DIR/t.IMGFMT.base > No errors were found on the image. > *** done >
On 12.08.19 14:14, Vladimir Sementsov-Ogievskiy wrote: > 09.08.2019 19:13, Max Reitz wrote: >> This changes iotest 204's output, because blkdebug on top of a COW node >> used to make qemu-img map disregard the rest of the backing chain (the >> backing chain was broken by the filter). With this patch, the >> allocation in the base image is reported correctly. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> qemu-img.c | 33 ++++++++++++++++++++------------- >> tests/qemu-iotests/204.out | 1 + >> 2 files changed, 21 insertions(+), 13 deletions(-) >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 79983772de..3b30c5ae70 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c [...] >> @@ -2490,7 +2492,8 @@ static int img_convert(int argc, char **argv) >> * s.target_backing_sectors has to be negative, which it will >> * be automatically). The backing file length is used only >> * for optimizations, so such a case is not fatal. */ >> - s.target_backing_sectors = bdrv_nb_sectors(out_bs->backing->bs); >> + s.target_backing_sectors = >> + bdrv_nb_sectors(bdrv_filtered_cow_bs(out_bs)); > > why not skip_rw_filters? It will fail if out_bs is filter.. Because I forgot this place. :-) Although backing_chain_next() would be simpler here and do the same, effectively. Max
09.08.2019 19:13, Max Reitz wrote: > This changes iotest 204's output, because blkdebug on top of a COW node > used to make qemu-img map disregard the rest of the backing chain (the > backing chain was broken by the filter). With this patch, the > allocation in the base image is reported correctly. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qemu-img.c | 33 ++++++++++++++++++++------------- > tests/qemu-iotests/204.out | 1 + > 2 files changed, 21 insertions(+), 13 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 79983772de..3b30c5ae70 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1012,7 +1012,7 @@ static int img_commit(int argc, char **argv) > /* This is different from QMP, which by default uses the deepest file in > * the backing chain (i.e., the very base); however, the traditional > * behavior of qemu-img commit is using the immediate backing file. */ > - base_bs = backing_bs(bs); > + base_bs = bdrv_backing_chain_next(bs); > if (!base_bs) { > error_setg(&local_err, "Image does not have a backing file"); > goto done; > @@ -1632,18 +1632,20 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) > if (s->sector_next_status <= sector_num) { > uint64_t offset = (sector_num - src_cur_offset) * BDRV_SECTOR_SIZE; > int64_t count; > + BlockDriverState *src_bs = blk_bs(s->src[src_cur]); > + BlockDriverState *base; > + > + if (s->target_has_backing) { > + base = bdrv_filtered_cow_bs(bdrv_skip_rw_filters(src_bs)); > + } else { > + base = NULL; > + } > > do { > count = n * BDRV_SECTOR_SIZE; > > - if (s->target_has_backing) { > - ret = bdrv_block_status(blk_bs(s->src[src_cur]), offset, > - count, &count, NULL, NULL); > - } else { > - ret = bdrv_block_status_above(blk_bs(s->src[src_cur]), NULL, > - offset, count, &count, NULL, > - NULL); > - } > + ret = bdrv_block_status_above(src_bs, base, offset, count, &count, > + NULL, NULL); > > if (ret < 0) { > if (s->salvage) { > @@ -2490,7 +2492,8 @@ static int img_convert(int argc, char **argv) > * s.target_backing_sectors has to be negative, which it will > * be automatically). The backing file length is used only > * for optimizations, so such a case is not fatal. */ > - s.target_backing_sectors = bdrv_nb_sectors(out_bs->backing->bs); > + s.target_backing_sectors = > + bdrv_nb_sectors(bdrv_filtered_cow_bs(out_bs)); bdrv_nb_sectors(bdrv_backing_chain_next(out_bs)) > } else { > s.target_backing_sectors = -1; > } > @@ -2853,6 +2856,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset, > > depth = 0; > for (;;) { > + bs = bdrv_skip_rw_filters(bs); > ret = bdrv_block_status(bs, offset, bytes, &bytes, &map, &file); > if (ret < 0) { > return ret; > @@ -2861,7 +2865,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset, > if (ret & (BDRV_BLOCK_ZERO|BDRV_BLOCK_DATA)) { > break; > } > - bs = backing_bs(bs); > + bs = bdrv_filtered_cow_bs(bs); > if (bs == NULL) { > ret = 0; > break; > @@ -3216,6 +3220,7 @@ static int img_rebase(int argc, char **argv) > uint8_t *buf_old = NULL; > uint8_t *buf_new = NULL; > BlockDriverState *bs = NULL, *prefix_chain_bs = NULL; > + BlockDriverState *unfiltered_bs; > char *filename; > const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg; > int c, flags, src_flags, ret; > @@ -3350,6 +3355,8 @@ static int img_rebase(int argc, char **argv) > } > bs = blk_bs(blk); > > + unfiltered_bs = bdrv_skip_rw_filters(bs); > + > if (out_basefmt != NULL) { > if (bdrv_find_format(out_basefmt) == NULL) { > error_report("Invalid format name: '%s'", out_basefmt); > @@ -3361,7 +3368,7 @@ static int img_rebase(int argc, char **argv) > /* For safe rebasing we need to compare old and new backing file */ > if (!unsafe) { > QDict *options = NULL; > - BlockDriverState *base_bs = backing_bs(bs); > + BlockDriverState *base_bs = bdrv_filtered_cow_bs(unfiltered_bs); > > if (base_bs) { > blk_old_backing = blk_new(qemu_get_aio_context(), > @@ -3517,7 +3524,7 @@ static int img_rebase(int argc, char **argv) > * If cluster wasn't changed since prefix_chain, we don't need > * to take action > */ > - ret = bdrv_is_allocated_above(backing_bs(bs), prefix_chain_bs, > + ret = bdrv_is_allocated_above(unfiltered_bs, prefix_chain_bs, s/unfiltered_bs/bdrv_filtered_cow_bs(unfiltered_bs)/ > false, offset, n, &n); > if (ret < 0) { > error_report("error while reading image metadata: %s", > diff --git a/tests/qemu-iotests/204.out b/tests/qemu-iotests/204.out > index f3a10fbe90..684774d763 100644 > --- a/tests/qemu-iotests/204.out > +++ b/tests/qemu-iotests/204.out > @@ -59,5 +59,6 @@ Offset Length File > 0x900000 0x2400000 TEST_DIR/t.IMGFMT > 0x3c00000 0x1100000 TEST_DIR/t.IMGFMT > 0x6a00000 0x400000 TEST_DIR/t.IMGFMT > +0x6e00000 0x1200000 TEST_DIR/t.IMGFMT.base > No errors were found on the image. > *** done > With two fixes: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
diff --git a/qemu-img.c b/qemu-img.c index 79983772de..3b30c5ae70 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1012,7 +1012,7 @@ static int img_commit(int argc, char **argv) /* This is different from QMP, which by default uses the deepest file in * the backing chain (i.e., the very base); however, the traditional * behavior of qemu-img commit is using the immediate backing file. */ - base_bs = backing_bs(bs); + base_bs = bdrv_backing_chain_next(bs); if (!base_bs) { error_setg(&local_err, "Image does not have a backing file"); goto done; @@ -1632,18 +1632,20 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) if (s->sector_next_status <= sector_num) { uint64_t offset = (sector_num - src_cur_offset) * BDRV_SECTOR_SIZE; int64_t count; + BlockDriverState *src_bs = blk_bs(s->src[src_cur]); + BlockDriverState *base; + + if (s->target_has_backing) { + base = bdrv_filtered_cow_bs(bdrv_skip_rw_filters(src_bs)); + } else { + base = NULL; + } do { count = n * BDRV_SECTOR_SIZE; - if (s->target_has_backing) { - ret = bdrv_block_status(blk_bs(s->src[src_cur]), offset, - count, &count, NULL, NULL); - } else { - ret = bdrv_block_status_above(blk_bs(s->src[src_cur]), NULL, - offset, count, &count, NULL, - NULL); - } + ret = bdrv_block_status_above(src_bs, base, offset, count, &count, + NULL, NULL); if (ret < 0) { if (s->salvage) { @@ -2490,7 +2492,8 @@ static int img_convert(int argc, char **argv) * s.target_backing_sectors has to be negative, which it will * be automatically). The backing file length is used only * for optimizations, so such a case is not fatal. */ - s.target_backing_sectors = bdrv_nb_sectors(out_bs->backing->bs); + s.target_backing_sectors = + bdrv_nb_sectors(bdrv_filtered_cow_bs(out_bs)); } else { s.target_backing_sectors = -1; } @@ -2853,6 +2856,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset, depth = 0; for (;;) { + bs = bdrv_skip_rw_filters(bs); ret = bdrv_block_status(bs, offset, bytes, &bytes, &map, &file); if (ret < 0) { return ret; @@ -2861,7 +2865,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset, if (ret & (BDRV_BLOCK_ZERO|BDRV_BLOCK_DATA)) { break; } - bs = backing_bs(bs); + bs = bdrv_filtered_cow_bs(bs); if (bs == NULL) { ret = 0; break; @@ -3216,6 +3220,7 @@ static int img_rebase(int argc, char **argv) uint8_t *buf_old = NULL; uint8_t *buf_new = NULL; BlockDriverState *bs = NULL, *prefix_chain_bs = NULL; + BlockDriverState *unfiltered_bs; char *filename; const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg; int c, flags, src_flags, ret; @@ -3350,6 +3355,8 @@ static int img_rebase(int argc, char **argv) } bs = blk_bs(blk); + unfiltered_bs = bdrv_skip_rw_filters(bs); + if (out_basefmt != NULL) { if (bdrv_find_format(out_basefmt) == NULL) { error_report("Invalid format name: '%s'", out_basefmt); @@ -3361,7 +3368,7 @@ static int img_rebase(int argc, char **argv) /* For safe rebasing we need to compare old and new backing file */ if (!unsafe) { QDict *options = NULL; - BlockDriverState *base_bs = backing_bs(bs); + BlockDriverState *base_bs = bdrv_filtered_cow_bs(unfiltered_bs); if (base_bs) { blk_old_backing = blk_new(qemu_get_aio_context(), @@ -3517,7 +3524,7 @@ static int img_rebase(int argc, char **argv) * If cluster wasn't changed since prefix_chain, we don't need * to take action */ - ret = bdrv_is_allocated_above(backing_bs(bs), prefix_chain_bs, + ret = bdrv_is_allocated_above(unfiltered_bs, prefix_chain_bs, false, offset, n, &n); if (ret < 0) { error_report("error while reading image metadata: %s", diff --git a/tests/qemu-iotests/204.out b/tests/qemu-iotests/204.out index f3a10fbe90..684774d763 100644 --- a/tests/qemu-iotests/204.out +++ b/tests/qemu-iotests/204.out @@ -59,5 +59,6 @@ Offset Length File 0x900000 0x2400000 TEST_DIR/t.IMGFMT 0x3c00000 0x1100000 TEST_DIR/t.IMGFMT 0x6a00000 0x400000 TEST_DIR/t.IMGFMT +0x6e00000 0x1200000 TEST_DIR/t.IMGFMT.base No errors were found on the image. *** done
This changes iotest 204's output, because blkdebug on top of a COW node used to make qemu-img map disregard the rest of the backing chain (the backing chain was broken by the filter). With this patch, the allocation in the base image is reported correctly. Signed-off-by: Max Reitz <mreitz@redhat.com> --- qemu-img.c | 33 ++++++++++++++++++++------------- tests/qemu-iotests/204.out | 1 + 2 files changed, 21 insertions(+), 13 deletions(-)