[Qemu-devel] [PATCH] block/quorum: implement .bdrv_co_get_block_status

[Qemu-devel] [PATCH] block/quorum: implement .bdrv_co_get_block_status

[Liu Yuan]

- allow drive-mirror to create sprase mirror on images like qcow2
- allow qemu-img map to work as expected on quorum driver

Cc: Benoit Canet <benoit@irqsave.net>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Liu Yuan <namei.unix@gmail.com>
---
 block/quorum.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/block/quorum.c b/block/quorum.c
index ebf5c71..f0d0a98 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -780,6 +780,21 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
     return result;
 }
 
+static int64_t coroutine_fn quorum_co_get_block_status(BlockDriverState *bs,
+                                                       int64_t sector_num,
+                                                       int nb_sectors,
+                                                       int *pnum)
+{
+    BDRVQuorumState *s = bs->opaque;
+    BlockDriverState *child_bs = s->bs[0];
+
+    if (child_bs->drv->bdrv_co_get_block_status)
+        return child_bs->drv->bdrv_co_get_block_status(child_bs, sector_num,
+                                                       nb_sectors, pnum);
+
+    return bdrv_get_block_status(child_bs, sector_num, nb_sectors, pnum);
+}
+
 static bool quorum_recurse_is_first_non_filter(BlockDriverState *bs,
                                                BlockDriverState *candidate)
 {
@@ -1038,6 +1053,7 @@ static BlockDriver bdrv_quorum = {
     .bdrv_close                         = quorum_close,
 
     .bdrv_co_flush_to_disk              = quorum_co_flush,
+    .bdrv_co_get_block_status           = quorum_co_get_block_status,
 
     .bdrv_getlength                     = quorum_getlength,
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH] block/quorum: implement .bdrv_co_get_block_status

[Fam Zheng]

On Thu, 07/17 19:50, Liu Yuan wrote:
> - allow drive-mirror to create sprase mirror on images like qcow2
> - allow qemu-img map to work as expected on quorum driver
> 
> Cc: Benoit Canet <benoit@irqsave.net>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Liu Yuan <namei.unix@gmail.com>
> ---
>  block/quorum.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index ebf5c71..f0d0a98 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -780,6 +780,21 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
>      return result;
>  }
>  
> +static int64_t coroutine_fn quorum_co_get_block_status(BlockDriverState *bs,
> +                                                       int64_t sector_num,
> +                                                       int nb_sectors,
> +                                                       int *pnum)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    BlockDriverState *child_bs = s->bs[0];

Should we consider other children?

Fam

> +
> +    if (child_bs->drv->bdrv_co_get_block_status)
> +        return child_bs->drv->bdrv_co_get_block_status(child_bs, sector_num,
> +                                                       nb_sectors, pnum);
> +
> +    return bdrv_get_block_status(child_bs, sector_num, nb_sectors, pnum);
> +}
> +

Re: [Qemu-devel] [PATCH] block/quorum: implement .bdrv_co_get_block_status

[Liu Yuan]

On Fri, Jul 18, 2014 at 11:11:31AM +0800, Fam Zheng wrote:
> On Thu, 07/17 19:50, Liu Yuan wrote:
> > - allow drive-mirror to create sprase mirror on images like qcow2
> > - allow qemu-img map to work as expected on quorum driver
> > 
> > Cc: Benoit Canet <benoit@irqsave.net>
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Liu Yuan <namei.unix@gmail.com>
> > ---
> >  block/quorum.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/block/quorum.c b/block/quorum.c
> > index ebf5c71..f0d0a98 100644
> > --- a/block/quorum.c
> > +++ b/block/quorum.c
> > @@ -780,6 +780,21 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
> >      return result;
> >  }
> >  
> > +static int64_t coroutine_fn quorum_co_get_block_status(BlockDriverState *bs,
> > +                                                       int64_t sector_num,
> > +                                                       int nb_sectors,
> > +                                                       int *pnum)
> > +{
> > +    BDRVQuorumState *s = bs->opaque;
> > +    BlockDriverState *child_bs = s->bs[0];
> 
> Should we consider other children?
> 

No, other children are identical because we always do multiple sync writes
and at boot time we check the length.

Thanks
Yuan

Re: [Qemu-devel] [PATCH] block/quorum: implement .bdrv_co_get_block_status

[Kevin Wolf]

Am 18.07.2014 um 07:52 hat Liu Yuan geschrieben:
> On Fri, Jul 18, 2014 at 11:11:31AM +0800, Fam Zheng wrote:
> > On Thu, 07/17 19:50, Liu Yuan wrote:
> > > - allow drive-mirror to create sprase mirror on images like qcow2
> > > - allow qemu-img map to work as expected on quorum driver
> > > 
> > > Cc: Benoit Canet <benoit@irqsave.net>
> > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > Signed-off-by: Liu Yuan <namei.unix@gmail.com>
> > > ---
> > >  block/quorum.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/block/quorum.c b/block/quorum.c
> > > index ebf5c71..f0d0a98 100644
> > > --- a/block/quorum.c
> > > +++ b/block/quorum.c
> > > @@ -780,6 +780,21 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
> > >      return result;
> > >  }
> > >  
> > > +static int64_t coroutine_fn quorum_co_get_block_status(BlockDriverState *bs,
> > > +                                                       int64_t sector_num,
> > > +                                                       int nb_sectors,
> > > +                                                       int *pnum)
> > > +{
> > > +    BDRVQuorumState *s = bs->opaque;
> > > +    BlockDriverState *child_bs = s->bs[0];
> > 
> > Should we consider other children?
> > 
> 
> No, other children are identical because we always do multiple sync writes
> and at boot time we check the length.

But the basic assumption in quorum is that a single child isn't reliable
and can fail. Otherwise, you wouldn't have to read from all of them and
compare the results.

Kevin

Re: [Qemu-devel] [PATCH] block/quorum: implement .bdrv_co_get_block_status

[Eric Blake]

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

On 07/17/2014 09:11 PM, Fam Zheng wrote:
> On Thu, 07/17 19:50, Liu Yuan wrote:
>> - allow drive-mirror to create sprase mirror on images like qcow2
>> - allow qemu-img map to work as expected on quorum driver
>>

>>  
>> +static int64_t coroutine_fn quorum_co_get_block_status(BlockDriverState *bs,
>> +                                                       int64_t sector_num,
>> +                                                       int nb_sectors,
>> +                                                       int *pnum)
>> +{
>> +    BDRVQuorumState *s = bs->opaque;
>> +    BlockDriverState *child_bs = s->bs[0];
> 
> Should we consider other children?

Yes. Just because the children are written identically from the guest
point of view does not mean they start identically mapped, only that
they start with identical guest-visible contents.  In particular, a
quorum is allowed to be composed of disparate children (I could have a
quorum between a raw, a qcow2 v0.10, and a qcow2 v1.1 - and those three
formats have DIFFERENT notions of whether block_status can report
sparse, even for the same guest contents), or even where one child is a
chain of images while the other is flat.

For this function, I'm not sure you need a full quorum to declare a
block sparse, but you definitely need to consider that the first child
might not report sparse while later children do.  In other words, you
need to come up with some sane pattern that collapses information from
all children into a reliable answer for the quorum as a whole (probably
okay if any one child declares a sector as sparse to say the quorum is
sparse).  Likewise, you cannot assume that all the children have the
same number of consecutive sectors in the same allocated condition; even
though you can still come up with a reliable number for the quorum
(probably the minimum of the numbers reported by each child).

-- 
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] block/quorum: implement .bdrv_co_get_block_status

[Paolo Bonzini]

Il 17/07/2014 13:50, Liu Yuan ha scritto:
> - allow drive-mirror to create sprase mirror on images like qcow2
> - allow qemu-img map to work as expected on quorum driver
> 
> Cc: Benoit Canet <benoit@irqsave.net>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Liu Yuan <namei.unix@gmail.com>
> ---
>  block/quorum.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index ebf5c71..f0d0a98 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -780,6 +780,21 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
>      return result;
>  }
>  
> +static int64_t coroutine_fn quorum_co_get_block_status(BlockDriverState *bs,
> +                                                       int64_t sector_num,
> +                                                       int nb_sectors,
> +                                                       int *pnum)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    BlockDriverState *child_bs = s->bs[0];
> +
> +    if (child_bs->drv->bdrv_co_get_block_status)
> +        return child_bs->drv->bdrv_co_get_block_status(child_bs, sector_num,
> +                                                       nb_sectors, pnum);

Is this "if" necessary?

> +    return bdrv_get_block_status(child_bs, sector_num, nb_sectors, pnum);

Also, the definition of BDRV_BLOCK_OFFSET_VALID explicitly refers to
bs->file, so you probably have to exclude it from the result as well as
BDRV_BLOCK_RAW.

Paolo

> +
>  static bool quorum_recurse_is_first_non_filter(BlockDriverState *bs,
>                                                 BlockDriverState *candidate)
>  {
> @@ -1038,6 +1053,7 @@ static BlockDriver bdrv_quorum = {
>      .bdrv_close                         = quorum_close,
>  
>      .bdrv_co_flush_to_disk              = quorum_co_flush,
> +    .bdrv_co_get_block_status           = quorum_co_get_block_status,
>  
>      .bdrv_getlength                     = quorum_getlength,
>  
> 

Re: [Qemu-devel] [PATCH] block/quorum: implement .bdrv_co_get_block_status

[Liu Yuan]

On Fri, Jul 18, 2014 at 03:35:57PM +0200, Paolo Bonzini wrote:
> Il 17/07/2014 13:50, Liu Yuan ha scritto:
> > - allow drive-mirror to create sprase mirror on images like qcow2
> > - allow qemu-img map to work as expected on quorum driver
> > 
> > Cc: Benoit Canet <benoit@irqsave.net>
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Liu Yuan <namei.unix@gmail.com>
> > ---
> >  block/quorum.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/block/quorum.c b/block/quorum.c
> > index ebf5c71..f0d0a98 100644
> > --- a/block/quorum.c
> > +++ b/block/quorum.c
> > @@ -780,6 +780,21 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
> >      return result;
> >  }
> >  
> > +static int64_t coroutine_fn quorum_co_get_block_status(BlockDriverState *bs,
> > +                                                       int64_t sector_num,
> > +                                                       int nb_sectors,
> > +                                                       int *pnum)
> > +{
> > +    BDRVQuorumState *s = bs->opaque;
> > +    BlockDriverState *child_bs = s->bs[0];
> > +
> > +    if (child_bs->drv->bdrv_co_get_block_status)
> > +        return child_bs->drv->bdrv_co_get_block_status(child_bs, sector_num,
> > +                                                       nb_sectors, pnum);
> 
> Is this "if" necessary?

Yes, otherwise bdrv_get_block_status() will be called multiple times and the
result for qcow2 won't return desired value im my test. Or we can simply call
bdrv_get_block_status() plus some tricks?

> 
> > +    return bdrv_get_block_status(child_bs, sector_num, nb_sectors, pnum);
> 
> Also, the definition of BDRV_BLOCK_OFFSET_VALID explicitly refers to
> bs->file, so you probably have to exclude it from the result as well as
> BDRV_BLOCK_RAW.
> 

Thanks for reminding.

Yuan