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

[Kevin Wolf]

Am 17.11.2016 um 15:54 hat Alberto Garcia geschrieben:
> On Thu 10 Nov 2016 06:19:07 PM CET, Kevin Wolf wrote:
> > +static void quorum_rewrite_entry(void *opaque)
> > +{
> > +    QuorumCo *co = opaque;
> > +    QuorumAIOCB *acb = co->acb;
> > +    BDRVQuorumState *s = acb->bs->opaque;
> > +    int ret;
> > +
> > +    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.

> > +    /* 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.

If you feel strongly about this, I will change it.

Kevin