Re: [Qemu-devel] [PATCH 07/10] qcow2: Implement .bdrv_inactivate

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3479 bytes --]

Am 22.12.2015 um 22:17 hat Eric Blake geschrieben:
> On 12/22/2015 09:46 AM, Kevin Wolf wrote:
> > The callback has to ensure that closing or flushing the image afterwards
> > wouldn't cause a write access to the image files. This means that just
> > the caches have to be written out, which is part of the existing
> > .bdrv_close implementation.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2.c | 45 ++++++++++++++++++++++++++++-----------------
> >  1 file changed, 28 insertions(+), 17 deletions(-)
> > 
> 
> Mostly code motion, but I still ended up with questions.
> 
> >  
> > +static int qcow2_inactivate(BlockDriverState *bs)
> > +{
> > +    BDRVQcow2State *s = bs->opaque;
> > +    int ret, result = 0;
> > +
> > +    ret = qcow2_cache_flush(bs, s->l2_table_cache);
> > +    if (ret) {
> > +        result = ret;
> > +        error_report("Failed to flush the L2 table cache: %s",
> > +                     strerror(-ret));
> 
> I asked Markus if we want error_report_errno() - and ever since then, I
> keep finding more and more uses that would benefit from it :)

At least they should be easy to find if we ever do introduce it.

> > +    }
> > +
> > +    ret = qcow2_cache_flush(bs, s->refcount_block_cache);
> > +    if (ret) {
> > +        result = ret;
> > +        error_report("Failed to flush the refcount block cache: %s",
> > +                     strerror(-ret));
> > +    }
> > +
> 
> If the media fails in between these two statements,...
> 
> > +    if (result == 0) {
> > +        qcow2_mark_clean(bs);
> 
> ...can't qcow2_mark_clean() fail due to an EIO or other write error?  Do
> we care?  I guess the worst is that we didn't mark the image clean after
> all, which is no worse than if qemu[-img] had been SIGKILL'd at the same
> point where I hypothesized that the media could fail.

Exactly. We can't avoid that disks fail, but we can make sure that we do
things in the right order so that the failure can't result in
corruption. Marking the image clean as the last thing should be the
right order to ensure this.

> > +    }
> > +
> > +    return result;
> 
> If both flushes failed, you return the result set to the return value of
> the second flush.  Is it ever possible that the return value of the
> first flush might be more useful?

I can imagine that there are such cases. But if both fail, we don't have
a way to tell which error code is more useful, so we can just pick any.
We have an error_report() for both, at least, in case the admin needs to
check what happened.

> > @@ -1693,23 +1719,7 @@ static void qcow2_close(BlockDriverState *bs)
> >      s->l1_table = NULL;
> >  
> >      if (!(bs->open_flags & BDRV_O_INCOMING)) {
> 
> > -        if (!ret1 && !ret2) {
> > -            qcow2_mark_clean(bs);
> > -        }
> > +        qcow2_inactivate(bs);
> >      }
> 
> Then again, the lone existing caller in this addition doesn't even care
> about the return value.  The only other caller was your new code added
> earlier in the series; in 5/10 migration_completion(), which uses the
> value as a conditional but doesn't try to call strerror(-ret).
> 
> Since this is mostly code motion, any semantic changes you would want to
> make based on my questions above probably belong in their own patches.
> Therefore, for this patch as written,
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]