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

[Emanuele Giuseppe Esposito]

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