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

[Jeff Cody]

On Wed, Jan 22, 2014 at 09:54:26AM +0800, Fam Zheng wrote:
> On Tue, 01/21 11:31, 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 <address@hidden>
> > ---
> >  block/mirror.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 2932bab..528b61a 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -630,11 +630,47 @@ 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);
> > +    base_length = bdrv_getlength(base);
> > +
> > +    if (length < 0 || base_length < 0) {
> 
> I prefer to add an error to errp here, at least tell which bdrv_getlength
> failed helps.
>

OK, I can do that.

> > +        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;
> > +    }
> > +
> > +
> >      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);
> 
> Well, I hope this never fail. (But if it does, should we do anything else than
> ignoring it?)
>

Me too :) There isn't a lot we can do but ignore it; the original
error that lead us here is most likely more important and informative
than the reopen error, so I don't think it makes sense to override
errp.  And failing to restore the original flags is likely not
catastrophic (but symptomatic).

> 
> > +    return;
> >  }
> > -- 
> > 1.8.3.1
> >