[v3,2/3] block: resize backing image during active layer commit, if needed

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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/mirror.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

Le Thursday 23 Jan 2014 à 16:48:56 (-0500), Jeff Cody a écrit :
> 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/mirror.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 2932bab..2a33aa6 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -630,11 +630,52 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
>                           BlockDriverCompletionFunc *cb,
>                           void *opaque, Error **errp)
>  {
> +    int64_t length, base_length;
> +    int orig_base_flags;
> +
> +    orig_base_flags = bdrv_get_flags(base);
> +
>      if (bdrv_reopen(base, bs->open_flags, errp)) {
>          return;
>      }
> +
> +    length = bdrv_getlength(bs);
> +    if (length < 0) {
> +        error_setg(errp, "Unable to determine length of %s", bs->filename);
> +        goto error_restore_flags;
> +    }
> +
> +    base_length = bdrv_getlength(base);
> +    if (base_length < 0) {
> +        error_setg(errp, "Unable to determine length of %s", base->filename);
> +        goto error_restore_flags;
> +    }
> +
> +    if (length > base_length) {
> +        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);
> +            goto error_restore_flags;
> +        }


> +    } else if (length < 0) {
> +        goto error_restore_flags;
> +    }
The code already test for (length < 0) ten lines above. Is this branch really
needed ?

Best regards

Benoît

> +
> +
>      bdrv_ref(base);
>      mirror_start_job(bs, base, speed, 0, 0,
>                       on_error, on_error, cb, opaque, errp,
>                       &commit_active_job_driver, false, base);
> +    if (error_is_set(errp)) {
> +        goto error_restore_flags;
> +    }
> +
> +    return;
> +
> +error_restore_flags:
> +    /* ignore error and errp for bdrv_reopen, because we want to propagate
> +     * the original error */
> +    bdrv_reopen(base, orig_base_flags, NULL);
> +    return;
>  }
> -- 
> 1.8.3.1
> 
>

On 01/23/2014 02:48 PM, 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>

> +    if (length > base_length) {
> +        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);
> +            goto error_restore_flags;
> +        }
> +    } else if (length < 0) {
> +        goto error_restore_flags;
> +    }
> +
> +
>      bdrv_ref(base);
>      mirror_start_job(bs, base, speed, 0, 0,
>                       on_error, on_error, cb, opaque, errp,
>                       &commit_active_job_driver, false, base);
> +    if (error_is_set(errp)) {
> +        goto error_restore_flags;

If we get here, bdrv_truncate() succeeded at enlarging the base file.  A
full rollback would imply adding a second bdrv_truncate() in the error
handling path to undo the growth back to the original size (of course,
ignoring failure to shrink).  But that's more complex, and probably not
worth adding in (at any rate, leaving the base file larger doesn't
affect semantics of the bytes seen by the guest, and for file formats
that handle resizing via a sparse tail doesn't even consume much
storage).  So my R-b stands.

On Thu, Jan 23, 2014 at 11:05:26PM +0100, Benoît Canet wrote:
> Le Thursday 23 Jan 2014 à 16:48:56 (-0500), Jeff Cody a écrit :
> > 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>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  block/mirror.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 2932bab..2a33aa6 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -630,11 +630,52 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
> >                           BlockDriverCompletionFunc *cb,
> >                           void *opaque, Error **errp)
> >  {
> > +    int64_t length, base_length;
> > +    int orig_base_flags;
> > +
> > +    orig_base_flags = bdrv_get_flags(base);
> > +
> >      if (bdrv_reopen(base, bs->open_flags, errp)) {
> >          return;
> >      }
> > +
> > +    length = bdrv_getlength(bs);
> > +    if (length < 0) {
> > +        error_setg(errp, "Unable to determine length of %s", bs->filename);
> > +        goto error_restore_flags;
> > +    }
> > +
> > +    base_length = bdrv_getlength(base);
> > +    if (base_length < 0) {
> > +        error_setg(errp, "Unable to determine length of %s", base->filename);
> > +        goto error_restore_flags;
> > +    }
> > +
> > +    if (length > base_length) {
> > +        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);
> > +            goto error_restore_flags;
> > +        }
> 
> 
> > +    } else if (length < 0) {
> > +        goto error_restore_flags;
> > +    }
> The code already test for (length < 0) ten lines above. Is this branch really
> needed ?
> 
> Best regards
> 
> Benoît

Thanks - no, it is not needed.  I did not mean to have that there -
while harmless, it is essentially dead code and should be culled.
I'll submit a v4 to take care of that.

> 
> > +
> > +
> >      bdrv_ref(base);
> >      mirror_start_job(bs, base, speed, 0, 0,
> >                       on_error, on_error, cb, opaque, errp,
> >                       &commit_active_job_driver, false, base);
> > +    if (error_is_set(errp)) {
> > +        goto error_restore_flags;
> > +    }
> > +
> > +    return;
> > +
> > +error_restore_flags:
> > +    /* ignore error and errp for bdrv_reopen, because we want to propagate
> > +     * the original error */
> > +    bdrv_reopen(base, orig_base_flags, NULL);
> > +    return;
> >  }
> > -- 
> > 1.8.3.1
> > 
> >