diff mbox series

[v3,1/3] block: include base when checking image chain for block allocation

Message ID 1554483379-682051-2-git-send-email-andrey.shinkevich@virtuozzo.com
State New
Headers show
Series block/stream: get rid of the base | expand

Commit Message

Andrey Shinkevich April 5, 2019, 4:56 p.m. UTC 
This patch is used in the 'block/stream: introduce a bottom node'
that is following. Instead of the base node, the caller may pass
the node that has the base as its backing image to the new function
bdrv_is_allocated_above_inclusive() and get rid of the dependency
on the base that may change during commit/stream parallel jobs.
Now, if the specified base is not found in the backing image chain,
the QEMU will crash.

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c            | 33 ++++++++++++++++++++++++++++-----
 include/block/block.h |  4 ++++
 2 files changed, 32 insertions(+), 5 deletions(-)

Comments

Alberto Garcia April 8, 2019, 3:03 p.m. UTC | #1 
On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
> +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);
> +}

Instead of having these two, isn't it simpler to add an 'include_base'
parameter to the original function?

Another alternative (I haven't checked this one so it could be more
cumbersome): change the semantics of the function to always include the
base and modify the callers.

Berto
Vladimir Sementsov-Ogievskiy April 8, 2019, 3:14 p.m. UTC | #2 
08.04.2019 18:03, Alberto Garcia wrote:
> On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
>> +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);
>> +}
> 
> Instead of having these two, isn't it simpler to add an 'include_base'
> parameter to the original function?
> 
> Another alternative (I haven't checked this one so it could be more
> cumbersome): change the semantics of the function to always include the
> base and modify the callers.
> 

The idea was exactly to avoid modifying all callers.. And it seems good, at least as first step.
We may want to modify other callers but it is out of stream series.
Andrey Shinkevich April 8, 2019, 3:16 p.m. UTC | #3 
On 08/04/2019 18:03, Alberto Garcia wrote:
> On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
>> +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);
>> +}
> 
> Instead of having these two, isn't it simpler to add an 'include_base'
> parameter to the original function?
> 
> Another alternative (I haven't checked this one so it could be more
> cumbersome): change the semantics of the function to always include the
> base and modify the callers.
> 
> Berto
> 

The idea behind those two functions was to keep the rest of the code
unmodified. Currently, we have the issue with the block-stream
parallel jobs. What if we manage this case first and then, when
proved to be robust, take care of the rest?
Alberto Garcia April 8, 2019, 3:22 p.m. UTC | #4 
On Mon 08 Apr 2019 05:16:22 PM CEST, Andrey Shinkevich wrote:
> On 08/04/2019 18:03, Alberto Garcia wrote:
>> On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
>>> +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);
>>> +}
>> 
>> Instead of having these two, isn't it simpler to add an 'include_base'
>> parameter to the original function?
>> 
>> Another alternative (I haven't checked this one so it could be more
>> cumbersome): change the semantics of the function to always include the
>> base and modify the callers.
>
> The idea behind those two functions was to keep the rest of the code
> unmodified. Currently, we have the issue with the block-stream
> parallel jobs. What if we manage this case first and then, when proved
> to be robust, take care of the rest?

Sure, that makes sense if you need to do significant changes to the
other callers, but if you just need to pass an extra 'false'
parameter...

It's not a big deal, I just think you'd have a simpler patch.

Berto
Andrey Shinkevich April 8, 2019, 3:29 p.m. UTC | #5 
On 08/04/2019 18:22, Alberto Garcia wrote:
> On Mon 08 Apr 2019 05:16:22 PM CEST, Andrey Shinkevich wrote:
>> On 08/04/2019 18:03, Alberto Garcia wrote:
>>> On Fri 05 Apr 2019 06:56:17 PM CEST, Andrey Shinkevich wrote:
>>>> +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);
>>>> +}
>>>
>>> Instead of having these two, isn't it simpler to add an 'include_base'
>>> parameter to the original function?
>>>
>>> Another alternative (I haven't checked this one so it could be more
>>> cumbersome): change the semantics of the function to always include the
>>> base and modify the callers.
>>
>> The idea behind those two functions was to keep the rest of the code
>> unmodified. Currently, we have the issue with the block-stream
>> parallel jobs. What if we manage this case first and then, when proved
>> to be robust, take care of the rest?
> 
> Sure, that makes sense if you need to do significant changes to the
> other callers, but if you just need to pass an extra 'false'
> parameter...
> 
> It's not a big deal, I just think you'd have a simpler patch.
> 
> Berto
> 

OK, I'll do that with the next v4.
diff mbox series

Patch

diff --git a/block/io.c b/block/io.c
index dfc153b..7e6019f 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,19 @@  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;
 
+    assert(base || !base_included);
+
     intermediate = top;
-    while (intermediate && intermediate != base) {
+    while (base_included || intermediate != base) {
         int64_t pnum_inter;
         int64_t size_inter;
 
@@ -2360,6 +2364,10 @@  int bdrv_is_allocated_above(BlockDriverState *top,
             n = pnum_inter;
         }
 
+        if (intermediate == base) {
+            break;
+        }
+
         intermediate = backing_bs(intermediate);
     }
 
@@ -2367,6 +2375,21 @@  int bdrv_is_allocated_above(BlockDriverState *top,
     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..be1e9a0 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -449,6 +449,10 @@  int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
 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);