Re: [Qemu-block] [PATCH 1/3] block: include base when checking image chain for block allocation

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> Signed-off-by: Andrey Shinkevich <address@hidden>
> ---
>   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);
> 


-- 
Best regards,
Vladimir