[PATCH] vmdk:truncate more one sector in init extent

[PATCH] vmdk:truncate more one sector in init extent

[luzhipeng]

issue:https://gitlab.com/qemu-project/qemu/-/issues/1357
empty vmdk only contains metadata, ovftool failed.
So it allocates more one sector for empty disk. the ovftool
command line: ovftool input.ovf output.ova

Signed-off-by: luzhipeng <luzhipeng@cestc.cn>
---
 block/vmdk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 78f6433607..283dee9b49 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2286,7 +2286,7 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
         goto exit;
     }
 
-    ret = blk_co_truncate(blk, le64_to_cpu(header.grain_offset) << 9, false,
+    ret = blk_co_truncate(blk, (le64_to_cpu(header.grain_offset) << 9) + BDRV_SECTOR_SIZE,
+                          false, PREALLOC_MODE_OFF, 0, errp);
     if (ret < 0) {
         goto exit;
-- 
2.39.3

Re: [PATCH] vmdk:truncate more one sector in init extent

[Kevin Wolf]

Am 22.08.2024 um 12:52 hat luzhipeng geschrieben:
> issue:https://gitlab.com/qemu-project/qemu/-/issues/1357
> empty vmdk only contains metadata, ovftool failed.
> So it allocates more one sector for empty disk. the ovftool
> command line: ovftool input.ovf output.ova
> 
> Signed-off-by: luzhipeng <luzhipeng@cestc.cn>

I think this commit message needs more of the information from the bug
report, otherwise it seems unexplainable why adding an empty sector
should make a difference.

> diff --git a/block/vmdk.c b/block/vmdk.c
> index 78f6433607..283dee9b49 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -2286,7 +2286,7 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
>          goto exit;
>      }
>  
> -    ret = blk_co_truncate(blk, le64_to_cpu(header.grain_offset) << 9, false,
> +    ret = blk_co_truncate(blk, (le64_to_cpu(header.grain_offset) << 9) + BDRV_SECTOR_SIZE,
> +                          false, PREALLOC_MODE_OFF, 0, errp);
>      if (ret < 0) {
>          goto exit;

This is not a good fix. It means that we will always leave an empty
sector after the header, even if more data follows.

Does the problem really only happen with empty images? I think we don't
necessarily add an end-of-stream marker for other images either, we just
align the image size to full sectors at the end of 'qemu-img convert'.

I wonder if vmdk_co_pwritev_compressed() should be changed to write both
a footer and an explicit end-of-stream marker for streamOptimized images
in the bytes == 0 case.

Kevin

Re: [PATCH] vmdk:truncate more one sector in init extent

[Zhipeng Lu]

在 2024/10/19 0:31, Kevin Wolf 写道:
> Am 22.08.2024 um 12:52 hat luzhipeng geschrieben:
>> issue:https://gitlab.com/qemu-project/qemu/-/issues/1357
>> empty vmdk only contains metadata, ovftool failed.
>> So it allocates more one sector for empty disk. the ovftool
>> command line: ovftool input.ovf output.ova
>>
>> Signed-off-by: luzhipeng <luzhipeng@cestc.cn>
> 
> I think this commit message needs more of the information from the bug
> report, otherwise it seems unexplainable why adding an empty sector
> should make a difference.


I really don't know the real reason, I suspect it's a special 
requirement of the ovftool tool,
but this issue does exist.

>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 78f6433607..283dee9b49 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -2286,7 +2286,7 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
>>           goto exit;
>>       }
>>   
>> -    ret = blk_co_truncate(blk, le64_to_cpu(header.grain_offset) << 9, false,
>> +    ret = blk_co_truncate(blk, (le64_to_cpu(header.grain_offset) << 9) + BDRV_SECTOR_SIZE,
>> +                          false, PREALLOC_MODE_OFF, 0, errp);
>>       if (ret < 0) {
>>           goto exit;
> 
> This is not a good fix. It means that we will always leave an empty
> sector after the header, even if more data follows.
> 
> Does the problem really only happen with empty images? I think we don't
> necessarily add an end-of-stream marker for other images either, we just
> align the image size to full sectors at the end of 'qemu-img convert'.

this issue indeed exists.it is not fix for aligning the image size

> I wonder if vmdk_co_pwritev_compressed() should be changed to write both
> a footer and an explicit end-of-stream marker for streamOptimized images
> in the bytes == 0 case.
> 
> Kevin
> 
> 
>