* [Qemu-devel] [PATCH v2] vmdk: align end of file to a sector boundary
@ 2018-09-13 2:31 yuchenlin
2018-09-13 2:54 ` Fam Zheng
0 siblings, 1 reply; 4+ messages in thread
From: yuchenlin @ 2018-09-13 2:31 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, qemu-block, yuchenlin
From: yuchenlin <yuchenlin@synology.com>
There is a rare case which the size of last compressed cluster
is larger than the cluster size, which will cause the file is
not aligned at the sector boundary.
Signed-off-by: yuchenlin <yuchenlin@synology.com>
---
v1 -> v2:
* Add more detail comment.
* Add QEMU_ALIGN_UP to show the intention more clearly.
* Add newline in the end of bdrv_truncate.
thanks
block/vmdk.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index a9d0084e36..2c9e86d98f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1698,6 +1698,27 @@ static int coroutine_fn
vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov)
{
+ if (bytes == 0) {
+ /* The caller will write bytes 0 to signal EOF.
+ * When receive it, we align EOF to a sector boundary. */
+ BDRVVmdkState *s = bs->opaque;
+ int i, ret;
+ int64_t length;
+
+ for (i = 0; i < s->num_extents; i++) {
+ length = bdrv_getlength(s->extents[i].file->bs);
+ if (length < 0) {
+ return length;
+ }
+ length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE);
+ ret = bdrv_truncate(s->extents[i].file, length,
+ PREALLOC_MODE_OFF, NULL);
+ if (ret < 0) {
+ return ret;
+ }
+ }
+ return 0;
+ }
return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);
}
--
2.18.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] vmdk: align end of file to a sector boundary
2018-09-13 2:31 [Qemu-devel] [PATCH v2] vmdk: align end of file to a sector boundary yuchenlin
@ 2018-09-13 2:54 ` Fam Zheng
2018-09-13 7:47 ` yuchenlin
0 siblings, 1 reply; 4+ messages in thread
From: Fam Zheng @ 2018-09-13 2:54 UTC (permalink / raw)
To: yuchenlin; +Cc: qemu-devel, qemu-block
On Thu, 09/13 10:31, yuchenlin@synology.com wrote:
> From: yuchenlin <yuchenlin@synology.com>
>
> There is a rare case which the size of last compressed cluster
> is larger than the cluster size, which will cause the file is
> not aligned at the sector boundary.
The code looks good to me. Can you also explain why it is important to align
file size to sector boundary in the comment?
Fam
>
> Signed-off-by: yuchenlin <yuchenlin@synology.com>
> ---
> v1 -> v2:
> * Add more detail comment.
> * Add QEMU_ALIGN_UP to show the intention more clearly.
> * Add newline in the end of bdrv_truncate.
>
> thanks
>
> block/vmdk.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index a9d0084e36..2c9e86d98f 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1698,6 +1698,27 @@ static int coroutine_fn
> vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
> uint64_t bytes, QEMUIOVector *qiov)
> {
> + if (bytes == 0) {
> + /* The caller will write bytes 0 to signal EOF.
> + * When receive it, we align EOF to a sector boundary. */
> + BDRVVmdkState *s = bs->opaque;
> + int i, ret;
> + int64_t length;
> +
> + for (i = 0; i < s->num_extents; i++) {
> + length = bdrv_getlength(s->extents[i].file->bs);
> + if (length < 0) {
> + return length;
> + }
> + length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE);
> + ret = bdrv_truncate(s->extents[i].file, length,
> + PREALLOC_MODE_OFF, NULL);
> + if (ret < 0) {
> + return ret;
> + }
> + }
> + return 0;
> + }
> return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);
> }
>
> --
> 2.18.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] vmdk: align end of file to a sector boundary
2018-09-13 2:54 ` Fam Zheng
@ 2018-09-13 7:47 ` yuchenlin
2018-09-13 8:20 ` Fam Zheng
0 siblings, 1 reply; 4+ messages in thread
From: yuchenlin @ 2018-09-13 7:47 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, qemu-block
On 2018-09-13 10:54, Fam Zheng wrote:
> On Thu, 09/13 10:31, yuchenlin@synology.com wrote:
>> From: yuchenlin <yuchenlin@synology.com>
>>
>> There is a rare case which the size of last compressed cluster
>> is larger than the cluster size, which will cause the file is
>> not aligned at the sector boundary.
>
> The code looks good to me. Can you also explain why it is important to
> align
> file size to sector boundary in the comment?
>
> Fam
>
In my opinion, there are three reasons to do it. First, if vmdk doesn't
align at the sector boundary, there may be many undefined behaviors,
such as in vbox it will show VMDK: Compressed image is corrupted
'88-disk1.vmdk' (VERR_ZIP_CORRUPTED) when we try to import an ova with
unaligned vmdk. Second, all the cluster_sector is aligned
to sector, the last one should be like this, too. Third, it ease reading
with sector based I/Os.
What do you think?
yuchenlin
>>
>> Signed-off-by: yuchenlin <yuchenlin@synology.com>
>> ---
>> v1 -> v2:
>> * Add more detail comment.
>> * Add QEMU_ALIGN_UP to show the intention more clearly.
>> * Add newline in the end of bdrv_truncate.
>>
>> thanks
>>
>> block/vmdk.c | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index a9d0084e36..2c9e86d98f 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -1698,6 +1698,27 @@ static int coroutine_fn
>> vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>> uint64_t bytes, QEMUIOVector *qiov)
>> {
>> + if (bytes == 0) {
>> + /* The caller will write bytes 0 to signal EOF.
>> + * When receive it, we align EOF to a sector boundary. */
>> + BDRVVmdkState *s = bs->opaque;
>> + int i, ret;
>> + int64_t length;
>> +
>> + for (i = 0; i < s->num_extents; i++) {
>> + length = bdrv_getlength(s->extents[i].file->bs);
>> + if (length < 0) {
>> + return length;
>> + }
>> + length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE);
>> + ret = bdrv_truncate(s->extents[i].file, length,
>> + PREALLOC_MODE_OFF, NULL);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> + }
>> + return 0;
>> + }
>> return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);
>> }
>>
>> --
>> 2.18.0
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] vmdk: align end of file to a sector boundary
2018-09-13 7:47 ` yuchenlin
@ 2018-09-13 8:20 ` Fam Zheng
0 siblings, 0 replies; 4+ messages in thread
From: Fam Zheng @ 2018-09-13 8:20 UTC (permalink / raw)
To: yuchenlin; +Cc: qemu-devel, qemu-block
On Thu, 09/13 15:47, yuchenlin wrote:
> On 2018-09-13 10:54, Fam Zheng wrote:
> > On Thu, 09/13 10:31, yuchenlin@synology.com wrote:
> > > From: yuchenlin <yuchenlin@synology.com>
> > >
> > > There is a rare case which the size of last compressed cluster
> > > is larger than the cluster size, which will cause the file is
> > > not aligned at the sector boundary.
> >
> > The code looks good to me. Can you also explain why it is important to
> > align
> > file size to sector boundary in the comment?
> >
> > Fam
> >
>
> In my opinion, there are three reasons to do it. First, if vmdk doesn't
> align at the sector boundary, there may be many undefined behaviors, such as
> in vbox it will show VMDK: Compressed image is corrupted '88-disk1.vmdk'
> (VERR_ZIP_CORRUPTED) when we try to import an ova with unaligned vmdk.
> Second, all the cluster_sector is aligned
> to sector, the last one should be like this, too. Third, it ease reading
> with sector based I/Os.
>
> What do you think?
Yes, it would be nice if you could add this information to the commit message or
the code comment.
Fam
>
> yuchenlin
>
> > >
> > > Signed-off-by: yuchenlin <yuchenlin@synology.com>
> > > ---
> > > v1 -> v2:
> > > * Add more detail comment.
> > > * Add QEMU_ALIGN_UP to show the intention more clearly.
> > > * Add newline in the end of bdrv_truncate.
> > >
> > > thanks
> > >
> > > block/vmdk.c | 21 +++++++++++++++++++++
> > > 1 file changed, 21 insertions(+)
> > >
> > > diff --git a/block/vmdk.c b/block/vmdk.c
> > > index a9d0084e36..2c9e86d98f 100644
> > > --- a/block/vmdk.c
> > > +++ b/block/vmdk.c
> > > @@ -1698,6 +1698,27 @@ static int coroutine_fn
> > > vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
> > > uint64_t bytes, QEMUIOVector *qiov)
> > > {
> > > + if (bytes == 0) {
> > > + /* The caller will write bytes 0 to signal EOF.
> > > + * When receive it, we align EOF to a sector boundary. */
> > > + BDRVVmdkState *s = bs->opaque;
> > > + int i, ret;
> > > + int64_t length;
> > > +
> > > + for (i = 0; i < s->num_extents; i++) {
> > > + length = bdrv_getlength(s->extents[i].file->bs);
> > > + if (length < 0) {
> > > + return length;
> > > + }
> > > + length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE);
> > > + ret = bdrv_truncate(s->extents[i].file, length,
> > > + PREALLOC_MODE_OFF, NULL);
> > > + if (ret < 0) {
> > > + return ret;
> > > + }
> > > + }
> > > + return 0;
> > > + }
> > > return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);
> > > }
> > >
> > > --
> > > 2.18.0
> > >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-09-13 8:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-13 2:31 [Qemu-devel] [PATCH v2] vmdk: align end of file to a sector boundary yuchenlin
2018-09-13 2:54 ` Fam Zheng
2018-09-13 7:47 ` yuchenlin
2018-09-13 8:20 ` Fam Zheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).