* [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary
@ 2018-08-28 3:17 yuchenlin
2018-09-05 2:14 ` yuchenlin
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: yuchenlin @ 2018-08-28 3:17 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>
---
block/vmdk.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index a9d0084e36..a8ae7c65d2 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1698,6 +1698,24 @@ static int coroutine_fn
vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov)
{
+ if (bytes == 0) {
+ /* align end of file 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;
+ }
+ 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.17.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary
2018-08-28 3:17 [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary yuchenlin
@ 2018-09-05 2:14 ` yuchenlin
2018-09-12 9:34 ` Fam Zheng
2018-09-12 11:54 ` Fam Zheng
2 siblings, 0 replies; 7+ messages in thread
From: yuchenlin @ 2018-09-05 2:14 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, qemu-block
Ping!
yuchenlin@synology.com 於 2018-08-28 11:18 寫道:
> 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> --- block/vmdk.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/block/vmdk.c b/block/vmdk.c index a9d0084e36..a8ae7c65d2 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1698,6 +1698,24 @@ static int coroutine_fn vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov) { + if (bytes == 0) { + /* align end of file 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; + } + 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.17.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary
2018-08-28 3:17 [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary yuchenlin
2018-09-05 2:14 ` yuchenlin
@ 2018-09-12 9:34 ` Fam Zheng
2018-09-12 9:52 ` yuchenlin
2018-09-12 11:54 ` Fam Zheng
2 siblings, 1 reply; 7+ messages in thread
From: Fam Zheng @ 2018-09-12 9:34 UTC (permalink / raw)
To: yuchenlin; +Cc: qemu-devel, qemu-block
On Tue, 08/28 11:17, 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.
I don't understand. Doesn't it mean that if you force the alignment by
truncating out the extra bytes, some data is lost?
>
> Signed-off-by: yuchenlin <yuchenlin@synology.com>
> ---
> block/vmdk.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index a9d0084e36..a8ae7c65d2 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1698,6 +1698,24 @@ static int coroutine_fn
> vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
> uint64_t bytes, QEMUIOVector *qiov)
> {
> + if (bytes == 0) {
Where is this bytes == 0 condition from?
> + /* align end of file 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;
> + }
> + 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.17.0
>
Fam
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary
2018-09-12 9:34 ` Fam Zheng
@ 2018-09-12 9:52 ` yuchenlin
2018-09-12 11:52 ` Fam Zheng
0 siblings, 1 reply; 7+ messages in thread
From: yuchenlin @ 2018-09-12 9:52 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, qemu-block
Fam Zheng <famz@redhat.com> 於 2018-09-12 17:34 寫道:
> On Tue, 08/28 11:17, 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. I don't understand. Doesn't it mean that if you force the alignment by truncating out the extra bytes, some data is lost?
You can take qcow2_co_pwritev_compressed in block/qcow2.c as an example.
The bdrv_getlength will return the length in bytes which is always a multiple of BDRV_SECTOR_SIZE.
After truncates this size, the vmdk is extended to align sector size.
> > > Signed-off-by: yuchenlin <yuchenlin@synology.com> > --- > block/vmdk.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/block/vmdk.c b/block/vmdk.c > index a9d0084e36..a8ae7c65d2 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -1698,6 +1698,24 @@ static int coroutine_fn > vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, > uint64_t bytes, QEMUIOVector *qiov) > { > + if (bytes == 0) { Where is this bytes == 0 condition from?
From the end of convert_do_copy in qemu-img.c.
if (s->compressed && !s->ret) {
/* signal EOF to align */
ret = blk_pwrite_compressed(s->target, 0, NULL, 0);
if (ret < 0) {
return ret;
}
}
It signals the EOF to the block driver.
> > + /* align end of file 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; > + } > + 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.17.0 > Fam
yuchenlin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary
2018-09-12 9:52 ` yuchenlin
@ 2018-09-12 11:52 ` Fam Zheng
0 siblings, 0 replies; 7+ messages in thread
From: Fam Zheng @ 2018-09-12 11:52 UTC (permalink / raw)
To: yuchenlin; +Cc: qemu-devel, qemu-block
On Wed, 09/12 17:52, yuchenlin wrote:
>
> Fam Zheng <famz@redhat.com> 於 2018-09-12 17:34 寫道:
> > On Tue, 08/28 11:17, 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. I don't understand. Doesn't it mean that if you force the alignment by truncating out the extra bytes, some data is lost?
>
> You can take qcow2_co_pwritev_compressed in block/qcow2.c as an example.
> The bdrv_getlength will return the length in bytes which is always a multiple of BDRV_SECTOR_SIZE.
> After truncates this size, the vmdk is extended to align sector size.
> > > > Signed-off-by: yuchenlin <yuchenlin@synology.com> > --- > block/vmdk.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/block/vmdk.c b/block/vmdk.c > index a9d0084e36..a8ae7c65d2 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -1698,6 +1698,24 @@ static int coroutine_fn > vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, > uint64_t bytes, QEMUIOVector *qiov) > { > + if (bytes == 0) { Where is this bytes == 0 condition from?
>
> From the end of convert_do_copy in qemu-img.c.
>
> if (s->compressed && !s->ret) {
> /* signal EOF to align */
> ret = blk_pwrite_compressed(s->target, 0, NULL, 0);
> if (ret < 0) {
> return ret;
> }
> }
I see. Please mention this in the comment of the if check.
Fam
>
> It signals the EOF to the block driver.
> > > + /* align end of file 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; > + } > + 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.17.0 > Fam
>
>
> yuchenlin
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary
2018-08-28 3:17 [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary yuchenlin
2018-09-05 2:14 ` yuchenlin
2018-09-12 9:34 ` Fam Zheng
@ 2018-09-12 11:54 ` Fam Zheng
2018-09-13 1:51 ` yuchenlin
2 siblings, 1 reply; 7+ messages in thread
From: Fam Zheng @ 2018-09-12 11:54 UTC (permalink / raw)
To: yuchenlin; +Cc: qemu-devel, qemu-block
On Tue, 08/28 11:17, 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.
>
> Signed-off-by: yuchenlin <yuchenlin@synology.com>
> ---
> block/vmdk.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index a9d0084e36..a8ae7c65d2 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1698,6 +1698,24 @@ static int coroutine_fn
> vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
> uint64_t bytes, QEMUIOVector *qiov)
> {
> + if (bytes == 0) {
> + /* align end of file 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;
> + }
Could you add "length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE);" to show the
intention more clearly?
Fam
> + 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.17.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary
2018-09-12 11:54 ` Fam Zheng
@ 2018-09-13 1:51 ` yuchenlin
0 siblings, 0 replies; 7+ messages in thread
From: yuchenlin @ 2018-09-13 1:51 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, qemu-block
On 2018-09-12 19:54, Fam Zheng wrote:
> On Tue, 08/28 11:17, 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.
>>
>> Signed-off-by: yuchenlin <yuchenlin@synology.com>
>> ---
>> block/vmdk.c | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index a9d0084e36..a8ae7c65d2 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -1698,6 +1698,24 @@ static int coroutine_fn
>> vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>> uint64_t bytes, QEMUIOVector *qiov)
>> {
>> + if (bytes == 0) {
>> + /* align end of file 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;
>> + }
>
> Could you add "length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE);" to
> show the
> intention more clearly?
>
> Fam
>
Thank you for your effort, I will do it.
yuchenlin
>> + 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.17.0
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-09-13 1:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-28 3:17 [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary yuchenlin
2018-09-05 2:14 ` yuchenlin
2018-09-12 9:34 ` Fam Zheng
2018-09-12 9:52 ` yuchenlin
2018-09-12 11:52 ` Fam Zheng
2018-09-12 11:54 ` Fam Zheng
2018-09-13 1:51 ` yuchenlin
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.