block: commits of snapshots larger than backing files
cover block: commits of snapshots larger than backing files
If a snapshot is larger than a backing file, then the offline bdrv_commit and the live active layer commit will fail with an i/o error (usually). A live commit of a non-active layer will complete successfully, as it runs bdrv_truncate() on the backing image to resize it to the larger size.
For both bdrv_commit() and commit_active_start(), this series will resize the underlying base image if needed. If the resize fails, an error will be returned.
Jeff Cody (2):
block: resize backing file image during offline commit, if necessary
block: resize backing image during active layer commit, if needed
block.c | 18 +++++++++++++++--- block/mirror.c | 13 +++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-)
Signature · 1 line
1.8.3.1
PATCH 1/2 block: resize backing file image during offline commit, if necessary
Currently, if an image file is logically larger than its backing file, commiting it via 'qemu-img commit' will fail.
For instance, if we have a base image with a virtual size 10G, and a snapshot image of size 20G, then committing the snapshot offline with 'qemu-img commit' will likely fail.
This will automatically attempt to resize the base image, if the snapshot image to be committed is larger.
--- block.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
block.c +15 -3
@@ -1876,10 +1876,10 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix)int bdrv_commit (BlockDriverState *bs) {BlockDriver *drv = bs->drv; -int64_t sector, total_sectors; +int64_t sector, total_sectors, length;int n, ro, open_flags;int ret =0 ; -uint8_t *buf; +uint8_t *buf =NULL ;char filename[PATH_MAX ];if (!drv)
@@ -1904,7 +1904,19 @@ int bdrv_commit(BlockDriverState *bs) } } - total_sectors =bdrv_getlength (bs) >>BDRV_SECTOR_BITS ; + length =bdrv_getlength (bs); + +/* If our top snapshot is larger than the backing file image, +* grow the backing file image if possible. If not possible, +* we must return an error */ +if (length >bdrv_getlength (bs->backing_hd)) { + ret =bdrv_truncate (bs->backing_hd, length); +if (ret <0 ) { + goto ro_cleanup; + } + } + + total_sectors = length >>BDRV_SECTOR_BITS ; buf =g_malloc (COMMIT_BUF_SECTORS *BDRV_SECTOR_SIZE );for (sector =0 ; sector < total_sectors; sector += n) {
Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH 1/2] block: resize backing file image during offline commit, if necessary
block.c int bdrv_commit(BlockDriverState *bs) · line 1913
Quoted 12 lines · Jeff Cody
block.c +7
-1@@ -1904,7 +1904,19 @@ int bdrv_commit(BlockDriverState *bs) } } - total_sectors =bdrv_getlength (bs) >>BDRV_SECTOR_BITS ; + length =bdrv_getlength (bs); + +/* If our top snapshot is larger than the backing file image, +* grow the backing file image if possible. If not possible, +* we must return an error */ +if (length >bdrv_getlength (bs->backing_hd)) { + ret =bdrv_truncate (bs->backing_hd, length);
bdrv_getlength() is allowed to return -errno. It's unlikely here but we should at least check bdrv_getlength(bs), probably also bdrv_getlength(bs->backing_hd).
PATCH 2/2 block: resize backing image during active layer commit, if needed
In reply to Jeff CodyIf the top image to commit is the active layer, and also larger than the base image, then an I/O error will likely be returned during block-commit.
For instance, if we have a base image with a virtual size 10G, and a active layer image of size 20G, then committing the snapshot via 'block-commit' will likely fail.
This will automatically attempt to resize the base image, if the active layer image to be committed is larger.
--- block/mirror.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
block/mirror.c +13 -0
@@ -630,9 +630,22 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, BlockDriverCompletionFunc *cb, void *opaque, Error **errp) { +int64_t length;if (bdrv_reopen (base, bs->open_flags, errp)) {return ; } + + length =bdrv_getlength (bs); + +if (length >bdrv_getlength (base)) { +if (bdrv_truncate (base, length) <0 ) { +error_setg (errp,"Top image %s is larger than base image %s, and " +"resize of base image failed." , + bs->filename, base->filename); +return ; + } + } +bdrv_ref (base); mirror_start_job(bs, base, speed, 0, 0, on_error, on_error, cb, opaque, errp,
Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH 2/2] block: resize backing image during active layer commit, if needed
Quoted 21 lines · Jeff Cody
If the top image to commit is the active layer, and also larger than the base image, then an I/O error will likely be returned during block-commit.
For instance, if we have a base image with a virtual size 10G, and a active layer image of size 20G, then committing the snapshot via 'block-commit' will likely fail.
This will automatically attempt to resize the base image, if the active layer image to be committed is larger.
Signed-off-by Jeff Cody <[email protected]>--- block/mirror.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)block/mirror.c +0
-0
block/mirror.c void commit_active_start(BlockDriverState *bs, BlockDriverState *base, · line 636
Quoted 7 lines
block/mirror.c +1
-0@@ -630,9 +630,22 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, BlockDriverCompletionFunc *cb, void *opaque, Error **errp) { +int64_t length;if (bdrv_reopen (base, bs->open_flags, errp)) {return ; }
"base" is already reopened here.
block/mirror.c void commit_active_start(BlockDriverState *bs, BlockDriverState *base, · line 645
Quoted 9 lines
block/mirror.c +9
-0+ + length =bdrv_getlength (bs); + +if (length >bdrv_getlength (base)) { +if (bdrv_truncate (base, length) <0 ) { +error_setg (errp,"Top image %s is larger than base image %s, and " +"resize of base image failed." , + bs->filename, base->filename); +return ;
Should we restore open flags for base?
Thanks,
Fam
Quoted 9 lines
block/mirror.c +3
-0+ } + } +bdrv_ref (base); mirror_start_job(bs, base, speed, 0, 0, on_error, on_error, cb, opaque, errp,Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH 2/2] block: resize backing image during active layer commit, if needed
Quoted 32 lines · Fam Zheng
Quoted 21 lines · Jeff Cody
If the top image to commit is the active layer, and also larger than the base image, then an I/O error will likely be returned during block-commit.
For instance, if we have a base image with a virtual size 10G, and a active layer image of size 20G, then committing the snapshot via 'block-commit' will likely fail.
This will automatically attempt to resize the base image, if the active layer image to be committed is larger.
Signed-off-by Jeff Cody <[email protected]>--- block/mirror.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)block/mirror.c +0
-0block/mirror.c void commit_active_start(BlockDriverState *bs, BlockDriverState *base, · line 636
Quoted 7 lines
block/mirror.c +1
-0@@ -630,9 +630,22 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, BlockDriverCompletionFunc *cb, void *opaque, Error **errp) { +int64_t length;if (bdrv_reopen (base, bs->open_flags, errp)) {return ; }"base" is already reopened here.
Quoted 12 lines
block/mirror.c void commit_active_start(BlockDriverState *bs, BlockDriverState *base, · line 645
Quoted 9 lines
block/mirror.c +9
-0+ + length =bdrv_getlength (bs); + +if (length >bdrv_getlength (base)) { +if (bdrv_truncate (base, length) <0 ) { +error_setg (errp,"Top image %s is larger than base image %s, and " +"resize of base image failed." , + bs->filename, base->filename); +return ;Should we restore open flags for base?
Good catch. Yes, I think we should; and I think we should also do it after the call to mirror_start_job, if errp is set. I'll add that in for v2.
Quoted 9 lines
Quoted 9 lines
block/mirror.c +3
-0+ } + } +bdrv_ref (base); mirror_start_job(bs, base, speed, 0, 0, on_error, on_error, cb, opaque, errp,Signature · 1 line
1.8.3.1
Quoted 8 lines · Jeff Cody
If a snapshot is larger than a backing file, then the offline
bdrv_commitand the live active layer commit will fail with an i/o error (usually). A live commit of a non-active layer will complete successfully, as it runsbdrv_truncate()on the backing image to resize it to the larger size.For both
bdrv_commit()andcommit_active_start(), this series will resize the underlying base image if needed. If the resize fails, an error will be returned.
This got me thinking about the opposite case: when the snapshot is smaller than the backing file. We leave the backing file with its original size. In practice this is "safe" because the partition and volume metadata should show the smaller size. If the user really wants to shrink the device they can still truncate after completing the "commit" operation.
Can you update the QEMU documentation to explicitly cover both snapshot
Quoted 1 line
backing and snapshot < backing cases?
Thanks,
Stefan
Quoted 10 lines · Stefan Hajnoczi
Quoted 8 lines · Jeff Cody
If a snapshot is larger than a backing file, then the offline
bdrv_commitand the live active layer commit will fail with an i/o error (usually). A live commit of a non-active layer will complete successfully, as it runsbdrv_truncate()on the backing image to resize it to the larger size.For both
bdrv_commit()andcommit_active_start(), this series will resize the underlying base image if needed. If the resize fails, an error will be returned.
Quoted 10 lines
This got me thinking about the opposite case: when the snapshot is smaller than the backing file. We leave the backing file with its original size. In practice this is "safe" because the partition and volume metadata should show the smaller size. If the user really wants to shrink the device they can still truncate after completing the "commit" operation.
Can you update the QEMU documentation to explicitly cover both snapshot
Quoted 1 line
backing and snapshot < backing cases?
Yes, no problem.