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

[Kevin Wolf <kwolf@redhat.com>]

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?

Also, shouldn't it be QCOW2_DISCARD_OTHER?

Kevin