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

[Max Reitz <mreitz@redhat.com>]

On 2013-10-10 15:57, Kevin Wolf wrote:
> 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.

Well, yes, I somehow thought of something like this; but I thought 
that'd be the problem of qcow2_alloc_cluster_link_l2 and as soon as that 
function has been called, though maybe we cannot remove it from the 
requests in flight (global for the BDS), but we still should remove it 
from the local l2meta list.

>> 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.

Okay, I'll add it.

>>> 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).

Seems easy enough to remember, thanks. ;-)

Max