| Message ID | 1473424308-19812-2-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
On 09.09.2016 15:31, Denis V. Lunev wrote: > They should work very similar, covering same areas if backing store is > shorter than the image. This change is necessary for the followup patch > switching to bdrv_get_block_status_above() in mirror to avoid assert > in check_block. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Stefan Hajnoczi <stefanha@redhat.com> > CC: Fam Zheng <famz@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > CC: Jeff Cody <jcody@redhat.com> > --- > block/io.c | 26 ++++++++++++++++++++------ > 1 file changed, 20 insertions(+), 6 deletions(-) > > diff --git a/block/io.c b/block/io.c > index 420944d..0422123 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1745,14 +1745,28 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs, > > assert(bs != base); > for (p = bs; p != base; p = backing_bs(p)) { > - ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file); > - if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) { > - break; > + int sc; > + ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file); > + if (ret < 0) { > + return ret; > + } else if (ret & BDRV_BLOCK_ALLOCATED) { > + *pnum = sc; > + return ret; > + } > + > + /* > + * [sector_num, nb_sectors] is unallocated on top but intermediate > + * might have > + * > + * [sector_num+x, nr_sectors] allocated. > + */ this comment is unrelated here, as you reduce nb_sectors (used in bdrv_co_get_block_status() above) in the following "if" > + if (nb_sectors > sc && > + (p == bs || sector_num + sc < p->total_sectors)) { > + nb_sectors = sc; > } > - /* [sector_num, pnum] unallocated on this layer, which could be only > - * the first part of [sector_num, nb_sectors]. */ > - nb_sectors = MIN(nb_sectors, *pnum); > } > + > + *pnum = nb_sectors; > return ret; > } >
On Fri, Sep 09, 2016 at 03:31:47PM +0300, Denis V. Lunev wrote: > They should work very similar, covering same areas if backing store is > shorter than the image. This change is necessary for the followup patch > switching to bdrv_get_block_status_above() in mirror to avoid assert > in check_block. I wonder why bdrv_is_allocated_above has to be a separate function rather than a trivial wrapper around bdrv_get_block_status_above() (like bdrv_is_allocated() is over bdrv_get_block_status())? > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Stefan Hajnoczi <stefanha@redhat.com> > CC: Fam Zheng <famz@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > CC: Jeff Cody <jcody@redhat.com> > --- > block/io.c | 26 ++++++++++++++++++++------ > 1 file changed, 20 insertions(+), 6 deletions(-) > > diff --git a/block/io.c b/block/io.c > index 420944d..0422123 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1745,14 +1745,28 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs, > > assert(bs != base); > for (p = bs; p != base; p = backing_bs(p)) { > - ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file); > - if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) { > - break; > + int sc; > + ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file); > + if (ret < 0) { > + return ret; > + } else if (ret & BDRV_BLOCK_ALLOCATED) { > + *pnum = sc; > + return ret; > + } > + > + /* > + * [sector_num, nb_sectors] is unallocated on top but intermediate > + * might have > + * > + * [sector_num+x, nr_sectors] allocated. > + */ > + if (nb_sectors > sc && > + (p == bs || sector_num + sc < p->total_sectors)) { > + nb_sectors = sc; > } > - /* [sector_num, pnum] unallocated on this layer, which could be only > - * the first part of [sector_num, nb_sectors]. */ > - nb_sectors = MIN(nb_sectors, *pnum); > } > + > + *pnum = nb_sectors; > return ret; IIUC in the chain image->backing_1->backing_2, where size(image) > size(backing_1) and size(backing_1) < size(backing_2), if the status of blocks beyond size(backing_1) is requested we'll start falling through to backing_2. I'm not certain this is desirable. (And yes, this is already the case in bdrv_is_allocated_above). Roman.
diff --git a/block/io.c b/block/io.c index 420944d..0422123 100644 --- a/block/io.c +++ b/block/io.c @@ -1745,14 +1745,28 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs, assert(bs != base); for (p = bs; p != base; p = backing_bs(p)) { - ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file); - if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) { - break; + int sc; + ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file); + if (ret < 0) { + return ret; + } else if (ret & BDRV_BLOCK_ALLOCATED) { + *pnum = sc; + return ret; + } + + /* + * [sector_num, nb_sectors] is unallocated on top but intermediate + * might have + * + * [sector_num+x, nr_sectors] allocated. + */ + if (nb_sectors > sc && + (p == bs || sector_num + sc < p->total_sectors)) { + nb_sectors = sc; } - /* [sector_num, pnum] unallocated on this layer, which could be only - * the first part of [sector_num, nb_sectors]. */ - nb_sectors = MIN(nb_sectors, *pnum); } + + *pnum = nb_sectors; return ret; }
They should work very similar, covering same areas if backing store is shorter than the image. This change is necessary for the followup patch switching to bdrv_get_block_status_above() in mirror to avoid assert in check_block. Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Stefan Hajnoczi <stefanha@redhat.com> CC: Fam Zheng <famz@redhat.com> CC: Kevin Wolf <kwolf@redhat.com> CC: Max Reitz <mreitz@redhat.com> CC: Jeff Cody <jcody@redhat.com> --- block/io.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)