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

[Kevin Wolf]

Am 10.10.2013 um 14:54 hat Max Reitz geschrieben:
> If the "currently" implied that this will turn out bad if there is a
> new error condition between a successful call to
> qcow2_alloc_cluster_link_l2 and the removal of the L2Meta request
> from the list: Yes, that's true, of course.

Yes, that's the scenario. It seems easy to miss the error handling path
when reviewing a change to this code.

> However, as you've said,
> currently, there is no such condition; and I don't see why it should
> be introduced. The sole purpose of the list seems to be (to me) to
> execute qcow2_alloc_cluster_link_l2 on every of its elements. Thus,
> as soon as qcow2_alloc_cluster_link_l2 is successful, the
> corresponding request should be removed from the list.

If anything isn't complex enough in qcow2, think about how things will
turn out with Delayed COW and chances are that it does become complex.

For example, you can then have other requests running in parallel which
use the newly allocated cluster. You may decrease the refcount only
after the last of them has completed. This is just the first case that
comes to mind, I'm sure there's more fun to be had.

> So, in case you do agree that it currently works fine, I would not
> consider it risky; if this patch is applied and some time in the
> future anything introduces a "goto fail" between
> qcow2_alloc_cluster_link_l2 and l2_meta = next, this patch would
> simply have to make sure that qcow2_free_clusters isn't called in
> this case. In the probably very unlikely case all my previous
> assumptions and conclusions were true, I'd just add a comment in the
> qcow2_alloc_cluster_link_l2 loop informing about this case (“If you
> add a goto fail here, make sure to pay attention” or something along
> these lines).

Adding a comment there sounds like a fair compromise.

> >Also, shouldn't it be QCOW2_DISCARD_OTHER?
> 
> I'm always unsure about the discard flags. ;-)
> 
> I try to follow the rule of “use the specific type (or ‘other’) for
> freeing ‘out of the blue’, but use ‘always’ if it's just a very
> recent allocation that is being undone again”. I'd gladly accept
> better recommendations. ;-)

To be honest, I'm not sure if there are any legitimate use cases for
'always'... Discard is a slow operation, so unless there's a specific
reason anyway, I'd default to 'other' (or a specific type, of course).

Kevin