[Qemu-devel] [PATCH 0/2] block: two fixes for image opening

[Qemu-devel] [PATCH 0/2] block: two fixes for image opening

[Fam Zheng]

This fixes two bugs of image opening. The vmdk fix also applies to 1.6.

Fam Zheng (2):
  block: don't lose data from last incomplete sector
  vmdk: fix cluster size check for flat extents

 block.c      | 2 +-
 block/vmdk.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] block: don't lose data from last incomplete sector

[Fam Zheng]

To read the last sector that is not aligned to sector boundary, current
code for growable backends, since commit 893a8f6 "block: Produce zeros
when protocols reading beyond end of file", drops the data and directly
returns zeroes. That is incorrect.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index a325efc..9b28ef9 100644
--- a/block.c
+++ b/block.c
@@ -2613,7 +2613,7 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
             goto out;
         }
 
-        total_sectors = len >> BDRV_SECTOR_BITS;
+        total_sectors = (len + BDRV_SECTOR_SIZE - 1) >> BDRV_SECTOR_BITS;
         max_nb_sectors = MAX(0, total_sectors - sector_num);
         if (max_nb_sectors > 0) {
             ret = drv->bdrv_co_readv(bs, sector_num,
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] vmdk: fix cluster size check for flat extents

[Fam Zheng]

We use the extent size as cluster size for flat extents (where no L1/L2
table is allocated so it's safe).

So don't check the cluster size for flat case in opening.

Otherwise flat extent opening is broken:

    # qemu-img create -f vmdk -o subformat=monolithicFlat /tmp/a.vmdk 100G
    Formatting '/tmp/a.vmdk', fmt=vmdk size=107374182400 compat6=off subformat='monolithicFlat' zeroed_grain=off
    # qemu-img info /tmp/a.vmdk
    image: /tmp/a.vmdk
    file format: raw
    virtual size: 0 (0 bytes)
    disk size: 4.0K

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index fb5b529..fdd4eaa 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -396,7 +396,7 @@ static int vmdk_add_extent(BlockDriverState *bs,
     VmdkExtent *extent;
     BDRVVmdkState *s = bs->opaque;
 
-    if (cluster_sectors > 0x200000) {
+    if (!flat && cluster_sectors > 0x200000) {
         /* 0x200000 * 512Bytes = 1GB for one cluster is unrealistic */
         error_report("invalid granularity, image may be corrupt");
         return -EINVAL;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] vmdk: fix cluster size check for flat extents

[Stefan Hajnoczi]

On Wed, Sep 18, 2013 at 07:14:15PM +0800, Fam Zheng wrote:
> We use the extent size as cluster size for flat extents (where no L1/L2
> table is allocated so it's safe).

Why is the extent size passed as the cluster size parameter?

The cleaner fix would be to stop passing it as cluster size :).

Stefan

Re: [Qemu-devel] [PATCH 0/2] block: two fixes for image opening

[Stefan Hajnoczi]

On Wed, Sep 18, 2013 at 07:14:13PM +0800, Fam Zheng wrote:
> This fixes two bugs of image opening. The vmdk fix also applies to 1.6.
> 
> Fam Zheng (2):
>   block: don't lose data from last incomplete sector
>   vmdk: fix cluster size check for flat extents
> 
>  block.c      | 2 +-
>  block/vmdk.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Please add qemu-iotests cases for these bugs.  This way we get
regression tests that ensure the same bug will not be reintroduced in
the future.

Stefan

Re: [Qemu-devel] [PATCH 1/2] block: don't lose data from last incomplete sector

[Stefan Hajnoczi]

On Wed, Sep 18, 2013 at 07:14:14PM +0800, Fam Zheng wrote:
> To read the last sector that is not aligned to sector boundary, current
> code for growable backends, since commit 893a8f6 "block: Produce zeros
> when protocols reading beyond end of file", drops the data and directly
> returns zeroes. That is incorrect.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I merged this patch because I hit the problem myself today.  And my code
includes a test case which will be posted soon.

So don't worry about writing a test case for this patch.  I still don't
fully understand the vmdk patch and would like a test case for it.

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

Stefan

Re: [Qemu-devel] [PATCH 2/2] vmdk: fix cluster size check for flat extents

[Paolo Bonzini]

Il 19/09/2013 13:31, Stefan Hajnoczi ha scritto:
>> > We use the extent size as cluster size for flat extents (where no L1/L2
>> > table is allocated so it's safe).
> Why is the extent size passed as the cluster size parameter?

I think it's so that the flat extent doesn't take up too many cache entries.

Paolo

> The cleaner fix would be to stop passing it as cluster size :).

Re: [Qemu-devel] [PATCH 2/2] vmdk: fix cluster size check for flat extents

[Fam Zheng]

On Fri, 09/20 17:04, Paolo Bonzini wrote:
> Il 19/09/2013 13:31, Stefan Hajnoczi ha scritto:
> >> > We use the extent size as cluster size for flat extents (where no L1/L2
> >> > table is allocated so it's safe).
> > Why is the extent size passed as the cluster size parameter?
> 
> I think it's so that the flat extent doesn't take up too many cache entries.
> 
> Paolo

Flat extent doesn't take cache entry at all. It's passed as this because so
that more the code path for finding data offset can be the same with sparse:
flat is a special case of sparse with only 1 cluster. Otherwize flat needs to
be treated specially.

Another fix would be pass zero to cluster size parameter but initialize it to
extent size after parameter checking. But not quite different.

Fam