Re: [Qemu-block] [Qemu-devel] [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'.



-- 
/kashyap

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

[Kevin Wolf]

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


pgp8pg5gbJmqB.pgp
Description: PGP signature

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

[Eric Blake]

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [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 
>> CC: Stefan Hajnoczi 
>> CC: Fam Zheng 
>> CC: Kevin Wolf 
>> CC: Max Reitz 
>> CC: Jeff Cody 
>> ---
>>  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, , 
>> 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-block] [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 
> CC: Stefan Hajnoczi 
> CC: Fam Zheng 
> CC: Kevin Wolf 
> CC: Max Reitz 
> CC: Jeff Cody 
> ---
>  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, , 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
> 
>