Re: [Qemu-devel] [RFC PATCH 6/8] quorum: Avoid bdrv_aio_writev() for rewrites

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2841 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]