Re: [PATCH] blkdebug: ignore invalid rules in non-coroutine context

2023-01-17 Thread Kevin Wolf 
Am 17.01.2023 um 16:44 hat Kevin Wolf geschrieben:
> Am 15.12.2022 um 14:02 hat Paolo Bonzini geschrieben:
> > blkdebug events can be called from either non-coroutine or coroutine
> > contexts.  However, suspend actions only make sense from within
> > a coroutine.  Currently, using those action would lead to an abort() in
> > qemu_coroutine_yield() ("Co-routine is yielding to no one").  Catch them
> > and print an error instead.
> > 
> > Signed-off-by: Paolo Bonzini 
> > ---
> >  block.c  |  2 +-
> >  block/blkdebug.c | 10 --
> >  include/block/block-io.h |  2 +-
> >  include/block/block_int-common.h |  3 ++-
> >  4 files changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 3f2bd128570e..49c66475c73e 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -6334,7 +6334,7 @@ BlockStatsSpecific 
> > *bdrv_get_specific_stats(BlockDriverState *bs)
> >  return drv->bdrv_get_specific_stats(bs);
> >  }
> >  
> > -void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event)
> > +void coroutine_mixed_fn bdrv_debug_event(BlockDriverState *bs, 
> > BlkdebugEvent event)
> >  {
> >  IO_CODE();
> >  if (!bs || !bs->drv || !bs->drv->bdrv_debug_event) {
> > diff --git a/block/blkdebug.c b/block/blkdebug.c
> > index 4265ca125e25..ce297961b7db 100644
> > --- a/block/blkdebug.c
> > +++ b/block/blkdebug.c
> > @@ -31,6 +31,7 @@
> >  #include "block/qdict.h"
> >  #include "qemu/module.h"
> >  #include "qemu/option.h"
> > +#include "qemu/error-report.h"
> >  #include "qapi/qapi-visit-block-core.h"
> >  #include "qapi/qmp/qdict.h"
> >  #include "qapi/qmp/qlist.h"
> > @@ -837,7 +838,7 @@ static void process_rule(BlockDriverState *bs, struct 
> > BlkdebugRule *rule,
> >  }
> >  }
> >  
> > -static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
> > +static void coroutine_mixed_fn blkdebug_debug_event(BlockDriverState *bs, 
> > BlkdebugEvent event)
> >  {
> >  BDRVBlkdebugState *s = bs->opaque;
> >  struct BlkdebugRule *rule, *next;
> > @@ -855,7 +856,12 @@ static void blkdebug_debug_event(BlockDriverState *bs, 
> > BlkdebugEvent event)
> >  }
> >  
> >  while (actions_count[ACTION_SUSPEND] > 0) {
> > -qemu_coroutine_yield();
> > +if (qemu_in_coroutine()) {
> > +qemu_coroutine_yield();
> > +} else {
> > +error_report("Non-coroutine event %s cannot suspend\n",
> > + BlkdebugEvent_lookup.array[event]);
> 
> error_report() already adds a newline, so we shouldn't have an "\n"
> here.
> 
> > +}
> >  actions_count[ACTION_SUSPEND]--;
> >  }
> >  }
> 
> Thanks, fixed this up and applied to the block branch.

In fact, this conflicts with a patch in my series:

[PATCH v2 13/14] block: Convert bdrv_debug_event() to co_wrapper_mixed

Resolving the conflict essentially reverts this one because after that
patch it actually is a coroutine_fn. So I may just drop this one agian.

Kevin

Re: [PATCH] blkdebug: ignore invalid rules in non-coroutine context

2023-01-17 Thread Kevin Wolf 
Am 15.12.2022 um 14:02 hat Paolo Bonzini geschrieben:
> blkdebug events can be called from either non-coroutine or coroutine
> contexts.  However, suspend actions only make sense from within
> a coroutine.  Currently, using those action would lead to an abort() in
> qemu_coroutine_yield() ("Co-routine is yielding to no one").  Catch them
> and print an error instead.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block.c  |  2 +-
>  block/blkdebug.c | 10 --
>  include/block/block-io.h |  2 +-
>  include/block/block_int-common.h |  3 ++-
>  4 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3f2bd128570e..49c66475c73e 100644
> --- a/block.c
> +++ b/block.c
> @@ -6334,7 +6334,7 @@ BlockStatsSpecific 
> *bdrv_get_specific_stats(BlockDriverState *bs)
>  return drv->bdrv_get_specific_stats(bs);
>  }
>  
> -void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event)
> +void coroutine_mixed_fn bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent 
> event)
>  {
>  IO_CODE();
>  if (!bs || !bs->drv || !bs->drv->bdrv_debug_event) {
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 4265ca125e25..ce297961b7db 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -31,6 +31,7 @@
>  #include "block/qdict.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
> +#include "qemu/error-report.h"
>  #include "qapi/qapi-visit-block-core.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qlist.h"
> @@ -837,7 +838,7 @@ static void process_rule(BlockDriverState *bs, struct 
> BlkdebugRule *rule,
>  }
>  }
>  
> -static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
> +static void coroutine_mixed_fn blkdebug_debug_event(BlockDriverState *bs, 
> BlkdebugEvent event)
>  {
>  BDRVBlkdebugState *s = bs->opaque;
>  struct BlkdebugRule *rule, *next;
> @@ -855,7 +856,12 @@ static void blkdebug_debug_event(BlockDriverState *bs, 
> BlkdebugEvent event)
>  }
>  
>  while (actions_count[ACTION_SUSPEND] > 0) {
> -qemu_coroutine_yield();
> +if (qemu_in_coroutine()) {
> +qemu_coroutine_yield();
> +} else {
> +error_report("Non-coroutine event %s cannot suspend\n",
> + BlkdebugEvent_lookup.array[event]);

error_report() already adds a newline, so we shouldn't have an "\n"
here.

> +}
>  actions_count[ACTION_SUSPEND]--;
>  }
>  }

Thanks, fixed this up and applied to the block branch.

Kevin

Re: [PATCH] blkdebug: ignore invalid rules in non-coroutine context

2022-12-15 Thread Kevin Wolf 
Am 15.12.2022 um 14:02 hat Paolo Bonzini geschrieben:
> blkdebug events can be called from either non-coroutine or coroutine
> contexts.  However, suspend actions only make sense from within
> a coroutine.  Currently, using those action would lead to an abort() in
> qemu_coroutine_yield() ("Co-routine is yielding to no one").  Catch them
> and print an error instead.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block.c  |  2 +-
>  block/blkdebug.c | 10 --
>  include/block/block-io.h |  2 +-
>  include/block/block_int-common.h |  3 ++-
>  4 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3f2bd128570e..49c66475c73e 100644
> --- a/block.c
> +++ b/block.c
> @@ -6334,7 +6334,7 @@ BlockStatsSpecific 
> *bdrv_get_specific_stats(BlockDriverState *bs)
>  return drv->bdrv_get_specific_stats(bs);
>  }
>  
> -void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event)
> +void coroutine_mixed_fn bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent 
> event)

coroutine_mixed_fn isn't a thing. I assume this depends on some patch
you haven't sent yet?

Kevin

[PATCH] blkdebug: ignore invalid rules in non-coroutine context

2022-12-15 Thread Paolo Bonzini 
blkdebug events can be called from either non-coroutine or coroutine
contexts.  However, suspend actions only make sense from within
a coroutine.  Currently, using those action would lead to an abort() in
qemu_coroutine_yield() ("Co-routine is yielding to no one").  Catch them
and print an error instead.

Signed-off-by: Paolo Bonzini 
---
 block.c  |  2 +-
 block/blkdebug.c | 10 --
 include/block/block-io.h |  2 +-
 include/block/block_int-common.h |  3 ++-
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 3f2bd128570e..49c66475c73e 100644
--- a/block.c
+++ b/block.c
@@ -6334,7 +6334,7 @@ BlockStatsSpecific 
*bdrv_get_specific_stats(BlockDriverState *bs)
 return drv->bdrv_get_specific_stats(bs);
 }
 
-void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event)
+void coroutine_mixed_fn bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent 
event)
 {
 IO_CODE();
 if (!bs || !bs->drv || !bs->drv->bdrv_debug_event) {
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 4265ca125e25..ce297961b7db 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -31,6 +31,7 @@
 #include "block/qdict.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/error-report.h"
 #include "qapi/qapi-visit-block-core.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
@@ -837,7 +838,7 @@ static void process_rule(BlockDriverState *bs, struct 
BlkdebugRule *rule,
 }
 }
 
-static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
+static void coroutine_mixed_fn blkdebug_debug_event(BlockDriverState *bs, 
BlkdebugEvent event)
 {
 BDRVBlkdebugState *s = bs->opaque;
 struct BlkdebugRule *rule, *next;
@@ -855,7 +856,12 @@ static void blkdebug_debug_event(BlockDriverState *bs, 
BlkdebugEvent event)
 }
 
 while (actions_count[ACTION_SUSPEND] > 0) {
-qemu_coroutine_yield();
+if (qemu_in_coroutine()) {
+qemu_coroutine_yield();
+} else {
+error_report("Non-coroutine event %s cannot suspend\n",
+ BlkdebugEvent_lookup.array[event]);
+}
 actions_count[ACTION_SUSPEND]--;
 }
 }
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 1fa717a545a0..0e7032a23936 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -175,7 +175,7 @@ void *qemu_try_blockalign0(BlockDriverState *bs, size_t 
size);
 void bdrv_enable_copy_on_read(BlockDriverState *bs);
 void bdrv_disable_copy_on_read(BlockDriverState *bs);
 
-void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event);
+void coroutine_mixed_fn bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent 
event);
 
 #define BLKDBG_EVENT(child, evt) \
 do { \
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index c34c525fa6ba..1d4fd5094a5b 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -726,7 +726,8 @@ struct BlockDriver {
 int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_check)(
 BlockDriverState *bs, BdrvCheckResult *result, BdrvCheckMode fix);
 
-void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent event);
+void coroutine_mixed_fn (*bdrv_debug_event)(BlockDriverState *bs,
+BlkdebugEvent event);
 
 /* io queue for linux-aio */
 void (*bdrv_io_plug)(BlockDriverState *bs);
-- 
2.38.1

Re: [PATCH] blkdebug: ignore invalid rules in non-coroutine context

2022-10-13 Thread Paolo Bonzini 

On 10/13/22 15:28, Markus Armbruster wrote:

Let's have another look at the remaining patch hunk:

 @@ -858,7 +864,12 @@ static void blkdebug_debug_event(BlockDriverState 
*bs, BlkdebugEvent event)
  }

  while (actions_count[ACTION_SUSPEND] > 0) {
 -qemu_coroutine_yield();
 +if (qemu_in_coroutine()) {
 +qemu_coroutine_yield();
 +} else {
 +error_report("Non-coroutine event %s cannot suspend\n",
 + BlkdebugEvent_lookup.array[event]);
 +}
  actions_count[ACTION_SUSPEND]--;
  }
  }

If I understand this correctly, the user asked us to suspend, but it now
turns out suspend doesn't make sense, so we ignore the request.
Correct?


Yes.


warn_report()?  info_report()?


Sure, warn_report() can work too.

Paolo

Re: [PATCH] blkdebug: ignore invalid rules in non-coroutine context

2022-10-13 Thread Markus Armbruster 
Paolo Bonzini  writes:

> On 10/13/22 12:56, Markus Armbruster wrote:
>> rule_check() is called from blkdebug_co_preadv(), blkdebug_co_pwritev(),
>> blkdebug_co_pwrite_zeroes(), blkdebug_co_pdiscard(),
>> blkdebug_co_block_status() (all marked coroutine_fn), and
>> blkdebug_co_flush() (which looks like it should be marked coroutine_fn).
>
> Yes (separate patch sent, 
> https://lore.kernel.org/qemu-devel/20221013123711.620631-11-pbonz...@redhat.com/T/#u).
>
>> Ignorant question: how could it be called outside coroutine context?
>
> You're right, only blkdebug_debug_event() can be called outside coroutine 
> context.  I confused process_rule() (called by 
> blkdebug_debug_event(), both inside and outside coroutine context) with 
> rule_check() (called in coroutine context).

Let's drop the rule_check() hunk then.

>> Also, code smell: reporting an error without taking an error path.  But
>> let's worry about that only after I understand the problem you're trying
>> to fix.
>
> Unfortunately there's no way to know in advance if an event will be called 
> inside vs. outside a coroutine.  I can keep the abort() if you 
> think it's preferrable, so what you get is still a crash but with a nicer 
> error message.  Since this is debugging code either solution has 
> pros and cons.

Let's have another look at the remaining patch hunk:

@@ -858,7 +864,12 @@ static void blkdebug_debug_event(BlockDriverState *bs, 
BlkdebugEvent event)
 }

 while (actions_count[ACTION_SUSPEND] > 0) {
-qemu_coroutine_yield();
+if (qemu_in_coroutine()) {
+qemu_coroutine_yield();
+} else {
+error_report("Non-coroutine event %s cannot suspend\n",
+ BlkdebugEvent_lookup.array[event]);
+}
 actions_count[ACTION_SUSPEND]--;
 }
 }

If I understand this correctly, the user asked us to suspend, but it now
turns out suspend doesn't make sense, so we ignore the request.
Correct?

warn_report()?  info_report()?

Re: [PATCH] blkdebug: ignore invalid rules in non-coroutine context

2022-10-13 Thread Paolo Bonzini 

On 10/13/22 12:56, Markus Armbruster wrote:

rule_check() is called from blkdebug_co_preadv(), blkdebug_co_pwritev(),
blkdebug_co_pwrite_zeroes(), blkdebug_co_pdiscard(),
blkdebug_co_block_status() (all marked coroutine_fn), and
blkdebug_co_flush() (which looks like it should be marked coroutine_fn).


Yes (separate patch sent, 
https://lore.kernel.org/qemu-devel/20221013123711.620631-11-pbonz...@redhat.com/T/#u).



Ignorant question: how could it be called outside coroutine context?


You're right, only blkdebug_debug_event() can be called outside 
coroutine context.  I confused process_rule() (called by 
blkdebug_debug_event(), both inside and outside coroutine context) with 
rule_check() (called in coroutine context).



Also, code smell: reporting an error without taking an error path.  But
let's worry about that only after I understand the problem you're trying
to fix.


Unfortunately there's no way to know in advance if an event will be 
called inside vs. outside a coroutine.  I can keep the abort() if you 
think it's preferrable, so what you get is still a crash but with a 
nicer error message.  Since this is debugging code either solution has 
pros and cons.


Paolo

Re: [PATCH] blkdebug: ignore invalid rules in non-coroutine context

2022-10-13 Thread Markus Armbruster 
Paolo Bonzini  writes:

> blkdebug events can be called from either non-coroutine or coroutine
> contexts.  However, some actions (specifically suspend actions and
> errors reported with immediately=off) only make sense from within
> a coroutine.
>
> Currently, using those action would lead to an abort() in
> qemu_coroutine_yield() ("Co-routine is yielding to no one").
> Catch them and print an error instead.
>
> Signed-off-by: Paolo Bonzini 
> ---
>  block/blkdebug.c | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index bbf2948703..bf0aedb17d 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -31,6 +31,7 @@
>  #include "block/qdict.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
> +#include "qemu/error-report.h"
>  #include "qapi/qapi-visit-block-core.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qlist.h"
> @@ -623,8 +624,13 @@ static int rule_check(BlockDriverState *bs, uint64_t 
> offset, uint64_t bytes,
>  
>  qemu_mutex_unlock(>lock);
>  if (!immediately) {
> -aio_co_schedule(qemu_get_current_aio_context(), 
> qemu_coroutine_self());
> -qemu_coroutine_yield();
> +if (qemu_in_coroutine()) {
> +aio_co_schedule(qemu_get_current_aio_context(), 
> qemu_coroutine_self());
> +qemu_coroutine_yield();
> +} else {
> +error_report("Non-coroutine event %s needs immediately = off\n",
> + BlkdebugEvent_lookup.array[rule->event]);

rule_check() is called from blkdebug_co_preadv(), blkdebug_co_pwritev(),
blkdebug_co_pwrite_zeroes(), blkdebug_co_pdiscard(),
blkdebug_co_block_status() (all marked coroutine_fn), and
blkdebug_co_flush() (which looks like it should be marked coroutine_fn).

Ignorant question: how could it be called outside coroutine context?

Also, code smell: reporting an error without taking an error path.  But
let's worry about that only after I understand the problem you're trying
to fix.

> +}
>  }
>  
>  return -error;
> @@ -858,7 +864,12 @@ static void blkdebug_debug_event(BlockDriverState *bs, 
> BlkdebugEvent event)
>  }
>  
>  while (actions_count[ACTION_SUSPEND] > 0) {
> -qemu_coroutine_yield();
> +if (qemu_in_coroutine()) {
> +qemu_coroutine_yield();
> +} else {
> +error_report("Non-coroutine event %s cannot suspend\n",
> + BlkdebugEvent_lookup.array[event]);
> +}
>  actions_count[ACTION_SUSPEND]--;
>  }
>  }

[PATCH] blkdebug: ignore invalid rules in non-coroutine context

2022-10-13 Thread Paolo Bonzini 
blkdebug events can be called from either non-coroutine or coroutine
contexts.  However, some actions (specifically suspend actions and
errors reported with immediately=off) only make sense from within
a coroutine.

Currently, using those action would lead to an abort() in
qemu_coroutine_yield() ("Co-routine is yielding to no one").
Catch them and print an error instead.

Signed-off-by: Paolo Bonzini 
---
 block/blkdebug.c | 17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index bbf2948703..bf0aedb17d 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -31,6 +31,7 @@
 #include "block/qdict.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/error-report.h"
 #include "qapi/qapi-visit-block-core.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
@@ -623,8 +624,13 @@ static int rule_check(BlockDriverState *bs, uint64_t 
offset, uint64_t bytes,
 
 qemu_mutex_unlock(>lock);
 if (!immediately) {
-aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
-qemu_coroutine_yield();
+if (qemu_in_coroutine()) {
+aio_co_schedule(qemu_get_current_aio_context(), 
qemu_coroutine_self());
+qemu_coroutine_yield();
+} else {
+error_report("Non-coroutine event %s needs immediately = off\n",
+ BlkdebugEvent_lookup.array[rule->event]);
+}
 }
 
 return -error;
@@ -858,7 +864,12 @@ static void blkdebug_debug_event(BlockDriverState *bs, 
BlkdebugEvent event)
 }
 
 while (actions_count[ACTION_SUSPEND] > 0) {
-qemu_coroutine_yield();
+if (qemu_in_coroutine()) {
+qemu_coroutine_yield();
+} else {
+error_report("Non-coroutine event %s cannot suspend\n",
+ BlkdebugEvent_lookup.array[event]);
+}
 actions_count[ACTION_SUSPEND]--;
 }
 }
-- 
2.37.3