Patchwork [V2,4/7] qed: add bdrv_invalidate_cache to be called after incoming live migration

login
register
mail settings
Submitter Benoit Canet
Date March 21, 2012, 3:52 p.m.
Message ID <1332345124-381-5-git-send-email-benoit.canet@gmail.com>
Download mbox | patch
Permalink /patch/148039/
State New
Headers show

Comments

Benoit Canet - March 21, 2012, 3:52 p.m.
The QED image is reopened to flush metadata and check consistency.

Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
---
 block/qed.c |   15 +++++++++++++++
 block/qed.h |    1 +
 2 files changed, 16 insertions(+), 0 deletions(-)
Stefan Hajnoczi - March 22, 2012, 1:21 p.m.
On Wed, Mar 21, 2012 at 3:52 PM, Benoît Canet <benoit.canet@gmail.com> wrote:
> The QED image is reopened to flush metadata and check consistency.
>
> Signed-off-by: Benoit Canet <benoit.canet@gmail.com>
> ---
>  block/qed.c |   15 +++++++++++++++
>  block/qed.h |    1 +
>  2 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/block/qed.c b/block/qed.c
> index a041d31..c47272c 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -375,6 +375,10 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
>     int ret;
>
>     s->bs = bs;
> +
> +    /* backup flags for bdrv_qed_invalidate_cache */
> +    s->flags = flags;

It's not clear to me why we need to introduce this field to stash
flags values.  bs->open_flags already has this information.

Originally this was introduced in 06d9260ffa9 ("qcow2: implement
bdrv_invalidate_cache (v2)") for qcow2.  I wonder if that field is
necessary when we already have bs->open_flags.

What I don't like about s->flags is that it duplicates state *and*
it's done in each block driver that supports .bdrv_invalidate_cache().
 So I wonder if we can drop it?

Stefan
Benoit Canet - March 22, 2012, 2:15 p.m.
>It's not clear to me why we need to introduce this field to stash
>flags values.  bs->open_flags already has this information.

>Originally this was introduced in 06d9260ffa9 ("qcow2: implement
>bdrv_invalidate_cache (v2)") for qcow2.  I wonder if that field is
>necessary when we already have bs->open_flags.

>What I don't like about s->flags is that it duplicates state *and*
>it's done in each block driver that supports .bdrv_invalidate_cache().
> So I wonder if we can drop it?

I added this flag after seeing the following code in in bdrv_open_common.

    /*
     * Clear flags that are internal to the block layer before opening the
     * image.
     */
    open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);

This lead me to believe that  bs->open_flags != open_flags.
Paolo Bonzini - March 22, 2012, 2:17 p.m.
Il 22/03/2012 15:15, Benoît Canet ha scritto:
> 
> I added this flag after seeing the following code in in bdrv_open_common.
> 
>     /*
>      * Clear flags that are internal to the block layer before opening the
>      * image.
>      */
>     open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
> 
> This lead me to believe that  bs->open_flags != open_flags.

Correct, see the first two patches in the series at
http://lists.gnu.org/archive/html/qemu-devel/2012-01/msg03467.html

Paolo
Stefan Hajnoczi - March 22, 2012, 3:30 p.m.
On Thu, Mar 22, 2012 at 2:17 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 22/03/2012 15:15, Benoît Canet ha scritto:
>>
>> I added this flag after seeing the following code in in bdrv_open_common.
>>
>>     /*
>>      * Clear flags that are internal to the block layer before opening the
>>      * image.
>>      */
>>     open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>>
>> This lead me to believe that  bs->open_flags != open_flags.
>
> Correct, see the first two patches in the series at
> http://lists.gnu.org/archive/html/qemu-devel/2012-01/msg03467.html

We mangle the flags but I guess what I'm asking is, does it even
matter?  Those two bits don't affect qed or qcow2 .bdrv_open().  If
possible, I think we should use bs->open_flags.

Stefan

Patch

diff --git a/block/qed.c b/block/qed.c
index a041d31..c47272c 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -375,6 +375,10 @@  static int bdrv_qed_open(BlockDriverState *bs, int flags)
     int ret;
 
     s->bs = bs;
+
+    /* backup flags for bdrv_qed_invalidate_cache */
+    s->flags = flags;
+
     QSIMPLEQ_INIT(&s->allocating_write_reqs);
 
     ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header));
@@ -1516,6 +1520,16 @@  static int bdrv_qed_change_backing_file(BlockDriverState *bs,
     return ret;
 }
 
+static void bdrv_qed_invalidate_cache(BlockDriverState *bs)
+{
+    BDRVQEDState *s = bs->opaque;
+    int flags = s->flags;
+
+    bdrv_qed_close(bs);
+    memset(s, 0, sizeof(BDRVQEDState));
+    bdrv_qed_open(bs, flags);
+}
+
 static int bdrv_qed_check(BlockDriverState *bs, BdrvCheckResult *result)
 {
     BDRVQEDState *s = bs->opaque;
@@ -1568,6 +1582,7 @@  static BlockDriver bdrv_qed = {
     .bdrv_getlength           = bdrv_qed_getlength,
     .bdrv_get_info            = bdrv_qed_get_info,
     .bdrv_change_backing_file = bdrv_qed_change_backing_file,
+    .bdrv_invalidate_cache    = bdrv_qed_invalidate_cache,
     .bdrv_check               = bdrv_qed_check,
 };
 
diff --git a/block/qed.h b/block/qed.h
index 62624a1..cb1ebd8 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -153,6 +153,7 @@  typedef struct QEDAIOCB {
 
 typedef struct {
     BlockDriverState *bs;           /* device */
+    int flags;                      /* open flags */
     uint64_t file_size;             /* length of image file, in bytes */
 
     QEDHeader header;               /* always cpu-endian */