Re: [PATCH v5 04/31] include/sysemu/block-backend: split header into I/O and global state (GS) API

[Hanna Reitz]

On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote:
> Similarly to the previous patches, split block-backend.h
> in block-backend-io.h and block-backend-global-state.h
>
> In addition, remove "block/block.h" include as it seems
> it is not necessary anymore, together with "qemu/iov.h"
>
> block-backend-common.h contains the structures shared between
> the two headers, and the functions that can't be categorized as
> I/O or global state.
>
> Assertions are added in the next patch.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   include/sysemu/block-backend-common.h       |  84 ++++++
>   include/sysemu/block-backend-global-state.h | 121 +++++++++
>   include/sysemu/block-backend-io.h           | 139 ++++++++++
>   include/sysemu/block-backend.h              | 269 +-------------------
>   block/block-backend.c                       |   9 +-
>   5 files changed, 353 insertions(+), 269 deletions(-)
>   create mode 100644 include/sysemu/block-backend-common.h
>   create mode 100644 include/sysemu/block-backend-global-state.h
>   create mode 100644 include/sysemu/block-backend-io.h

[...]

> diff --git a/include/sysemu/block-backend-global-state.h 
> b/include/sysemu/block-backend-global-state.h
> new file mode 100644
> index 0000000000..77009bf7a2
> --- /dev/null
> +++ b/include/sysemu/block-backend-global-state.h

[...]

> +int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf);

I don’t quite follow what makes blk_ioctl() a GS function.  (It’s 
possible that I forgot something about this from v4, though, and can’t 
find it anymore...)

I suppose it can be said that in the context of the block layer, ioctl 
functions are generally used outside of I/O threads as something that 
kind of affects the global data state, but... bdrv_co_ioctl() is in 
block-io.h, and internally we definitely do use ioctl in I/O threads 
(various uses in file-posix.c, e.g. to zero out areas).  I also don’t 
understand why blk_ioctl() is GS, and blk_aio_ioctl() is I/O.

And if you have a virtio-scsi device in an iothread, and then create a 
scsi-generic device on it, will bdrv_co_ioctl() not run in the I/O 
thread still...?  Hm, testing (on patch 6), it appears that not – I 
think that’s because it mostly uses blk_aio_ioctl(), apart from 
*_realize() and scsi_SG_IO_FROM_DEV() (whatever that is; and this makes 
me a bit cautious, too).  Still, I’m not quite sure how it’s more global 
state than e.g. writing zeroes or, well, blk_aio_ioctl().

However, testing brought some other problems to light:

$ ls /dev/sg?
/dev/sg0

$ sudo modprobe scsi_debug max_luns=1 num_tgts=1 add_host=1

$ ls /dev/sg?
/dev/sg0 /dev/sg1

$ ./qemu-system-x86_64 \
    -object iothread,id=foo \
    -device virtio-scsi,iothread=foo \
    -blockdev host_device,filename=/dev/sg1,node-name=bar \
    -device scsi-generic,drive=bar
qemu-system-x86_64: ../block/block-backend.c:2038: 
blk_set_guest_block_size: Assertion `qemu_in_main_thread()' failed.
[1]    121353 IOT instruction (core dumped)  ./qemu-system-x86_64 
-object iothread,id=foo -device virtio-scsi,iothread=foo


And:

$ ./qemu-system-x86_64 \
    -object iothread,id=foo \
    -device virtio-scsi,iothread=foo \
    -blockdev null-co,read-zeroes=true,node-name=bar \
    -device scsi-hd,drive=bar \
    -cdrom ~/tmp/arch.iso \
    -enable-kvm \
    -m 1g
qemu-system-x86_64: ../block/block-backend.c:1918: 
blk_enable_write_cache: Assertion `qemu_in_main_thread()' failed.
[1]    127203 IOT instruction (core dumped)  ./qemu-system-x86_64 
-object iothread,id=foo -device virtio-scsi,iothread=foo

If I boot from a CD under virtio-scsi (in an iothread), I also get an 
error for blk_lock_medium(), and if I eject such a CD, for blk_eject() 
and blk_get_attached_dev_id() (called from blk_eject()).

It appears like scsi_disk_emulate_command() is always run in the BB’s 
AioContext, and so everything it (indirectly) calls cannot assume to be 
in the main thread.  As for blk_set_guest_block_size(), that one’s 
called from scsi-generic’s scsi_read_complete().

Hanna