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
signature.asc
Description: PGP signature