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

[Kevin Wolf <kwolf@redhat.com>]

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.

1. Common functions
2. I/O functions
3. I/O or GS functions
4. GS functions

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