On 11/18/2016 06:21 AM, Kevin Wolf wrote: >>> + ret = bdrv_co_pwritev(s->children[co->i], >>> + acb->sector_num * BDRV_SECTOR_SIZE, >>> + acb->nb_sectors * BDRV_SECTOR_SIZE, >>> + acb->qiov, 0); >>> + (void) ret; >> >> Why do you need 'ret' at all? If it's a placeholder to remind us to do >> something with this value in the future, you can simply add a FIXME >> comment. > > I'm not sure whether we want to fix anything, it looks intentional to > me. I just wanted to be explicit about the ignored return value, both > for human readers and for tools like Coverity.
In bdrv_co_flush(), we have: /* Return value is ignored - it's ok if wait queue is empty */ qemu_co_queue_next(&bs->flush_queue); I don't know if Coverity would squawk, but the cast to void looks a bit stranger to me than a comment, where what we did in bdrv_co_flush() seems reasonable. There's also the patch proposal to introduce ignore_value() in place of a cast to void, which is a bit more self-documenting about places that intentionally ignore a return value while still shutting Coverity up: https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05165.html > >>> + /* one less rewrite to do */ >>> + acb->rewrite_count--; >>> + qemu_coroutine_enter_if_inactive(acb->co); >> >> I think you should only enter acb->co when acb->rewrite_count reaches >> zero. >> >> In all other cases the main coroutine simply iterates inside the while() >> loop, verifies that the number is still positive and yields again. >> >> The same applies to all other cases of qemu_coroutine_enter_if_inactive, >> by the way (I failed to notice it in patch #5). > > I think I like it better this way because it keeps the loop condition > local to the caller instead of spreading it across the caller and the > places that reenter. On the other hand, I can see that not doing the > extra context switch might be a little more efficient. Do we have a feel for how many context switches this would save? If it's in the noise, cleaner code is probably a win; but if it is a hotspot, then we should definitely try the optimization. > > If you feel strongly about this, I will change it. > > Kevin > > -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature