Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] block: Keep strong reference when draining all BDS

[Max Reitz]

On 2017-11-10 17:22, Kevin Wolf wrote:
> Am 10.11.2017 um 17:13 hat Max Reitz geschrieben:
>> On 2017-11-10 17:05, Kevin Wolf wrote:
>>> Am 10.11.2017 um 16:23 hat Max Reitz geschrieben:
>>>> On 2017-11-10 14:32, Fam Zheng wrote:
>>>>> On Fri, 11/10 14:17, Kevin Wolf wrote:
>>>>>> Do you actually need to keep references to all BDSes in the whole list
>>>>>> while using the iterator or would it be enough to just keep a reference
>>>>>> to the current one?
>>>>>
>>>>> To fix the bug we now see I think keeping the current is enough, but I 
>>>>> think
>>>>> implementing just like this patch is also good with some future-proofing: 
>>>>> we
>>>>> cannot know what will be wedged into the nexted aio_poll()'s over time 
>>>>> (and yes,
>>>>> we should really reduce the number of them.)
>>>>
>>>> I don't really want to think about whether it's safe to only keep a
>>>> reference to the current BDS.  I can't imagine any case where destroying
>>>> one root BDS leads to destroying another, but I'd rather be safe and not
>>>> have to think about it.  (Unless there is an important reason to only
>>>> keep a strong reference to the current one.)
>>>
>>> Why would it be a problem if another BDS from the list went away? If it
>>> is one that was already processed, we don't care, and if it was in the
>>> yet unprocessed part of the list, we'll just never return it.
>>
>> You mean from bdrv_next() in its current form?  Well, I know that when I
>> just put a bdrv_ref()/bdrv_unref() pair around the drain, I got a
>> segfault in blk_all_next() in bdrv_next().  I can investigate more, but
>> that's pretty much what I mean by "I don't really want to think about it".
> 
> No, I mean a bdrv_next() that is modified to bdrv_ref() only what
> it->blk/it->bs point to currently instead of allocating a whole list.

Seems to work, I guess my issue was that I unref'd the BDS too early
(should do that only after the next pointer has been fetched...).

This also means adding a new function like bdrv_next_cleanup() that has
to be called if you don't want to iterate over all of the BDSs, but I
guess it's still less code to write.

Max

signature.asc
Description: OpenPGP digital signature