* [Qemu-devel] [PATCH] vmdk: Fix next_cluster_sector for compressed write
@ 2015-05-06 12:23 Fam Zheng
2015-05-06 18:01 ` [Qemu-devel] [Qemu-block] " Max Reitz
0 siblings, 1 reply; 4+ messages in thread
From: Fam Zheng @ 2015-05-06 12:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Fam Zheng, qemu-stable, qemu-block
This fixes the bug introduced by commit c6ac36e (vmdk: Optimize cluster
allocation).
Sometimes, write_len could be larger than cluster size, because it
contains both data and marker. We must advance next_cluster_sector in
this case, otherwise the image gets corrupted.
Reported-by: Antoni Villalonga <qemu-list@friki.cat>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/vmdk.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 1c5e2ef..4b4a862 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1302,6 +1302,8 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
uLongf buf_len;
const uint8_t *write_buf = buf;
int write_len = nb_sectors * 512;
+ int64_t write_offset;
+ int64_t write_end_sector;
if (extent->compressed) {
if (!extent->has_marker) {
@@ -1320,10 +1322,14 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
write_buf = (uint8_t *)data;
write_len = buf_len + sizeof(VmdkGrainMarker);
}
- ret = bdrv_pwrite(extent->file,
- cluster_offset + offset_in_cluster,
- write_buf,
- write_len);
+ write_offset = cluster_offset + offset_in_cluster,
+ ret = bdrv_pwrite(extent->file, write_offset, write_buf, write_len);
+
+ write_end_sector = DIV_ROUND_UP(write_offset + write_len, BDRV_SECTOR_SIZE);
+
+ extent->next_cluster_sector = MAX(extent->next_cluster_sector,
+ write_end_sector);
+
if (ret != write_len) {
ret = ret < 0 ? ret : -EIO;
goto out;
--
1.9.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] vmdk: Fix next_cluster_sector for compressed write
2015-05-06 12:23 [Qemu-devel] [PATCH] vmdk: Fix next_cluster_sector for compressed write Fam Zheng
@ 2015-05-06 18:01 ` Max Reitz
2015-05-08 10:49 ` Kevin Wolf
0 siblings, 1 reply; 4+ messages in thread
From: Max Reitz @ 2015-05-06 18:01 UTC (permalink / raw)
To: Fam Zheng, qemu-devel; +Cc: qemu-stable, qemu-block
On 06.05.2015 14:23, Fam Zheng wrote:
> This fixes the bug introduced by commit c6ac36e (vmdk: Optimize cluster
> allocation).
>
> Sometimes, write_len could be larger than cluster size, because it
> contains both data and marker. We must advance next_cluster_sector in
> this case, otherwise the image gets corrupted.
>
> Reported-by: Antoni Villalonga <qemu-list@friki.cat>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/vmdk.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 1c5e2ef..4b4a862 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1302,6 +1302,8 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
> uLongf buf_len;
> const uint8_t *write_buf = buf;
> int write_len = nb_sectors * 512;
> + int64_t write_offset;
> + int64_t write_end_sector;
>
> if (extent->compressed) {
> if (!extent->has_marker) {
> @@ -1320,10 +1322,14 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
> write_buf = (uint8_t *)data;
> write_len = buf_len + sizeof(VmdkGrainMarker);
> }
> - ret = bdrv_pwrite(extent->file,
> - cluster_offset + offset_in_cluster,
> - write_buf,
> - write_len);
> + write_offset = cluster_offset + offset_in_cluster,
> + ret = bdrv_pwrite(extent->file, write_offset, write_buf, write_len);
> +
> + write_end_sector = DIV_ROUND_UP(write_offset + write_len, BDRV_SECTOR_SIZE);
> +
> + extent->next_cluster_sector = MAX(extent->next_cluster_sector,
> + write_end_sector);
> +
> if (ret != write_len) {
> ret = ret < 0 ? ret : -EIO;
> goto out;
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] vmdk: Fix next_cluster_sector for compressed write
2015-05-06 18:01 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2015-05-08 10:49 ` Kevin Wolf
0 siblings, 0 replies; 4+ messages in thread
From: Kevin Wolf @ 2015-05-08 10:49 UTC (permalink / raw)
To: Max Reitz; +Cc: Fam Zheng, qemu-devel, qemu-block, qemu-stable
Am 06.05.2015 um 20:01 hat Max Reitz geschrieben:
> On 06.05.2015 14:23, Fam Zheng wrote:
> >This fixes the bug introduced by commit c6ac36e (vmdk: Optimize cluster
> >allocation).
> >
> >Sometimes, write_len could be larger than cluster size, because it
> >contains both data and marker. We must advance next_cluster_sector in
> >this case, otherwise the image gets corrupted.
> >
> >Reported-by: Antoni Villalonga <qemu-list@friki.cat>
> >Signed-off-by: Fam Zheng <famz@redhat.com>
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
Thanks, added a "Cc: qemu-stable@nongnu.org" line to the commit message
and applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] vmdk: Fix next_cluster_sector for compressed write
[not found] ` <20150910085803.GB27306@ad.nay.redhat.com>
@ 2015-09-10 12:02 ` Kevin Wolf
0 siblings, 0 replies; 4+ messages in thread
From: Kevin Wolf @ 2015-09-10 12:02 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-block, Radoslav Gerganov, qemu-devel
Am 10.09.2015 um 10:58 hat Fam Zheng geschrieben:
> On Thu, 09/10 11:48, Radoslav Gerganov wrote:
> > On 10.09.2015 11:15, Fam Zheng wrote:
> > > On Thu, 09/10 10:53, Radoslav Gerganov wrote:
> > >> When the VMDK is streamOptimized (or compressed), the
> > >> next_cluster_sector must not be incremented by a fixed number of
> > >> sectors. Instead of this, it must be rounded up to the next consecutive
> > >> sector. Fixing this results in much smaller compressed images.
> > >>
> > >> Signed-off-by: Radoslav Gerganov <rgerganov@vmware.com>
> > >
> > > Is this patch enough to produce a valid streamOptimized image? Because I
> > > remember there were reports on some other issues with footer or grain markers.
> > >
> > > Fam
> >
> > The VMDK spec says that each marker and its associated block must begin on
> > sector boundary which is true before and after my patch. However, with my
> > patch we don't put empty sectors when we achieve good compression for the
> > current cluster but start the next cluster on the next consecutive sector.
> >
> > I have also verified that images produced with my patch can be booted in
> > VMware Workstation.
>
> Thanks, the change looks good to me.
>
> Reviewed-by: Fam Zheng <famz@redhat.com>
Thanks, applied to the block branch.
Radoslav, for your next patch please remember to always keep qemu-devel
CCed, even if you're sending the patch to a more specific mailing list.
I'm adding it to this mail so that people who aren't subscribed to
qemu-block have a chance to notice the patch.
Kevin
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-09-10 12:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-06 12:23 [Qemu-devel] [PATCH] vmdk: Fix next_cluster_sector for compressed write Fam Zheng
2015-05-06 18:01 ` [Qemu-devel] [Qemu-block] " Max Reitz
2015-05-08 10:49 ` Kevin Wolf
[not found] <1441871594-21252-1-git-send-email-rgerganov@vmware.com>
[not found] ` <20150910081557.GA27306@ad.nay.redhat.com>
[not found] ` <55F143C6.5020401@vmware.com>
[not found] ` <20150910085803.GB27306@ad.nay.redhat.com>
2015-09-10 12:02 ` [Qemu-devel] " Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.