[Qemu-devel] [PATCH 0/2] block: commits of snapshots larger than backing files

[Qemu-devel] [PATCH 0/2] block: commits of snapshots larger than backing files

[Jeff Cody]

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(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] block: resize backing file image during offline commit, if necessary

[Jeff Cody]

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.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 64e7d22..5e895fa 100644
--- a/block.c
+++ b/block.c
@@ -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) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] block: resize backing image during active layer commit, if needed

[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 <jcody@redhat.com>
---
 block/mirror.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index 2932bab..c4e42fa 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -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,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] block: resize backing image during active layer commit, if needed

[Fam Zheng]

On Mon, 01/13 15:18, Jeff Cody wrote:
> 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 <jcody@redhat.com>
> ---
>  block/mirror.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 2932bab..c4e42fa 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -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.

> +
> +    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

> +        }
> +    }
> +
>      bdrv_ref(base);
>      mirror_start_job(bs, base, speed, 0, 0,
>                       on_error, on_error, cb, opaque, errp,
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH 0/2] block: commits of snapshots larger than backing files

[Stefan Hajnoczi]

On Mon, Jan 13, 2014 at 03:18:44PM -0500, Jeff Cody wrote:
> 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.

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
 > backing and snapshot < backing cases?

Thanks,
Stefan

Re: [Qemu-devel] [PATCH 1/2] block: resize backing file image during offline commit, if necessary

[Stefan Hajnoczi]

On Mon, Jan 13, 2014 at 03:18:45PM -0500, Jeff Cody wrote:
> @@ -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).

Re: [Qemu-devel] [PATCH 0/2] block: commits of snapshots larger than backing files

[Jeff Cody]

On Fri, Jan 17, 2014 at 03:17:10PM +0800, Stefan Hajnoczi wrote:
> On Mon, Jan 13, 2014 at 03:18:44PM -0500, Jeff Cody wrote:
> > 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.
> 
> 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
>  > backing and snapshot < backing cases?
>

Yes, no problem.

Re: [Qemu-devel] [PATCH 2/2] block: resize backing image during active layer commit, if needed

[Jeff Cody]

On Wed, Jan 15, 2014 at 01:58:29PM +0800, Fam Zheng wrote:
> On Mon, 01/13 15:18, Jeff Cody wrote:
> > 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 <jcody@redhat.com>
> > ---
> >  block/mirror.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 2932bab..c4e42fa 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -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.
> 
> > +
> > +    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.

> > +        }
> > +    }
> > +
> >      bdrv_ref(base);
> >      mirror_start_job(bs, base, speed, 0, 0,
> >                       on_error, on_error, cb, opaque, errp,
> > -- 
> > 1.8.3.1
> >