[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 6/8] quorum: Avoid bdrv_aio_writev() for rew
Re: [Qemu-devel] [RFC PATCH 6/8] quorum: Avoid bdrv_aio_writev() for rewrites
Fri, 18 Nov 2016 15:11:22 -0600
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0
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
> 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 */
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:
>>> + /* 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
>> 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.
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Description: OpenPGP digital signature
Re: [Qemu-devel] [RFC PATCH 0/8] quorum: Implement .bdrv_co_preadv/pwritev(), Paolo Bonzini, 2016/11/11
Re: [Qemu-devel] [RFC PATCH 0/8] quorum: Implement .bdrv_co_preadv/pwritev(), no-reply, 2016/11/12