Re: [PATCH v6 02/33] include/block/block: split header into I/O and global state API

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

On 31/01/2022 16:58, Paolo Bonzini wrote:
> On 1/31/22 15:54, Kevin Wolf wrote:
>> So I guess the decision depends on what you're going to use the
>> categories in the future. I get the feeling that we have one more
>> category than this series introduces:
>>
>> * Global State (must run from the main thread/hold the BQL)
>> * I/O (can be called in any thread under the AioContext lock, doesn't
>>    modify global state, drain waits for it to complete)
>> * Common (doesn't even touch global state)
>> * iothread dependent (can run without the BQL, but only in one specific
>>    iothread while holding its AioContext lock; this would cover at least
>>    AIO_WAIT_WHILE() and all of its indirect callers)
> 
> Yes, I agree.
> 
> bdrv_drained_begin and friends are somewhat like a coroutine-level
> lock/unlock primitive, so they need to be available in both the main
> thread and the iothread.  They could be called iothread dependent,
> AioContext dependent, or perhaps "global or I/O".
> 
> That said, even if they are a different category, I think it makes sense
> to leave them in the same header file as I/O functions, because I/O
> functions are locked out between drained_begin and drained_end.
> 
> Paolo
> 

Proposed category description:
/*
 * "Global OR I/O" API functions. These functions can run without
 * the BQL, but only in one specific iothread/main loop.
 *
 * More specifically, these functions use BDRV_POLL_WHILE(bs), which
 * requires the caller to be either in the main thread and hold
 * the BlockdriverState (bs) AioContext lock, or directly in the
 * home thread that runs the bs AioContext. Calling them from
 * another thread in another AioContext would cause deadlocks.
 *
 * Therefore, these functions are not proper I/O, because they
 * can't run in *any* iothreads, but only in a specific one.
 */

Functions that will surely go under this category:

BDRV_POLL_WHILE
bdrv_parent_drained_begin_single
bdrv_parent_drained_end_single
bdrv_drain_poll
bdrv_drained_begin
bdrv_do_drained_begin_quiesce
bdrv_subtree_drained_begin
bdrv_drained_end
bdrv_drained_end_no_poll
bdrv_subtree_drained_end

(all generated_co_wrapper)
bdrv_truncate
bdrv_check
bdrv_invalidate_cache
bdrv_flush
bdrv_pdiscard
bdrv_readv_vmstate
bdrv_writev_vmstate


What I am not sure:

* bdrv_drain_all_begin - bdrv_drain_all_end - bdrv_drain_all: these were
classified as GS, because thay are always called from the main loop.
Should they go in this new category?

* how should I interpret "all the callers of BDRV_POLL_WHILE"?
Meaning, if I consider also the callers of the callers, we end up
covering much much more functions. Should I only consider the direct
callers (ie the above)?

Emanuele