Re: [Qemu-devel] [PATCH 1/2] qcow2: Undo leaked allocations in co_writev

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, Oct 10, 2013 at 02:26:25PM +0200, Kevin Wolf wrote:
> Am 10.10.2013 um 10:52 hat Max Reitz geschrieben:
> > If the write request spans more than one L2 table,
> > qcow2_alloc_cluster_offset cannot handle the required allocations
> > atomically. This results in leaks if it allocated new clusters in any
> > but the last L2 table touched and an error occurs in qcow2_co_writev
> > before having established the L2 link. These non-atomic allocations
> > were, however, indeed successful and are therefore given to the caller
> > in the L2Meta list.
> > 
> > If an error occurs in qcow2_co_writev and the L2Meta list is unwound,
> > all its remaining entries are clusters whose L2 links were not yet
> > established. Thus, all allocations in that list should be undone.
> > 
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  block/qcow2.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index b2489fb..6bedd5d 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1017,6 +1017,13 @@ fail:
> >      while (l2meta != NULL) {
> >          QCowL2Meta *next;
> >  
> > +        /* Undo all leaked allocations */
> > +        if (l2meta->nb_clusters != 0) {
> > +            qcow2_free_clusters(bs, l2meta->alloc_offset,
> > +                                l2meta->nb_clusters << s->cluster_bits,
> > +                                QCOW2_DISCARD_ALWAYS);
> > +        }
> > +
> >          if (l2meta->nb_clusters != 0) {
> >              QLIST_REMOVE(l2meta, next_in_flight);
> >          }
> 
> This feels a bit risky.
> 
> I think currently it does work, because qcow2_alloc_cluster_link_l2()
> can only return an error when it didn't update the L2 entry in the cache
> yet, but adding any error condition between that point and the L2Meta
> unwinding would result in corruption. I'm unsure, but perhaps a cluster
> leak is the lesser evil. Did you consider this? Do other people have an
> opinion on it?

I agree it's easy to make things worse by trying to free clusters in an
error path.  I find these types of changes a lot of effort (to write and
review properly) for relatively little gain.

Stefan