Re: [PATCH v7 00/31] block layer: split block APIs in global state and I/O

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

Thank you for the script!

> 
> Error: bdrv_get_full_backing_filename() is IO_CODE(), but calls GLOBAL_STATE_CODE() code
>        bdrv_get_full_backing_filename() -> bdrv_make_absolute_filename() -> bdrv_dirname() -> GLOBAL_STATE_CODE()

So this was something you caught in the first pass of reviews, and you said
"Why isn't this grouped as global state? (Or possibly I/O since it only
reads bs.)"

I will change bdrv_get_full_backing_filename to GS.

> 
> Error: blk_error_action() is IO_CODE(), but calls GLOBAL_STATE_CODE() code
>        blk_error_action() -> send_qmp_error_event() -> blk_iostatus_is_enabled() -> GLOBAL_STATE_CODE()

I see that blk_error_action is IO:
blk_aio_ioctl(s->qdev.conf.blk, SG_IO, io_header, scsi_block_sgio_complete, req); -> csi_block_sgio_complete -> scsi_handle_rw_error ->  blk_error_action
so let's move blk_iostatus_is_enabled to IO_CODE too, together with blk_iostatus_set_err
 
> 
> Error: bdrv_drained_end_no_poll() is IO_CODE(), but calls GLOBAL_STATE_CODE() code
>        bdrv_drained_end_no_poll() -> bdrv_do_drained_end() -> bdrv_do_drained_end() -> bdrv_co_yield_to_drain() -> bdrv_co_drain_bh_cb() -> bdrv_drain_all_begin() -> GLOBAL_STATE_CODE()
> 

False positive, as it will be called in a bh in the main loop.


> Error: nbd_co_do_establish_connection() is IO_CODE(), but calls GLOBAL_STATE_CODE() code
>        nbd_co_do_establish_connection() -> nbd_handle_updated_info() -> bdrv_apply_auto_read_only() -> GLOBAL_STATE_CODE()

bdrv_apply_auto_read_only is used in many .bdrv_open callbacks, while nbd_co_do_establish_connection is IO_CODE because it is used by .bdrv_co_block_status and similar callbacks that are classified as I/O. 
Still, we can change bdrv_apply_auto_read_only in IO_CODE, and let it still be used from GLOBAL_STATE_CODE.
This implies changing also bdrv_can_set_read_only (callee in bdrv_apply_auto_read_only) in IO_CODE

> 
> Error: bdrv_drained_end_no_poll() is IO_CODE(), but calls IO_OR_GS_CODE() code
>        bdrv_drained_end_no_poll() -> bdrv_do_drained_end() -> bdrv_do_drained_end() -> bdrv_co_yield_to_drain() -> bdrv_co_drain_bh_cb() -> bdrv_drain_all_begin() -> bdrv_do_drained_begin() -> bdrv_do_drained_begin() -> bdrv_do_drained_begin_quiesce() -> IO_OR_GS_CODE()


False positive

> 
> Error: blk_drain() is IO_OR_GS_CODE(), but calls GLOBAL_STATE_CODE() code
>        blk_drain() -> bdrv_ref() -> GLOBAL_STATE_CODE()

Here setting bdrv_ref as IO_OR_GS_CODE would work, as GLOBAL_STATE is still allowed call it.
Same applies for bdrv_unref, but we need to change in IO_OR_GS_CODE the following GLOBAL_STATE:
bdrv_close
bdrv_delete
bdrv_op_blocker_is_empty
bdrv_drain_all_end_quiesce

> 
> Error: bdrv_drained_begin() is IO_OR_GS_CODE(), but calls GLOBAL_STATE_CODE() code
>        bdrv_drained_begin() -> bdrv_do_drained_begin() -> bdrv_do_drained_begin() -> bdrv_co_yield_to_drain() -> bdrv_co_drain_bh_cb() -> bdrv_drain_all_begin() -> GLOBAL_STATE_CODE()

False positive
> 
> Error: bdrv_subtree_drained_begin() is IO_OR_GS_CODE(), but calls GLOBAL_STATE_CODE() code
>        bdrv_subtree_drained_begin() -> bdrv_do_drained_begin() -> bdrv_do_drained_begin() -> bdrv_co_yield_to_drain() -> bdrv_co_drain_bh_cb() -> bdrv_drain_all_begin() -> GLOBAL_STATE_CODE()

False positive
> 
> Error: bdrv_drained_end() is IO_OR_GS_CODE(), but calls GLOBAL_STATE_CODE() code
>        bdrv_drained_end() -> bdrv_do_drained_end() -> bdrv_do_drained_end() -> bdrv_co_yield_to_drain() -> bdrv_co_drain_bh_cb() -> bdrv_drain_all_begin() -> GLOBAL_STATE_CODE()
> 

False positive
> Error: bdrv_subtree_drained_end() is IO_OR_GS_CODE(), but calls GLOBAL_STATE_CODE() code
>        bdrv_subtree_drained_end() -> bdrv_do_drained_end() -> bdrv_do_drained_end() -> bdrv_co_yield_to_drain() -> bdrv_co_drain_bh_cb() -> bdrv_drain_all_begin() -> GLOBAL_STATE_CODE()
> 

False positive
> Error: bdrv_apply_subtree_drain() is IO_OR_GS_CODE(), but calls GLOBAL_STATE_CODE() code
>        bdrv_apply_subtree_drain() -> bdrv_do_drained_begin() -> bdrv_do_drained_begin() -> bdrv_co_yield_to_drain() -> bdrv_co_drain_bh_cb() -> bdrv_drain_all_begin() -> GLOBAL_STATE_CODE()
> 

False positive

> Error: bdrv_unapply_subtree_drain() is IO_OR_GS_CODE(), but calls GLOBAL_STATE_CODE() code
>        bdrv_unapply_subtree_drain() -> bdrv_do_drained_end() -> bdrv_do_drained_end() -> bdrv_co_yield_to_drain() -> bdrv_co_drain_bh_cb() -> bdrv_drain_all_begin() -> GLOBAL_STATE_CODE()
> 

False positive

> Error: bdrv_co_drain() is IO_OR_GS_CODE(), but calls GLOBAL_STATE_CODE() code
>        bdrv_co_drain() -> bdrv_drained_begin() -> bdrv_do_drained_begin() -> bdrv_do_drained_begin() -> bdrv_co_yield_to_drain() -> bdrv_co_drain_bh_cb() -> bdrv_drain_all_begin() -> GLOBAL_STATE_CODE()
> 

False positive

> Error: bdrv_drain() is IO_OR_GS_CODE(), but calls GLOBAL_STATE_CODE() code
>        bdrv_drain() -> bdrv_drained_begin() -> bdrv_do_drained_begin() -> bdrv_do_drained_begin() -> bdrv_co_yield_to_drain() -> bdrv_co_drain_bh_cb() -> bdrv_drain_all_begin() -> GLOBAL_STATE_CODE()
>

False positive

 
> Do you want to address some of these cases before we merge the series? I
> expect that usually we just miscategorised the caller, so the assertion
> wouldn't actually fail at runtime, but I haven't checked in detail yet.

I re-run the script and there are no more errors (except false positives).
Will send v8.

Emanuele