[Qemu-devel] [PATCH] block: Introduce bs->zero_beyond_eof

[Qemu-devel] [PATCH] block: Introduce bs->zero_beyond_eof

[Asias He]

In 4146b46c42e0989cb5842e04d88ab6ccb1713a48 (block: Produce zeros when
protocols reading beyond end of file), we break qemu-iotests ./check
-qcow2 022. This happens because qcow2 temporarily sets ->growable = 1
for vmstate accesses (which are stored beyond the end of regular image
data).

We introduce the bs->zero_beyond_eof to allow qcow2_load_vmstate() to
disable ->zero_beyond_eof temporarily in addition to enable ->growable.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Asias He <asias@redhat.com>
---
 block.c                   | 5 ++++-
 block/qcow2.c             | 3 +++
 include/block/block_int.h | 3 +++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index f3cd9fb..3a320d5 100644
--- a/block.c
+++ b/block.c
@@ -868,6 +868,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
     QDECREF(options);
 
     bs->growable = 1;
+    bs->zero_beyond_eof = true;
     *pbs = bs;
     return 0;
 
@@ -978,6 +979,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     }
 
     bs->options = options;
+    bs->zero_beyond_eof = true;
     options = qdict_clone_shallow(options);
 
     /* For snapshot=on, create a temporary qcow2 overlay */
@@ -1402,6 +1404,7 @@ void bdrv_close(BlockDriverState *bs)
         bs->valid_key = 0;
         bs->sg = 0;
         bs->growable = 0;
+        bs->zero_beyond_eof = false;
         QDECREF(bs->options);
         bs->options = NULL;
 
@@ -2544,7 +2547,7 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
         }
     }
 
-    if (!bs->growable) {
+    if (!(bs->zero_beyond_eof && bs->growable)) {
         ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
     } else {
         /* Read zeros after EOF of growable BDSes */
diff --git a/block/qcow2.c b/block/qcow2.c
index 3376901..14e863d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1722,12 +1722,15 @@ static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf,
 {
     BDRVQcowState *s = bs->opaque;
     int growable = bs->growable;
+    bool zero_beyond_eof = bs->zero_beyond_eof;
     int ret;
 
     BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD);
     bs->growable = 1;
+    bs->zero_beyond_eof = false;
     ret = bdrv_pread(bs, qcow2_vm_state_offset(s) + pos, buf, size);
     bs->growable = growable;
+    bs->zero_beyond_eof = zero_beyond_eof;
 
     return ret;
 }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index e45f2a0..74b0689 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -281,6 +281,9 @@ struct BlockDriverState {
     /* Whether the disk can expand beyond total_sectors */
     int growable;
 
+    /* Whether produces zeros when read beyond eof */
+    bool zero_beyond_eof;
+
     /* the memory alignment required for the buffers handled by this driver */
     int buffer_alignment;
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] block: Introduce bs->zero_beyond_eof

[Stefan Hajnoczi]

On Wed, Aug 21, 2013 at 04:26:04PM +0800, Asias He wrote:
> @@ -868,6 +868,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>      QDECREF(options);
>  
>      bs->growable = 1;
> +    bs->zero_beyond_eof = true;
>      *pbs = bs;
>      return 0;
>  
> @@ -978,6 +979,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>      }
>  
>      bs->options = options;
> +    bs->zero_beyond_eof = true;
>      options = qdict_clone_shallow(options);
>  
>      /* For snapshot=on, create a temporary qcow2 overlay */

We chatted about whether to duplicate bs->zero_beyond_eof = true on IRC.
Now I think you could put it in bdrv_open_common() to avoid duplication.
Every BDS should have ->zero_beyond_eof = true by default.

Stefan

[Qemu-devel] [PATCH v2] block: Introduce bs->zero_beyond_eof

[Asias He]

In 4146b46c42e0989cb5842e04d88ab6ccb1713a48 (block: Produce zeros when
protocols reading beyond end of file), we break qemu-iotests ./check
-qcow2 022. This happens because qcow2 temporarily sets ->growable = 1
for vmstate accesses (which are stored beyond the end of regular image
data).

We introduce the bs->zero_beyond_eof to allow qcow2_load_vmstate() to
disable ->zero_beyond_eof temporarily in addition to enable ->growable.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Asias He <asias@redhat.com>
---
Changes in v2: Set bs->zero_beyond_eof in bdrv_open_common

 block.c                   | 4 +++-
 block/qcow2.c             | 3 +++
 include/block/block_int.h | 3 +++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index f3cd9fb..6668c05 100644
--- a/block.c
+++ b/block.c
@@ -706,6 +706,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
 
     bs->open_flags = flags;
     bs->buffer_alignment = 512;
+    bs->zero_beyond_eof = true;
     open_flags = bdrv_open_flags(bs, flags);
     bs->read_only = !(open_flags & BDRV_O_RDWR);
 
@@ -1402,6 +1403,7 @@ void bdrv_close(BlockDriverState *bs)
         bs->valid_key = 0;
         bs->sg = 0;
         bs->growable = 0;
+        bs->zero_beyond_eof = false;
         QDECREF(bs->options);
         bs->options = NULL;
 
@@ -2544,7 +2546,7 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
         }
     }
 
-    if (!bs->growable) {
+    if (!(bs->zero_beyond_eof && bs->growable)) {
         ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
     } else {
         /* Read zeros after EOF of growable BDSes */
diff --git a/block/qcow2.c b/block/qcow2.c
index 3376901..14e863d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1722,12 +1722,15 @@ static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf,
 {
     BDRVQcowState *s = bs->opaque;
     int growable = bs->growable;
+    bool zero_beyond_eof = bs->zero_beyond_eof;
     int ret;
 
     BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD);
     bs->growable = 1;
+    bs->zero_beyond_eof = false;
     ret = bdrv_pread(bs, qcow2_vm_state_offset(s) + pos, buf, size);
     bs->growable = growable;
+    bs->zero_beyond_eof = zero_beyond_eof;
 
     return ret;
 }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index e45f2a0..74b0689 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -281,6 +281,9 @@ struct BlockDriverState {
     /* Whether the disk can expand beyond total_sectors */
     int growable;
 
+    /* Whether produces zeros when read beyond eof */
+    bool zero_beyond_eof;
+
     /* the memory alignment required for the buffers handled by this driver */
     int buffer_alignment;
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] block: Introduce bs->zero_beyond_eof

[Stefan Hajnoczi]

On Thu, Aug 22, 2013 at 03:24:14PM +0800, Asias He wrote:
> In 4146b46c42e0989cb5842e04d88ab6ccb1713a48 (block: Produce zeros when
> protocols reading beyond end of file), we break qemu-iotests ./check
> -qcow2 022. This happens because qcow2 temporarily sets ->growable = 1
> for vmstate accesses (which are stored beyond the end of regular image
> data).
> 
> We introduce the bs->zero_beyond_eof to allow qcow2_load_vmstate() to
> disable ->zero_beyond_eof temporarily in addition to enable ->growable.
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Asias He <asias@redhat.com>
> ---
> Changes in v2: Set bs->zero_beyond_eof in bdrv_open_common
> 
>  block.c                   | 4 +++-
>  block/qcow2.c             | 3 +++
>  include/block/block_int.h | 3 +++
>  3 files changed, 9 insertions(+), 1 deletion(-)

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

...with a little twist: since the broken patch hasn't been merged yet
I'm applying this fix *first* to keep the tree bisectable.

Stefan

Re: [Qemu-devel] [PATCH v2] block: Introduce bs->zero_beyond_eof

[Asias He]

On Thu, Aug 22, 2013 at 02:12:29PM +0200, Stefan Hajnoczi wrote:
> On Thu, Aug 22, 2013 at 03:24:14PM +0800, Asias He wrote:
> > In 4146b46c42e0989cb5842e04d88ab6ccb1713a48 (block: Produce zeros when
> > protocols reading beyond end of file), we break qemu-iotests ./check
> > -qcow2 022. This happens because qcow2 temporarily sets ->growable = 1
> > for vmstate accesses (which are stored beyond the end of regular image
> > data).
> > 
> > We introduce the bs->zero_beyond_eof to allow qcow2_load_vmstate() to
> > disable ->zero_beyond_eof temporarily in addition to enable ->growable.
> > 
> > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Asias He <asias@redhat.com>
> > ---
> > Changes in v2: Set bs->zero_beyond_eof in bdrv_open_common
> > 
> >  block.c                   | 4 +++-
> >  block/qcow2.c             | 3 +++
> >  include/block/block_int.h | 3 +++
> >  3 files changed, 9 insertions(+), 1 deletion(-)
> 
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block
> 
> ...with a little twist: since the broken patch hasn't been merged yet
> I'm applying this fix *first* to keep the tree bisectable.

Thanks!

> Stefan

-- 
Asias

Re: [Qemu-devel] [PATCH] block: Introduce bs->zero_beyond_eof

[Asias He]

On Wed, Aug 21, 2013 at 05:44:08PM +0200, Stefan Hajnoczi wrote:
> On Wed, Aug 21, 2013 at 04:26:04PM +0800, Asias He wrote:
> > @@ -868,6 +868,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> >      QDECREF(options);
> >  
> >      bs->growable = 1;
> > +    bs->zero_beyond_eof = true;
> >      *pbs = bs;
> >      return 0;
> >  
> > @@ -978,6 +979,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> >      }
> >  
> >      bs->options = options;
> > +    bs->zero_beyond_eof = true;
> >      options = qdict_clone_shallow(options);
> >  
> >      /* For snapshot=on, create a temporary qcow2 overlay */
> 
> We chatted about whether to duplicate bs->zero_beyond_eof = true on IRC.
> Now I think you could put it in bdrv_open_common() to avoid duplication.
> Every BDS should have ->zero_beyond_eof = true by default.

Nice. v2 is on its way.

> Stefan

-- 
Asias