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