| Message ID | 1553866154-257311-2-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/stream: get rid of the base | expand |
29.03.2019 16:29, Andrey Shinkevich wrote: > A caller of the function bdrv_is_allocated_above() may want to > include the base node in the search. It is useful when we have > parallel commit/stream jobs on the same backing image chain. The > base node may be a top one of a parallel job at the same time > and go away before the first job completed. Instead of base, pass > the node that has the base as its backing one to the new function > bdrv_is_allocated_above_inclusive() to manage the issue. I think it would be enough to say that it be used in following patch to drop dependence on base from block-stream job. > > Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > --- > block/io.c | 39 ++++++++++++++++++++++++++++++++++----- > include/block/block.h | 5 ++++- > 2 files changed, 38 insertions(+), 6 deletions(-) > > diff --git a/block/io.c b/block/io.c > index dfc153b..8b273e5 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -2317,7 +2317,8 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset, > * Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP] > * > * Return true if (a prefix of) the given range is allocated in any image > - * between BASE and TOP (inclusive). BASE can be NULL to check if the given > + * between BASE and TOP (TOP included). To check the BASE image, set the > + * 'base_included' to 'true'. The BASE can be NULL to check if the given > * offset is allocated in any image of the chain. Return false otherwise, > * or negative errno on failure. > * > @@ -2329,16 +2330,21 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset, > * but 'pnum' will only be 0 when end of file is reached. > * > */ > -int bdrv_is_allocated_above(BlockDriverState *top, > - BlockDriverState *base, > - int64_t offset, int64_t bytes, int64_t *pnum) > +static int bdrv_do_is_allocated_above(BlockDriverState *top, > + BlockDriverState *base, > + bool base_included, int64_t offset, > + int64_t bytes, int64_t *pnum) > { > BlockDriverState *intermediate; > int ret; > int64_t n = bytes; > + bool exit_loop = top == base ? true : false; > + /* Sanity check */ > + base_included = base ? base_included : false; > + bool include_node = top == base ? base_included : true; > > intermediate = top; > - while (intermediate && intermediate != base) { > + while (intermediate && include_node) { > int64_t pnum_inter; > int64_t size_inter; > > @@ -2361,12 +2367,35 @@ int bdrv_is_allocated_above(BlockDriverState *top, > } > > intermediate = backing_bs(intermediate); > + include_node = intermediate != base; > + if (exit_loop) { > + include_node = false; > + } else if (!include_node && base_included) { > + /* Iterate once more */ > + include_node = true; > + exit_loop = true; > + } how about this, without additional variables: -int bdrv_is_allocated_above(BlockDriverState *top, - BlockDriverState *base, - int64_t offset, int64_t bytes, int64_t *pnum) +static int bdrv_do_is_allocated_above(BlockDriverState *top, + BlockDriverState *base, + bool base_included, int64_t offset, + int64_t bytes, int64_t *pnum) { BlockDriverState *intermediate; int ret; int64_t n = bytes; intermediate = top; - while (intermediate && intermediate != base) { + while (base_included || intermediate != base) { int64_t pnum_inter; int64_t size_inter; @@ -2360,6 +2361,11 @@ int bdrv_is_allocated_above(BlockDriverState *top, n = pnum_inter; } + if (intermediate == base) { + assert(!base_included); + break; + } + intermediate = backing_bs(intermediate); } > } > > *pnum = n; > return 0; > } > > +int bdrv_is_allocated_above(BlockDriverState *top, > + BlockDriverState *base, > + int64_t offset, int64_t bytes, int64_t *pnum) > +{ > + return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum); > +} > + > +int bdrv_is_allocated_above_inclusive(BlockDriverState *top, > + BlockDriverState *base, > + int64_t offset, int64_t bytes, > + int64_t *pnum) > +{ > + return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum); > +} > + > typedef struct BdrvVmstateCo { > BlockDriverState *bs; > QEMUIOVector *qiov; > diff --git a/include/block/block.h b/include/block/block.h > index c7a2619..a7846d9 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -448,7 +448,10 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes, > int64_t *pnum); > int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, > int64_t offset, int64_t bytes, int64_t *pnum); > - > +int bdrv_is_allocated_above_inclusive(BlockDriverState *top, > + BlockDriverState *base, > + int64_t offset, int64_t bytes, > + int64_t *pnum); > bool bdrv_is_read_only(BlockDriverState *bs); > int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, > bool ignore_allow_rdw, Error **errp); >
On 29/03/2019 19:24, Vladimir Sementsov-Ogievskiy wrote: > 29.03.2019 16:29, Andrey Shinkevich wrote: >> A caller of the function bdrv_is_allocated_above() may want to >> include the base node in the search. It is useful when we have >> parallel commit/stream jobs on the same backing image chain. The >> base node may be a top one of a parallel job at the same time >> and go away before the first job completed. Instead of base, pass >> the node that has the base as its backing one to the new function >> bdrv_is_allocated_above_inclusive() to manage the issue. > > I think it would be enough to say that it be used in following patch > to drop dependence on base from block-stream job. > >> >> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> --- >> block/io.c | 39 ++++++++++++++++++++++++++++++++++----- >> include/block/block.h | 5 ++++- >> 2 files changed, 38 insertions(+), 6 deletions(-) >> >> diff --git a/block/io.c b/block/io.c >> index dfc153b..8b273e5 100644 >> --- a/block/io.c >> +++ b/block/io.c >> @@ -2317,7 +2317,8 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset, >> * Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP] >> * >> * Return true if (a prefix of) the given range is allocated in any image >> - * between BASE and TOP (inclusive). BASE can be NULL to check if the given >> + * between BASE and TOP (TOP included). To check the BASE image, set the >> + * 'base_included' to 'true'. The BASE can be NULL to check if the given >> * offset is allocated in any image of the chain. Return false otherwise, >> * or negative errno on failure. >> * >> @@ -2329,16 +2330,21 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset, >> * but 'pnum' will only be 0 when end of file is reached. >> * >> */ >> -int bdrv_is_allocated_above(BlockDriverState *top, >> - BlockDriverState *base, >> - int64_t offset, int64_t bytes, int64_t *pnum) >> +static int bdrv_do_is_allocated_above(BlockDriverState *top, >> + BlockDriverState *base, >> + bool base_included, int64_t offset, >> + int64_t bytes, int64_t *pnum) >> { >> BlockDriverState *intermediate; >> int ret; >> int64_t n = bytes; >> + bool exit_loop = top == base ? true : false; >> + /* Sanity check */ >> + base_included = base ? base_included : false; >> + bool include_node = top == base ? base_included : true; >> >> intermediate = top; >> - while (intermediate && intermediate != base) { >> + while (intermediate && include_node) { >> int64_t pnum_inter; >> int64_t size_inter; >> >> @@ -2361,12 +2367,35 @@ int bdrv_is_allocated_above(BlockDriverState *top, >> } >> >> intermediate = backing_bs(intermediate); >> + include_node = intermediate != base; >> + if (exit_loop) { >> + include_node = false; >> + } else if (!include_node && base_included) { >> + /* Iterate once more */ >> + include_node = true; >> + exit_loop = true; >> + } > > how about this, without additional variables: Unfortunately, 9 test cases do not pass in the file 030. > > -int bdrv_is_allocated_above(BlockDriverState *top, > - BlockDriverState *base, > - int64_t offset, int64_t bytes, int64_t *pnum) > +static int bdrv_do_is_allocated_above(BlockDriverState *top, > + BlockDriverState *base, > + bool base_included, int64_t offset, > + int64_t bytes, int64_t *pnum) > { > BlockDriverState *intermediate; > int ret; > int64_t n = bytes; > > intermediate = top; > - while (intermediate && intermediate != base) { > + while (base_included || intermediate != base) { > int64_t pnum_inter; > int64_t size_inter; > > @@ -2360,6 +2361,11 @@ int bdrv_is_allocated_above(BlockDriverState *top, > n = pnum_inter; > } > > + if (intermediate == base) { > + assert(!base_included); > + break; > + } > + > intermediate = backing_bs(intermediate); > } > > > >> } >> >> *pnum = n; >> return 0; >> } >> >> +int bdrv_is_allocated_above(BlockDriverState *top, >> + BlockDriverState *base, >> + int64_t offset, int64_t bytes, int64_t *pnum) >> +{ >> + return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum); >> +} >> + >> +int bdrv_is_allocated_above_inclusive(BlockDriverState *top, >> + BlockDriverState *base, >> + int64_t offset, int64_t bytes, >> + int64_t *pnum) >> +{ >> + return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum); >> +} >> + >> typedef struct BdrvVmstateCo { >> BlockDriverState *bs; >> QEMUIOVector *qiov; >> diff --git a/include/block/block.h b/include/block/block.h >> index c7a2619..a7846d9 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -448,7 +448,10 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes, >> int64_t *pnum); >> int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, >> int64_t offset, int64_t bytes, int64_t *pnum); >> - >> +int bdrv_is_allocated_above_inclusive(BlockDriverState *top, >> + BlockDriverState *base, >> + int64_t offset, int64_t bytes, >> + int64_t *pnum); >> bool bdrv_is_read_only(BlockDriverState *bs); >> int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, >> bool ignore_allow_rdw, Error **errp); >> > >
diff --git a/block/io.c b/block/io.c index dfc153b..8b273e5 100644 --- a/block/io.c +++ b/block/io.c @@ -2317,7 +2317,8 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset, * Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP] * * Return true if (a prefix of) the given range is allocated in any image - * between BASE and TOP (inclusive). BASE can be NULL to check if the given + * between BASE and TOP (TOP included). To check the BASE image, set the + * 'base_included' to 'true'. The BASE can be NULL to check if the given * offset is allocated in any image of the chain. Return false otherwise, * or negative errno on failure. * @@ -2329,16 +2330,21 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset, * but 'pnum' will only be 0 when end of file is reached. * */ -int bdrv_is_allocated_above(BlockDriverState *top, - BlockDriverState *base, - int64_t offset, int64_t bytes, int64_t *pnum) +static int bdrv_do_is_allocated_above(BlockDriverState *top, + BlockDriverState *base, + bool base_included, int64_t offset, + int64_t bytes, int64_t *pnum) { BlockDriverState *intermediate; int ret; int64_t n = bytes; + bool exit_loop = top == base ? true : false; + /* Sanity check */ + base_included = base ? base_included : false; + bool include_node = top == base ? base_included : true; intermediate = top; - while (intermediate && intermediate != base) { + while (intermediate && include_node) { int64_t pnum_inter; int64_t size_inter; @@ -2361,12 +2367,35 @@ int bdrv_is_allocated_above(BlockDriverState *top, } intermediate = backing_bs(intermediate); + include_node = intermediate != base; + if (exit_loop) { + include_node = false; + } else if (!include_node && base_included) { + /* Iterate once more */ + include_node = true; + exit_loop = true; + } } *pnum = n; return 0; } +int bdrv_is_allocated_above(BlockDriverState *top, + BlockDriverState *base, + int64_t offset, int64_t bytes, int64_t *pnum) +{ + return bdrv_do_is_allocated_above(top, base, false, offset, bytes, pnum); +} + +int bdrv_is_allocated_above_inclusive(BlockDriverState *top, + BlockDriverState *base, + int64_t offset, int64_t bytes, + int64_t *pnum) +{ + return bdrv_do_is_allocated_above(top, base, true, offset, bytes, pnum); +} + typedef struct BdrvVmstateCo { BlockDriverState *bs; QEMUIOVector *qiov; diff --git a/include/block/block.h b/include/block/block.h index c7a2619..a7846d9 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -448,7 +448,10 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes, int64_t *pnum); int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, int64_t offset, int64_t bytes, int64_t *pnum); - +int bdrv_is_allocated_above_inclusive(BlockDriverState *top, + BlockDriverState *base, + int64_t offset, int64_t bytes, + int64_t *pnum); bool bdrv_is_read_only(BlockDriverState *bs); int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, bool ignore_allow_rdw, Error **errp);
A caller of the function bdrv_is_allocated_above() may want to include the base node in the search. It is useful when we have parallel commit/stream jobs on the same backing image chain. The base node may be a top one of a parallel job at the same time and go away before the first job completed. Instead of base, pass the node that has the base as its backing one to the new function bdrv_is_allocated_above_inclusive() to manage the issue. Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- block/io.c | 39 ++++++++++++++++++++++++++++++++++----- include/block/block.h | 5 ++++- 2 files changed, 38 insertions(+), 6 deletions(-)