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

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

On 27/01/2022 12:01, Kevin Wolf wrote:
>> +/* Common functions that are neither I/O nor Global State */
>> +
>> +int bdrv_parse_aio(const char *mode, int *flags);
> Makes sense to me to have this here, it is just a helper function that
> parses stuff and doesn't touch any state. However, what is different
> about the following?
> 
> int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
> int bdrv_parse_discard_flags(const char *mode, int *flags);
> 
> Despite being simple helpers, you kept them as global state.
> 

I guess those two are only called by GS functions, while tihs one is
not. Anyways, you are right, I will move them to block-common.h

[...]

>>> +/* disk I/O throttling */
>> Let's remove this comment (maybe in a separate preparational patch), it
>> doesn't make any sense at all any more. It was added in commit
>> 0563e191516 to describe bdrv_io_limits_enable()/disable(), which were
>> removed in commit 97148076, but apparently I forgot to remove the
>> comment.
>> 

Done, sent as a separate patch "block.h: remove outdated comment"

[...]

>>> +void bdrv_init_with_whitelist(void);
>>> +bool bdrv_uses_whitelist(void);
>>> +int bdrv_is_whitelisted(BlockDriver *drv, bool read_only);
>> The whitelist is static and doesn't touch runtime state.

Ok I am moving all the three above in block-common.h

[...]

>>> +/* async block I/O */
>>> +void bdrv_aio_cancel(BlockAIOCB *acb);
>>> +void bdrv_invalidate_cache_all(Error **errp);
>>> +
>>> +int bdrv_inactivate_all(void);
>> The grouping is odd here. The comment refers only to bdrv_aio_cancel(),
>> while bdrv_invalidate_cache_all()/bdrv_inactivate_all() are a logically
>> related function pair.
>> 
>> Also odd: Why keep bdrv_aio_cancel() as global state, but make
>> bdrv_aio_cancel_async() an I/O function? (The latter is required, of
>> course, because virtio-scsi can call it in I/O threads.)
>> 
>> bdrv_aio_cancel() calls aio_poll(). I think this means that it actually
>> must run in the AioContext that is polled, which may or may not be the
>> main thread. Before this series, we assert running in the main thread
>> only in one if branch.

ok moving the comment + bdrv_aio_cancel in block-io.h

[...]

>>> +    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.

[...]

>>> +
>>> +/**
>>> + * 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".


Rest of comments that I did not answer: I agree.

Emanuele