Re: [Qemu-devel] [PATCH 1/3] block: add BlockBackend->in_flight counter
On Fri, Feb 09, 2018 at 06:26:41PM +0100, Paolo Bonzini wrote:
> On 09/02/2018 18:23, Kevin Wolf wrote:
> > Am 09.02.2018 um 17:28 hat Paolo Bonzini geschrieben:
> >> On 08/02/2018 18:18, Stefan Hajnoczi wrote:
> >>> +BlockDriverState *bs = blk_bs(blk);
> >>> +
> >>> +if (bs) {
> >>> +bdrv_drained_begin(bs);
> >>> +}
> >>> +
> >>> +/* We may have aio requests like -ENOMEDIUM in flight */
> >>> +while (atomic_mb_read(&blk->in_flight) > 0) {
> >>> +aio_poll(blk_get_aio_context(blk), true);
> >>> +}
> >>> +
> >>> +if (bs) {
> >>> +bdrv_drained_end(bs);
> >>
> >> This only works if you are in the default AioContext, otherwise you can
> >> run handlers for one AioContexts in two threads.
> >
> > Should aio_poll() assert that it's called in the right thread to make
> > sure we obey the rules?
> >
> >> bdrv_dec_in_flight uses bdrv_wakeup and BDRV_POLL_WHILE to ensure that
> >> this doesn't happen, so there would be more code that you have to copy
> >> into block-backend.c.
> >
> > Instead of copying, can't we generalise it into a POLL_WHILE(ctx,
> > wakeup, cond) and make BDRV_POLL_WHILE() a wrapper for that?
>
> Yes, or even move bdrv_wakeup to AioContext would do. We already have
> block layer-specific fields such as the linux-aio state(*).
>
> (*) though now that linux-aio has been improved to do something
> like epoll, there may be a better reason to place linux-aio
> state in AioContext.
Thanks for the ideas, will fix in v2.
Stefan
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH 1/3] block: add BlockBackend->in_flight counter
On 09/02/2018 18:23, Kevin Wolf wrote:
> Am 09.02.2018 um 17:28 hat Paolo Bonzini geschrieben:
>> On 08/02/2018 18:18, Stefan Hajnoczi wrote:
>>> +BlockDriverState *bs = blk_bs(blk);
>>> +
>>> +if (bs) {
>>> +bdrv_drained_begin(bs);
>>> +}
>>> +
>>> +/* We may have aio requests like -ENOMEDIUM in flight */
>>> +while (atomic_mb_read(&blk->in_flight) > 0) {
>>> +aio_poll(blk_get_aio_context(blk), true);
>>> +}
>>> +
>>> +if (bs) {
>>> +bdrv_drained_end(bs);
>>
>> This only works if you are in the default AioContext, otherwise you can
>> run handlers for one AioContexts in two threads.
>
> Should aio_poll() assert that it's called in the right thread to make
> sure we obey the rules?
>
>> bdrv_dec_in_flight uses bdrv_wakeup and BDRV_POLL_WHILE to ensure that
>> this doesn't happen, so there would be more code that you have to copy
>> into block-backend.c.
>
> Instead of copying, can't we generalise it into a POLL_WHILE(ctx,
> wakeup, cond) and make BDRV_POLL_WHILE() a wrapper for that?
Yes, or even move bdrv_wakeup to AioContext would do. We already have
block layer-specific fields such as the linux-aio state(*).
(*) though now that linux-aio has been improved to do something
like epoll, there may be a better reason to place linux-aio
state in AioContext.
Paolo
Re: [Qemu-devel] [PATCH 1/3] block: add BlockBackend->in_flight counter
Am 09.02.2018 um 17:28 hat Paolo Bonzini geschrieben:
> On 08/02/2018 18:18, Stefan Hajnoczi wrote:
> > +BlockDriverState *bs = blk_bs(blk);
> > +
> > +if (bs) {
> > +bdrv_drained_begin(bs);
> > +}
> > +
> > +/* We may have aio requests like -ENOMEDIUM in flight */
> > +while (atomic_mb_read(&blk->in_flight) > 0) {
> > +aio_poll(blk_get_aio_context(blk), true);
> > +}
> > +
> > +if (bs) {
> > +bdrv_drained_end(bs);
>
> This only works if you are in the default AioContext, otherwise you can
> run handlers for one AioContexts in two threads.
Should aio_poll() assert that it's called in the right thread to make
sure we obey the rules?
> bdrv_dec_in_flight uses bdrv_wakeup and BDRV_POLL_WHILE to ensure that
> this doesn't happen, so there would be more code that you have to copy
> into block-backend.c.
Instead of copying, can't we generalise it into a POLL_WHILE(ctx,
wakeup, cond) and make BDRV_POLL_WHILE() a wrapper for that?
Kevin
Re: [Qemu-devel] [PATCH 1/3] block: add BlockBackend->in_flight counter
On 08/02/2018 18:18, Stefan Hajnoczi wrote:
> +BlockDriverState *bs = blk_bs(blk);
> +
> +if (bs) {
> +bdrv_drained_begin(bs);
> +}
> +
> +/* We may have aio requests like -ENOMEDIUM in flight */
> +while (atomic_mb_read(&blk->in_flight) > 0) {
> +aio_poll(blk_get_aio_context(blk), true);
> +}
> +
> +if (bs) {
> +bdrv_drained_end(bs);
This only works if you are in the default AioContext, otherwise you can
run handlers for one AioContexts in two threads.
bdrv_dec_in_flight uses bdrv_wakeup and BDRV_POLL_WHILE to ensure that
this doesn't happen, so there would be more code that you have to copy
into block-backend.c.
Paolo
Re: [Qemu-devel] [PATCH 1/3] block: add BlockBackend->in_flight counter
On 02/08/2018 11:18 AM, Stefan Hajnoczi wrote: BlockBackend currently relies on BlockDriverState->in_flight to track requests for blk_drain(). There is a corner case where BlockDriverState->in_flight cannot be used though: blk->root can be NULL when there is no medium. This results in a segfault when the NULL pointer is dereferenced. Introduce a BlockBackend->in_flight counter for aio requests so it works even when blk->root == NULL. Based on a patch by Kevin Wolf . Signed-off-by: Kevin Wolf Signed-off-by: Stefan Hajnoczi --- block.c | 2 +- block/block-backend.c | 59 +-- 2 files changed, 53 insertions(+), 8 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
