[Qemu-devel] [PATCH] block: Update image size in bdrv_invalidate_cache()

[Qemu-devel] [PATCH] block: Update image size in bdrv_invalidate_cache()

[Kevin Wolf]

After migration has completed, we call bdrv_invalidate_cache() so that
drivers which cache some data drop their stale copy of the data and
reread it from the image file to get a new version of data that the
source modified while the migration was running.

Reloading metadata from the image file is useless, though, if the size
of the image file stays stale (this is a value that is cached for all
image formats in block.c). Reads from (meta)data after the old EOF
return only zeroes, causing image corruption.

We need to update bs->total_sectors in all layers that could potentially
have changed their size (i.e. backing files are not a concern - if they
are changed, we're in bigger trouble)

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c       | 10 +++++++++-
 block/qcow2.c |  2 ++
 block/qed.c   |  3 +++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index f1ef4b0..7b306fb 100644
--- a/block.c
+++ b/block.c
@@ -4776,9 +4776,17 @@ flush_parent:
 
 void bdrv_invalidate_cache(BlockDriverState *bs)
 {
-    if (bs->drv && bs->drv->bdrv_invalidate_cache) {
+    if (!bs->drv)  {
+        return;
+    }
+
+    if (bs->drv->bdrv_invalidate_cache) {
         bs->drv->bdrv_invalidate_cache(bs);
+    } else if (bs->file) {
+        bdrv_invalidate_cache(bs->file);
     }
+
+    refresh_total_sectors(bs, bs->total_sectors);
 }
 
 void bdrv_invalidate_cache_all(void)
diff --git a/block/qcow2.c b/block/qcow2.c
index cfe80be..b5b1e8c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1176,6 +1176,8 @@ static void qcow2_invalidate_cache(BlockDriverState *bs)
 
     qcow2_close(bs);
 
+    bdrv_invalidate_cache(bs->file);
+
     options = qdict_new();
     qdict_put(options, QCOW2_OPT_LAZY_REFCOUNTS,
               qbool_from_int(s->use_lazy_refcounts));
diff --git a/block/qed.c b/block/qed.c
index 8802ad3..837accd 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1563,6 +1563,9 @@ static void bdrv_qed_invalidate_cache(BlockDriverState *bs)
     BDRVQEDState *s = bs->opaque;
 
     bdrv_qed_close(bs);
+
+    bdrv_invalidate_cache(bs->file);
+
     memset(s, 0, sizeof(BDRVQEDState));
     bdrv_qed_open(bs, NULL, bs->open_flags, NULL);
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] block: Update image size in bdrv_invalidate_cache()

[Juan Quintela]

Kevin Wolf <kwolf@redhat.com> wrote:
> After migration has completed, we call bdrv_invalidate_cache() so that
> drivers which cache some data drop their stale copy of the data and
> reread it from the image file to get a new version of data that the
> source modified while the migration was running.
>
> Reloading metadata from the image file is useless, though, if the size
> of the image file stays stale (this is a value that is cached for all
> image formats in block.c). Reads from (meta)data after the old EOF
> return only zeroes, causing image corruption.
>
> We need to update bs->total_sectors in all layers that could potentially
> have changed their size (i.e. backing files are not a concern - if they
> are changed, we're in bigger trouble)
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>
Tested-by: Juan Quintela <quintela@redhat.com>

I had problems with migration with qcow2 files, and this fixes it.

Re: [Qemu-devel] [PATCH] block: Update image size in bdrv_invalidate_cache()

[Stefan Hajnoczi]

On Tue, Mar 11, 2014 at 10:58:39AM +0100, Kevin Wolf wrote:
> After migration has completed, we call bdrv_invalidate_cache() so that
> drivers which cache some data drop their stale copy of the data and
> reread it from the image file to get a new version of data that the
> source modified while the migration was running.
> 
> Reloading metadata from the image file is useless, though, if the size
> of the image file stays stale (this is a value that is cached for all
> image formats in block.c). Reads from (meta)data after the old EOF
> return only zeroes, causing image corruption.
> 
> We need to update bs->total_sectors in all layers that could potentially
> have changed their size (i.e. backing files are not a concern - if they
> are changed, we're in bigger trouble)
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c       | 10 +++++++++-
>  block/qcow2.c |  2 ++
>  block/qed.c   |  3 +++
>  3 files changed, 14 insertions(+), 1 deletion(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

Re: [Qemu-devel] [PATCH] block: Update image size in bdrv_invalidate_cache()

[Benoît Canet]

The Tuesday 11 Mar 2014 à 10:58:39 (+0100), Kevin Wolf wrote :
> After migration has completed, we call bdrv_invalidate_cache() so that
> drivers which cache some data drop their stale copy of the data and
> reread it from the image file to get a new version of data that the
> source modified while the migration was running.
> 
> Reloading metadata from the image file is useless, though, if the size
> of the image file stays stale (this is a value that is cached for all
> image formats in block.c). Reads from (meta)data after the old EOF
> return only zeroes, causing image corruption.
> 
> We need to update bs->total_sectors in all layers that could potentially
> have changed their size (i.e. backing files are not a concern - if they
> are changed, we're in bigger trouble)
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c       | 10 +++++++++-
>  block/qcow2.c |  2 ++
>  block/qed.c   |  3 +++
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index f1ef4b0..7b306fb 100644
> --- a/block.c
> +++ b/block.c
> @@ -4776,9 +4776,17 @@ flush_parent:
>  
>  void bdrv_invalidate_cache(BlockDriverState *bs)
>  {
> -    if (bs->drv && bs->drv->bdrv_invalidate_cache) {
> +    if (!bs->drv)  {
> +        return;
> +    }
> +
> +    if (bs->drv->bdrv_invalidate_cache) {
>          bs->drv->bdrv_invalidate_cache(bs);
> +    } else if (bs->file) {
> +        bdrv_invalidate_cache(bs->file);
>      }
> +
> +    refresh_total_sectors(bs, bs->total_sectors);
>  }
>  
>  void bdrv_invalidate_cache_all(void)
> diff --git a/block/qcow2.c b/block/qcow2.c
> index cfe80be..b5b1e8c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1176,6 +1176,8 @@ static void qcow2_invalidate_cache(BlockDriverState *bs)
>  
>      qcow2_close(bs);
>  
> +    bdrv_invalidate_cache(bs->file);
> +
>      options = qdict_new();
>      qdict_put(options, QCOW2_OPT_LAZY_REFCOUNTS,
>                qbool_from_int(s->use_lazy_refcounts));
> diff --git a/block/qed.c b/block/qed.c
> index 8802ad3..837accd 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -1563,6 +1563,9 @@ static void bdrv_qed_invalidate_cache(BlockDriverState *bs)
>      BDRVQEDState *s = bs->opaque;
>  
>      bdrv_qed_close(bs);
> +
> +    bdrv_invalidate_cache(bs->file);
> +
>      memset(s, 0, sizeof(BDRVQEDState));
>      bdrv_qed_open(bs, NULL, bs->open_flags, NULL);
>  }
> -- 
> 1.8.1.4
> 
> 

I have the impression that you are silently fixing other nits;

However:

Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH] block: Update image size in bdrv_invalidate_cache()

[Kevin Wolf]

Am 11.03.2014 um 17:41 hat Benoît Canet geschrieben:
> I have the impression that you are silently fixing other nits;

Am I?

Not intentionally at least. I think I can justify every single line of
the patch with respect to the bug described in the commit message. We
need to recursive into the protocol layer, and we need to refresh
bs->total_sectors. For drivers implementing the callback, we need to
recurse to bs->file between the internal open/close pair.

I can't see what could be left out in this patch.

Kevin

Re: [Qemu-devel] [PATCH] block: Update image size in bdrv_invalidate_cache()

[Benoît Canet]

The Wednesday 12 Mar 2014 à 10:40:00 (+0100), Kevin Wolf wrote :
> Am 11.03.2014 um 17:41 hat Benoît Canet geschrieben:
> > I have the impression that you are silently fixing other nits;
> 
> Am I?
> 
> Not intentionally at least. I think I can justify every single line of
> the patch with respect to the bug described in the commit message. We
> need to recursive into the protocol layer, and we need to refresh
> bs->total_sectors. For drivers implementing the callback, we need to
> recurse to bs->file between the internal open/close pair.
> 
> I can't see what could be left out in this patch.

I didn't understood the implication on recursion at first sight.

Best regards

Benoît

> 
> Kevin
>