Am 18.11.2016 um 22:11 hat Eric Blake geschrieben: > 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
If you don't like the void cast, I can remove it. After all, the bdrv_aio_writev() call in the original code didn't have it either. I'm just surprised that it feels strange to both of you, I thought it was a well-known idiom for "this value is intentionally unused". > > > >>> + /* 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. Should normally be (num_children - 1) per request. With inconsistent results and enabled rewrites, add another one for each child that needs to be corrected. Kevin
pgpUgEGPaT7PH.pgp
Description: PGP signature
