| Message ID | 20180828031751.29507-1-yuchenlin@synology.com |
|---|---|
| State | New |
| Headers | show |
| Series | vmdk: align end of file to a sector boundary | expand |
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
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
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
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 >
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 >
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 >>
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); }