[Qemu-devel] [PATCH v3 0/2] mirror: fix improperly filled copy_bitmap for mirror block job

[Qemu-devel] [PATCH v3 0/2] mirror: fix improperly filled copy_bitmap for mirror block job

[Denis V. Lunev]

bdrv_is_allocated_above() returns true in the case even for completel
zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both cases.

The patch stops using bdrv_is_allocated_above() wrapper and switches to
bdrv_get_block_status_above() to distinguish zeroed areas and areas with
data to avoid extra IO operations if possible.

Though this change requires some preparations in bdrv_get_block_status_above()
performed in the patch (1).

Changes from v2:
- reworked patch 1 to properly hide data below short image
- fixed comment in patch 1
- fixed mask assignment in patch 2 to cover bdrv_is_zero_initialized() case

Changes from v1:
- fixed assert in 041 test case (added patch 1)
- fixed commit message
- fixed status check to be on the safe side

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>

Denis V. Lunev (2):
  block: sync bdrv_co_get_block_status_above() with
    bdrv_is_allocated_above()
  mirror: fix improperly filled copy_bitmap for mirror block job

 block/io.c     | 25 ++++++++++++++++++++-----
 block/mirror.c | 17 +++++++++++------
 2 files changed, 31 insertions(+), 11 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v3 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()

[Denis V. Lunev]

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.

This change should be made very carefully. Let us assume that we have
top image and 2 backing stores L0->L1->L2.
  L0: --------------
  L1: -------
  L2: -------=======
The data marked as '=' in L2 should not appear as BDRV_BLOCK_ALLOCATED
and we should return it as filled in L0 image with properly calculated
*pnum value.

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 | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/block/io.c b/block/io.c
index 420944d..067d465 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1741,18 +1741,33 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
         BlockDriverState **file)
 {
     BlockDriverState *p;
-    int64_t ret = 0;
+    int64_t ret = 0, res = nb_sectors;
 
     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;
+        }
+
+        if (res > sc && (p == bs || sector_num + sc < p->total_sectors)) {
+            res = 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);
+        nb_sectors = MIN(nb_sectors, sc);
+
+        if (nb_sectors == 0) {
+            break;
+        }
     }
+
+    *pnum = res;
     return ret;
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v3 2/2] mirror: fix improperly filled copy_bitmap for mirror block job

[Denis V. Lunev]

bdrv_is_allocated_above() returns true in the case even for completel
zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both cases.

The patch stops using bdrv_is_allocated_above() wrapper and switches to
bdrv_get_block_status_above() to distinguish zeroed areas and areas with
data to avoid extra IO operations if possible.

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/mirror.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index e0b3f41..2710f62 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -548,11 +548,11 @@ static void mirror_throttle(MirrorBlockJob *s)
 
 static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 {
-    int64_t sector_num, end;
+    int64_t sector_num, end, alloc_mask;
     BlockDriverState *base = s->base;
     BlockDriverState *bs = blk_bs(s->common.blk);
     BlockDriverState *target_bs = blk_bs(s->target);
-    int ret, n;
+    int n;
 
     end = s->bdev_length / BDRV_SECTOR_SIZE;
 
@@ -585,11 +585,15 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
         mirror_drain(s);
     }
 
+    alloc_mask = base == NULL ? BDRV_BLOCK_ALLOCATED : BDRV_BLOCK_DATA;
+
     /* First part, loop on the sectors and initialize the dirty bitmap.  */
     for (sector_num = 0; sector_num < end; ) {
         /* Just to make sure we are not exceeding int limit. */
         int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
                              end - sector_num);
+        int64_t status;
+        BlockDriverState *file;
 
         mirror_throttle(s);
 
@@ -597,13 +601,14 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
             return 0;
         }
 
-        ret = bdrv_is_allocated_above(bs, base, sector_num, nb_sectors, &n);
-        if (ret < 0) {
-            return ret;
+        status = bdrv_get_block_status_above(bs, base, sector_num,
+                                             nb_sectors, &n, &file);
+        if (status < 0) {
+            return status;
         }
 
         assert(n > 0);
-        if (ret == 1) {
+        if (status & alloc_mask) {
             bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
         }
         sector_num += n;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 2/2] mirror: fix improperly filled copy_bitmap for mirror block job

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

On 09/15/2016 11:34 AM, Denis V. Lunev wrote:
> bdrv_is_allocated_above() returns true in the case even for completel

s/completel/completely/

> zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both cases.
> 
> The patch stops using bdrv_is_allocated_above() wrapper and switches to
> bdrv_get_block_status_above() to distinguish zeroed areas and areas with
> data to avoid extra IO operations if possible.
> 
> 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/mirror.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()

[Fam Zheng]

On Thu, 09/15 19:34, 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.
> 
> This change should be made very carefully. Let us assume that we have
> top image and 2 backing stores L0->L1->L2.

Stupid question: which one is top and which are backing?

>   L0: --------------
>   L1: -------
>   L2: -------=======
> The data marked as '=' in L2 should not appear as BDRV_BLOCK_ALLOCATED
> and we should return it as filled in L0 image with properly calculated
> *pnum value.

What '-', '=' and ' ' represent aren't immediately clear to me, could you put a
legend in the message too? Something like:

    '-': allocated
    '=': unallocated
    ' ': beyong EOF

> 
> 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 | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 420944d..067d465 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1741,18 +1741,33 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
>          BlockDriverState **file)
>  {
>      BlockDriverState *p;
> -    int64_t ret = 0;
> +    int64_t ret = 0, res = nb_sectors;
>  
>      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;
> +        }
> +
> +        if (res > sc && (p == bs || sector_num + sc < p->total_sectors)) {
> +            res = 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);
> +        nb_sectors = MIN(nb_sectors, sc);
> +
> +        if (nb_sectors == 0) {
> +            break;
> +        }
>      }
> +
> +    *pnum = res;
>      return ret;
>  }
>  
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH v3 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()

[Denis V. Lunev]

On 09/19/2016 04:21 AM, Fam Zheng wrote:
> On Thu, 09/15 19:34, 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.
>>
>> This change should be made very carefully. Let us assume that we have
>> top image and 2 backing stores L0->L1->L2.
> Stupid question: which one is top and which are backing?
L0 is top, L2 is at bottom.


>>   L0: --------------
>>   L1: -------
>>   L2: -------=======
>> The data marked as '=' in L2 should not appear as BDRV_BLOCK_ALLOCATED
>> and we should return it as filled in L0 image with properly calculated
>> *pnum value.
> What '-', '=' and ' ' represent aren't immediately clear to me, could you put a
> legend in the message too? Something like:
>
>     '-': allocated
>     '=': unallocated
>     ' ': beyong EOF
ok.

here '-' in unallocated
'=' is allocated.
virtual size of L1 image is shorter that L2 and L0, thus ' ' is beyond EOF.

Thank you, will rewrite today.

Den
>> 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 | 25 ++++++++++++++++++++-----
>>  1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 420944d..067d465 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -1741,18 +1741,33 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
>>          BlockDriverState **file)
>>  {
>>      BlockDriverState *p;
>> -    int64_t ret = 0;
>> +    int64_t ret = 0, res = nb_sectors;
>>  
>>      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;
>> +        }
>> +
>> +        if (res > sc && (p == bs || sector_num + sc < p->total_sectors)) {
>> +            res = 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);
>> +        nb_sectors = MIN(nb_sectors, sc);
>> +
>> +        if (nb_sectors == 0) {
>> +            break;
>> +        }
>>      }
>> +
>> +    *pnum = res;
>>      return ret;
>>  }
>>  
>> -- 
>> 2.7.4
>>
>>

Re: [Qemu-devel] [PATCH v3 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

On 09/18/2016 11:37 PM, Denis V. Lunev wrote:
> On 09/19/2016 04:21 AM, Fam Zheng wrote:
>> On Thu, 09/15 19:34, 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.
>>>
>>> This change should be made very carefully. Let us assume that we have
>>> top image and 2 backing stores L0->L1->L2.
>> Stupid question: which one is top and which are backing?
> L0 is top, L2 is at bottom.

I typically write this as:

L2 <- L1 <- L0

(read "L2 backs L1, which in turn backs L0") with the active on the
right.  So I understand the confusion in Fam's question where you were
using the opposite direction.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()

[Max Reitz]

On 2016-09-15 at 18:34, 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.
>
> This change should be made very carefully. Let us assume that we have
> top image and 2 backing stores L0->L1->L2.
>   L0: --------------
>   L1: -------
>   L2: -------=======
> The data marked as '=' in L2 should not appear as BDRV_BLOCK_ALLOCATED
> and we should return it as filled in L0 image with properly calculated
> *pnum value.
>
> 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 | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 420944d..067d465 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1741,18 +1741,33 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
>          BlockDriverState **file)
>  {
>      BlockDriverState *p;
> -    int64_t ret = 0;
> +    int64_t ret = 0, res = nb_sectors;

It's not wrong to make res an int64_t, but an int is sufficient.

>
>      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;
> +        }
> +
> +        if (res > sc && (p == bs || sector_num + sc < p->total_sectors)) {
> +            res = sc;

This definitely requires some comments because it took me a long time to 
figure out why we need "res" to be a separate variable from "nb_sectors" 
and why this condition is like it is.

So what I think this does is:

Basically, we want to return our final nb_sectors in *pnum. But we can 
have the constellation you noted in the commit message: A short 
intermediate layer, and the bottom layer has some data allocated beyond 
the end of that intermediate layer.

Now, when we pass through that intermediate layer, we need to shorten 
nb_sectors so that we don't query anything beyond the end of that 
intermediate layer because it doesn't matter anyway.

But we also want to remember that all of this area appears as 
unallocated to the top layer, so therefore we have to keep a second 
variable ("res") which retains this information.

Therefore, nb_sectors is always exactly the range we want to query, and 
"res" is the range we know to appear unallocated. This condition here 
tries to adjust "res" so that it conforms to that specification.

However, I'm not quite sure it actually does that. Let's take the case 
from your commit message:

L0: --------------
L1: -------
L2: -------=======

Let's say we invoke this function in the range [0, 14]. After passing 
through L0, res is 14 and nb_sectors is 14. After L1, res is still 14, 
but nb_sectors is 7. So far, so good.

But when passing through L2, "sc" will be 7 (and it will actually always 
be 7, regardless of what comes past sector 7, because nb_sectors is 7). 
Since L2 is larger than just 7 sectors, we will now reduce res to 7 as 
well (because sector_num + sc (= 0 + 7 = 7) < p->total_sectors (= 14)).

So therefore, we will set *pnum to 7. That doesn't seem too bad to me, 
but we could have achieved the same result by just setting *pnum to 
nb_sectors and not having to track the separate "res" variable.


Thus, I'm not quite sure what the point of this is. "res" will only be 
longer than "nb_sectors" as long as the layers get shorter or stay the 
same length when going downwards. As soon as one layer is longer than 
the one above it, "res" will probably be truncated to "sc" (which is 
going to be the same value as "nb_sectors", unless 
bdrv_co_get_block_status() returns a *pnum > nb_sectors).

I'm not sure whether I'm missing something here, though.

>          }
> +
>          /* [sector_num, pnum] unallocated on this layer, which could be only

The "pnum" here should be changed to "sc".

>           * the first part of [sector_num, nb_sectors].  */
> -        nb_sectors = MIN(nb_sectors, *pnum);
> +        nb_sectors = MIN(nb_sectors, sc);
> +
> +        if (nb_sectors == 0) {
> +            break;

While I can see that in this case ret would be 0, I think it wouldn't 
hurt to add an explicit "ret = 0;" here, too.

Max

> +        }
>      }
> +
> +    *pnum = res;
>      return ret;
>  }
>
>

Re: [Qemu-devel] [PATCH v3 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()

[Jeff Cody]

On Tue, Sep 20, 2016 at 01:18:12AM +0200, Max Reitz wrote:
> On 2016-09-15 at 18:34, 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.
> >
> >This change should be made very carefully. Let us assume that we have
> >top image and 2 backing stores L0->L1->L2.
> >  L0: --------------
> >  L1: -------
> >  L2: -------=======
> >The data marked as '=' in L2 should not appear as BDRV_BLOCK_ALLOCATED
> >and we should return it as filled in L0 image with properly calculated
> >*pnum value.
> >
> >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 | 25 ++++++++++++++++++++-----
> > 1 file changed, 20 insertions(+), 5 deletions(-)
> >
> >diff --git a/block/io.c b/block/io.c
> >index 420944d..067d465 100644
> >--- a/block/io.c
> >+++ b/block/io.c
> >@@ -1741,18 +1741,33 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
> >         BlockDriverState **file)
> > {
> >     BlockDriverState *p;
> >-    int64_t ret = 0;
> >+    int64_t ret = 0, res = nb_sectors;
> 
> It's not wrong to make res an int64_t, but an int is sufficient.
> 
> >
> >     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;
> >+        }
> >+
> >+        if (res > sc && (p == bs || sector_num + sc < p->total_sectors)) {
> >+            res = sc;

For what its worth, I think the code in bdrv_is_allocated_above() is a bit
more readable, which does largely the same thing as this function now does
with this patch.

Some of it is due to with the nomenclature of 'res' and 'sc' here, neither
of which are very intuitive.  Part of it is out of this patch's control: 
the explicit variable convention of "top", "intermediate", and "base" in the
while loop of bdrv_is_allocated_above() also aids readability.

I think since the functionality is largely pulled from
bdrv_is_allocated_above(), it would make sense to copy its syntax more
directly.

> 
> This definitely requires some comments because it took me a long time to
> figure out why we need "res" to be a separate variable from "nb_sectors" and
> why this condition is like it is.
> 
> So what I think this does is:
> 
> Basically, we want to return our final nb_sectors in *pnum. But we can have
> the constellation you noted in the commit message: A short intermediate
> layer, and the bottom layer has some data allocated beyond the end of that
> intermediate layer.
> 
> Now, when we pass through that intermediate layer, we need to shorten
> nb_sectors so that we don't query anything beyond the end of that
> intermediate layer because it doesn't matter anyway.
> 

That's the issue with this patch; we don't need to shorten nb_sectors,
precisely because it doesn't matter if it extends past the end of the
current layer. [1]


> But we also want to remember that all of this area appears as unallocated to
> the top layer, so therefore we have to keep a second variable ("res") which
> retains this information.
> 
> Therefore, nb_sectors is always exactly the range we want to query, and
> "res" is the range we know to appear unallocated. This condition here tries
> to adjust "res" so that it conforms to that specification.
>

This changes makes this function similar to "bdrv_is_allocated_above()".  It
may be useful to refer back to commit 63ba17d, to see the rationale when
that functioned got the similar functionality:

    block: Fix is_allocated_above with resized files

    In an image chain, if the base image is smaller than the current
    image, we need to make sure to use the current images count of
    unallocated blocks once we get to the end of the base image. Without
    this change the code will return 0 blocks when it gets to the end
    of the base image and mirror_run will fail its assertion.


This sounds compatible with your above analysis.

 
> However, I'm not quite sure it actually does that. Let's take the case from
> your commit message:
> 
> L0: --------------
> L1: -------
> L2: -------=======
> 
> Let's say we invoke this function in the range [0, 14]. After passing
> through L0, res is 14 and nb_sectors is 14. After L1, res is still 14, but
> nb_sectors is 7. So far, so good.
> 
> But when passing through L2, "sc" will be 7 (and it will actually always be
> 7, regardless of what comes past sector 7, because nb_sectors is 7). Since
> L2 is larger than just 7 sectors, we will now reduce res to 7 as well
> (because sector_num + sc (= 0 + 7 = 7) < p->total_sectors (= 14)).
> 
> So therefore, we will set *pnum to 7. That doesn't seem too bad to me, but
> we could have achieved the same result by just setting *pnum to nb_sectors
> and not having to track the separate "res" variable.
> 
> 
> Thus, I'm not quite sure what the point of this is. "res" will only be
> longer than "nb_sectors" as long as the layers get shorter or stay the same
> length when going downwards. As soon as one layer is longer than the one
> above it, "res" will probably be truncated to "sc" (which is going to be the
> same value as "nb_sectors", unless bdrv_co_get_block_status() returns a
> *pnum > nb_sectors).
> 
> I'm not sure whether I'm missing something here, though.
>

There is a bug, below [1]:

> >         }
> >+
> >         /* [sector_num, pnum] unallocated on this layer, which could be only
>
> The "pnum" here should be changed to "sc".
> 
> >          * the first part of [sector_num, nb_sectors].  */
> >-        nb_sectors = MIN(nb_sectors, *pnum);
> >+        nb_sectors = MIN(nb_sectors, sc);

[1]

The above code should be deleted completely. There isn't a reason to set
nb_sectors to the lowest value here - we should always be passing in the
original nb_sectors value (the underlying driver will truncate the returned
pnum value as appropriate).

> >+
> >+        if (nb_sectors == 0) {
> >+            break;

This can go as well.

-Jeff

> 
> While I can see that in this case ret would be 0, I think it wouldn't hurt
> to add an explicit "ret = 0;" here, too.
> 
> Max
> 
> >+        }
> >     }
> >+
> >+    *pnum = res;
> >     return ret;
> > }
> >
> >

Re: [Qemu-devel] [PATCH v3 2/2] mirror: fix improperly filled copy_bitmap for mirror block job

[Vladimir Sementsov-Ogievskiy]

On 15.09.2016 19:34, Denis V. Lunev wrote:
> bdrv_is_allocated_above() returns true in the case even for completel
> zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both cases.
>
> The patch stops using bdrv_is_allocated_above() wrapper and switches to
> bdrv_get_block_status_above() to distinguish zeroed areas and areas with
> data to avoid extra IO operations if possible.
>
> 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/mirror.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index e0b3f41..2710f62 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -548,11 +548,11 @@ static void mirror_throttle(MirrorBlockJob *s)
>   
>   static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>   {
> -    int64_t sector_num, end;
> +    int64_t sector_num, end, alloc_mask;
>       BlockDriverState *base = s->base;
>       BlockDriverState *bs = blk_bs(s->common.blk);
>       BlockDriverState *target_bs = blk_bs(s->target);
> -    int ret, n;
> +    int n;
>   
>       end = s->bdev_length / BDRV_SECTOR_SIZE;
>   
> @@ -585,11 +585,15 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>           mirror_drain(s);
>       }
>   
> +    alloc_mask = base == NULL ? BDRV_BLOCK_ALLOCATED : BDRV_BLOCK_DATA;

should not s/==/!= ?

> +
>       /* First part, loop on the sectors and initialize the dirty bitmap.  */
>       for (sector_num = 0; sector_num < end; ) {
>           /* Just to make sure we are not exceeding int limit. */
>           int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
>                                end - sector_num);
> +        int64_t status;
> +        BlockDriverState *file;
>   
>           mirror_throttle(s);
>   
> @@ -597,13 +601,14 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>               return 0;
>           }
>   
> -        ret = bdrv_is_allocated_above(bs, base, sector_num, nb_sectors, &n);
> -        if (ret < 0) {
> -            return ret;
> +        status = bdrv_get_block_status_above(bs, base, sector_num,
> +                                             nb_sectors, &n, &file);
> +        if (status < 0) {
> +            return status;
>           }
>   
>           assert(n > 0);
> -        if (ret == 1) {
> +        if (status & alloc_mask) {
>               bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
>           }
>           sector_num += n;


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

Am 19.09.2016 um 22:39 hat Eric Blake geschrieben:
> On 09/18/2016 11:37 PM, Denis V. Lunev wrote:
> > On 09/19/2016 04:21 AM, Fam Zheng wrote:
> >> On Thu, 09/15 19:34, 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.
> >>>
> >>> This change should be made very carefully. Let us assume that we have
> >>> top image and 2 backing stores L0->L1->L2.
> >> Stupid question: which one is top and which are backing?
> > L0 is top, L2 is at bottom.
> 
> I typically write this as:
> 
> L2 <- L1 <- L0
> 
> (read "L2 backs L1, which in turn backs L0") with the active on the
> right.  So I understand the confusion in Fam's question where you were
> using the opposite direction.

And I tend to use this one:

    base <- sn1 <- sn2 <- top

"sn*" isn't any better than "L*", but having at least one of "base" and
"top" (or "active") in there disambiguates the roles of the nodes.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()

[Kashyap Chamarthy]

On Mon, Sep 26, 2016 at 05:04:21PM +0200, Kevin Wolf wrote:
> Am 19.09.2016 um 22:39 hat Eric Blake geschrieben:

[...]

> > I typically write this as:
> > 
> > L2 <- L1 <- L0
> > 
> > (read "L2 backs L1, which in turn backs L0") with the active on the
> > right.  So I understand the confusion in Fam's question where you were
> > using the opposite direction.
> 
> And I tend to use this one:
> 
>     base <- sn1 <- sn2 <- top
> 
> "sn*" isn't any better than "L*", but having at least one of "base" and
> "top" (or "active") in there disambiguates the roles of the nodes.

Not to quibble over terminology too much, but now that I'm writing a doc
that I want to submit upstream, I began with your (Kevin's) notation.
Then, I thought: Hmm, "sn1" could also be referred to as 'base', and
"sn2" as 'top' when using `block-commit`' (and `block-stream`, once it
starts supporting intermediate streaming?).

And, moreover, as Eric (correctly) warns elsewhere about file-names vs.
points-in-time: the guest state when 'sn1' was created is contained in
'base', so one could argue that 'sn1' ("snapshot 1") is a misnomer, and
is technically 'overlay1'.

So, I used the below notation until recently, including 'active'
(with the rationale Kevin mentioned):

    base <- overlay1 <- overlay2  <- active

Then, someone asked: "In the above chain, are you pointing to 'overlay2'
as active, or is 'active' a separate image unto itself"?  "Sigh, so it
is still prone to misunderstanding", I thought.

Given that, for now, though slightly more verbose and space-occupying, I
settled on the below (occasionally doing s/base/orig/, to avoid the
"overlay1 could be referred to as 'base' in some cases" problem):

                                      
                                    Live QEMU
                                       |
                                       v
    base <- overlay1 <- overlay2 <- overlay3

FWIW, the above also avoids the problem of a file called 'active' being
described as: "previously-active, but not anymore, because its contents
are merged into its backing file" in the event of a 'block-commit'.

</end quibble>

-- 
/kashyap