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