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

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

On 07/02/2022 17:53, Kevin Wolf wrote:
> Am 01.02.2022 um 11:30 hat Paolo Bonzini geschrieben:
>> On 2/1/22 10:45, Emanuele Giuseppe Esposito wrote:
>>>> 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.
>>>
>>> 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?
>>
>> 1) They look at the list of BDS's, and 2) you can't in general be sure that
>> all BDS's are in *your* AioContext if you call them from a specific
>> AioContext.
>>
>> So they should be GS.
> 
> I agree, calling drain_all functions can only work from the main thread,
> so they are GS.
> 
>>> * 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)?
>>
>> In general it is safe to make a function GS even if it is potentially "GS or
>> I/O", because that _reduces_ the number of places you can call it from.
>> It's likewise safe to make it I/O-only, but probably it makes less sense.
> 
> Basically, we have a hierarchy of categories where you can always call
> functions in other categories with less restrictions, but never the
> opposite direction.

Added in the respective category documentation:
> 1. Common functions
 * These functions must never call any function from other categories
 * (I/O, "I/O or GS", Global State) except this one, but can be invoked by
 * all of them.

> 2. I/O functions
 * These functions can only call functions from I/O and common categories,
 * but can be invoked by GS, "I/O or GS" and I/O APIs.

> 3. I/O or GS functions
 * These functions can call any function from I/O, common and this
 * categories, but must be invoked only by other "I/O or GS" and GS APIs.

> 4. GS functions
 * These functions can call any function from this and other categories
 * (I/O, "I/O or GS", common), but must be invoked only by other GS APIs.

Emanuele
> 
> So common functions must never call any of the other categories. Global
> state functions can call functions in any category. And "I/O or GS"
> functions like BDRV_POLL_WHILE() can be called by other "I/O or GS" or
> just GS functions, but if it's ever (directly or indirectly) called by
> an I/O or common function, that would be a bug.
> 
> Kevin
>