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

[Kevin Wolf]

Am 31.01.2022 um 14:40 hat Emanuele Giuseppe Esposito geschrieben:
> On 27/01/2022 12:01, Kevin Wolf wrote:
> >>> +    BlockDriverState *bs_ = (bs);                          \
> >>> +    AIO_WAIT_WHILE(bdrv_get_aio_context(bs_),              \
> >>> +                   cond); })
> >> Hmm... AIO_WAIT_WHILE() is explicitly documented to be called from I/O
> >> threads. But it has to be a specific I/O thread, the home thread of the
> >> AioContext of bs.
> >> 
> >> So it really fits neither the description for global state (it works
> >> outside the main thread) nor for I/O functions (it doesn't work in any
> >> arbitrary thread).
> 
> So I would say BDRV_POLL_WHILE can go in block-common? what you wrote
> above is the exact definition of common functions.

Maybe, but either way I feel it should be categorised the same as its
users, like bdrv_drained_begin() below.

> >>> +
> >>> +/**
> >>> + * bdrv_drained_begin:
> >>> + *
> >>> + * Begin a quiesced section for exclusive access to the BDS, by disabling
> >>> + * external request sources including NBD server, block jobs, and device 
> >>> model.
> >>> + *
> >>> + * This function can be recursive.
> >>> + */
> >>> +void bdrv_drained_begin(BlockDriverState *bs);
> >> I don't see how the whole family of drain functions can work in any
> >> arbitrary thread. They call BDRV_POLL_WHILE(), the rules for which are
> >> that it has to be either in the main thread or in the AioContext of bs
> >> (which currently ensures that all block nodes and their users are in the
> >> same AioContext).
> >> 
> >> Cross-AioContext BDRV_POLL_WHILE() can cause deadlocks, as documented in
> >> AIO_WAIT_WHILE().
> >> 
> >> Actually, the same is true for all other BDRV_POLL_WHILE() callers, too.
> >> For example, every function generated by block-coroutine-wrapper.py can
> >> only be thread-safe when called in coroutine context, otherwise it has
> >> to be called from the node's AioContext.
> >> 
> 
> Maybe not in any arbitrary thread, but an iothread can call these
> function in any case. Therefore it is not a GS function for sure, so I
> would leave it as I/O.
> 
> I guess you can also see I/O more as "all the ones that are not GS".

Your current definition is:

/*
 * I/O API functions. These functions are thread-safe, and therefore
 * can run in any thread as long as the thread has called
 * aio_context_acquire/release().
 */

If you want it to mean "everything that isn't GS", then you need to
change this definition. (This in turn would mean that nothing would fit
the definiton for common functions any more, which is "neither GS nor
I/O".)

I thought that the definition that you currently have is actually
meaningful in the context of multiqueue, which I think is ultimately the
goal of your work. If at some point you're going to argue "this uses
only I/O functions, so it's threadsafe", the definition of I/O functions
must actually guarantee this.

If you categorise bdrv_drained_begin() as an I/O function, then this
property is violated because you have to call it from a specific thread.

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)

Kevin