We want to change .bdrv_co_drained_begin() back to be a non-coroutine
callback, so in preparation, avoid yielding in its implementation.

Because we increase bs->in_flight and bdrv_drained_begin() polls, the
behaviour is unchanged.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 2f36ad342c..013f826c44 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -282,9 +282,8 @@ static void coroutine_fn qed_unplug_allocating_write_reqs(BDRVQEDState *s)
     qemu_co_mutex_unlock(&s->table_lock);
 }
 
-static void coroutine_fn qed_need_check_timer_entry(void *opaque)
+static void coroutine_fn qed_need_check_timer(BDRVQEDState *s)
 {
-    BDRVQEDState *s = opaque;
     int ret;
 
     trace_qed_need_check_timer_cb(s);
@@ -310,9 +309,20 @@ static void coroutine_fn qed_need_check_timer_entry(void *opaque)
     (void) ret;
 }
 
+static void coroutine_fn qed_need_check_timer_entry(void *opaque)
+{
+    BDRVQEDState *s = opaque;
+
+    qed_need_check_timer(opaque);
+    bdrv_dec_in_flight(s->bs);
+}
+
 static void qed_need_check_timer_cb(void *opaque)
 {
+    BDRVQEDState *s = opaque;
     Coroutine *co = qemu_coroutine_create(qed_need_check_timer_entry, opaque);
+
+    bdrv_inc_in_flight(s->bs);
     qemu_coroutine_enter(co);
 }
 
@@ -363,8 +373,12 @@ static void coroutine_fn bdrv_qed_co_drain_begin(BlockDriverState *bs)
      * header is flushed.
      */
     if (s->need_check_timer && timer_pending(s->need_check_timer)) {
+        Coroutine *co;
+
         qed_cancel_need_check_timer(s);
-        qed_need_check_timer_entry(s);
+        co = qemu_coroutine_create(qed_need_check_timer_entry, s);
+        bdrv_inc_in_flight(bs);
+        aio_co_enter(bdrv_get_aio_context(bs), co);
     }
 }
 
-- 
2.38.1

On 08.11.22 13:37, Kevin Wolf wrote:
> We want to change .bdrv_co_drained_begin() back to be a non-coroutine
> callback, so in preparation, avoid yielding in its implementation.
>
> Because we increase bs->in_flight and bdrv_drained_begin() polls, the
> behaviour is unchanged.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qed.c | 20 +++++++++++++++++---
>   1 file changed, 17 insertions(+), 3 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Am 08/11/2022 um 13:37 schrieb Kevin Wolf:
> We want to change .bdrv_co_drained_begin() back to be a non-coroutine
> callback, so in preparation, avoid yielding in its implementation.
>
> Because we increase bs->in_flight and bdrv_drained_begin() polls, the
> behaviour is unchanged.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>


Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

On Tue, Nov 08, 2022 at 01:37:26PM +0100, Kevin Wolf wrote:
> @@ -310,9 +309,20 @@ static void coroutine_fn qed_need_check_timer_entry(void *opaque)
>      (void) ret;
>  }
>  
> +static void coroutine_fn qed_need_check_timer_entry(void *opaque)
> +{
> +    BDRVQEDState *s = opaque;
> +
> +    qed_need_check_timer(opaque);
> +    bdrv_dec_in_flight(s->bs);
> +}
> +
>  static void qed_need_check_timer_cb(void *opaque)
>  {
> +    BDRVQEDState *s = opaque;
>      Coroutine *co = qemu_coroutine_create(qed_need_check_timer_entry, opaque);
> +
> +    bdrv_inc_in_flight(s->bs);
>      qemu_coroutine_enter(co);
>  }
>  
> @@ -363,8 +373,12 @@ static void coroutine_fn bdrv_qed_co_drain_begin(BlockDriverState *bs)
>       * header is flushed.
>       */
>      if (s->need_check_timer && timer_pending(s->need_check_timer)) {
> +        Coroutine *co;
> +
>          qed_cancel_need_check_timer(s);
> -        qed_need_check_timer_entry(s);
> +        co = qemu_coroutine_create(qed_need_check_timer_entry, s);
> +        bdrv_inc_in_flight(bs);

Please include comments that indicate where inc/dec are paired. This is
like pairing memory barriers where it can be very hard to know after the
code has been written (and modified).

Am 09.11.2022 um 22:49 hat Stefan Hajnoczi geschrieben:
> On Tue, Nov 08, 2022 at 01:37:26PM +0100, Kevin Wolf wrote:
> > @@ -310,9 +309,20 @@ static void coroutine_fn qed_need_check_timer_entry(void *opaque)
> >      (void) ret;
> >  }
> >  
> > +static void coroutine_fn qed_need_check_timer_entry(void *opaque)
> > +{
> > +    BDRVQEDState *s = opaque;
> > +
> > +    qed_need_check_timer(opaque);
> > +    bdrv_dec_in_flight(s->bs);
> > +}
> > +
> >  static void qed_need_check_timer_cb(void *opaque)
> >  {
> > +    BDRVQEDState *s = opaque;
> >      Coroutine *co = qemu_coroutine_create(qed_need_check_timer_entry, opaque);
> > +
> > +    bdrv_inc_in_flight(s->bs);
> >      qemu_coroutine_enter(co);
> >  }
> >  
> > @@ -363,8 +373,12 @@ static void coroutine_fn bdrv_qed_co_drain_begin(BlockDriverState *bs)
> >       * header is flushed.
> >       */
> >      if (s->need_check_timer && timer_pending(s->need_check_timer)) {
> > +        Coroutine *co;
> > +
> >          qed_cancel_need_check_timer(s);
> > -        qed_need_check_timer_entry(s);
> > +        co = qemu_coroutine_create(qed_need_check_timer_entry, s);
> > +        bdrv_inc_in_flight(bs);
> 
> Please include comments that indicate where inc/dec are paired. This is
> like pairing memory barriers where it can be very hard to know after the
> code has been written (and modified).

I can do this, of course, if you like to have it in qed. However, it's
not something we're doing elsewhere.

bdrv_inc/dec_in_flight() are a lot simpler than barriers which
synchronise two completely independently running tasks. You just need to
follow the control flow from the inc() and you'll find the dec(). They
are much more similar to taking and releasing a lock than to barriers.

Callbacks always make the code a little harder to read, but personally I
think inc() before scheduling a new coroutine and then dec() at the end
of its entry function is a very obvious pattern that exists in other
places, too.

Kevin

On 11/8/22 15:37, Kevin Wolf wrote:
>       int ret;
>   
>       trace_qed_need_check_timer_cb(s);
> @@ -310,9 +309,20 @@ static void coroutine_fn qed_need_check_timer_entry(void *opaque)
>       (void) ret;
>   }
>   
> +static void coroutine_fn qed_need_check_timer_entry(void *opaque)
> +{
> +    BDRVQEDState *s = opaque;
> +
> +    qed_need_check_timer(opaque);
> +    bdrv_dec_in_flight(s->bs);

hmm, one question: don't we need aio_wait_kick() call here?

> +}
> +
>   static void qed_need_che

-- 
Best regards,
Vladimir

Am 09.11.2022 um 10:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 11/8/22 15:37, Kevin Wolf wrote:
> >       int ret;
> >       trace_qed_need_check_timer_cb(s);
> > @@ -310,9 +309,20 @@ static void coroutine_fn qed_need_check_timer_entry(void *opaque)
> >       (void) ret;
> >   }
> > +static void coroutine_fn qed_need_check_timer_entry(void *opaque)
> > +{
> > +    BDRVQEDState *s = opaque;
> > +
> > +    qed_need_check_timer(opaque);
> > +    bdrv_dec_in_flight(s->bs);
> 
> hmm, one question: don't we need aio_wait_kick() call here?

bdrv_dec_in_flight() already calls aio_wait_kick() internally, so any
places that use it don't need a separate aio_wait_kick().

Kevin

On 11/8/22 15:37, Kevin Wolf wrote:
> We want to change .bdrv_co_drained_begin() back to be a non-coroutine
> callback, so in preparation, avoid yielding in its implementation.
> 
> Because we increase bs->in_flight and bdrv_drained_begin() polls, the
> behaviour is unchanged.
> 
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir