| Message ID | 1332345124-381-5-git-send-email-benoit.canet@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
>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.
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
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
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 */
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(-)