Re: [PATCH v8 3/7] block: add block layer APIs resembling Linux ZonedBlockDevice ioctls

[Sam Li]

Markus Armbruster <armbru@redhat.com> 于2022年8月30日周二 19:57写道：
>
> Sam Li <faithilikerun@gmail.com> writes:
>
> > By adding zone management operations in BlockDriver, storage controller
> > emulation can use the new block layer APIs including Report Zone and
> > four zone management operations (open, close, finish, reset).
> >
> > Add zoned storage commands of the device: zone_report(zrp), zone_open(zo),
> > zone_close(zc), zone_reset(zrs), zone_finish(zf).
> >
> > For example, to test zone_report, use following command:
> > $ ./build/qemu-io --image-opts driver=zoned_host_device, 
> > filename=/dev/nullb0
> > -c "zrp offset nr_zones"
> >
> > Signed-off-by: Sam Li <faithilikerun@gmail.com>
> > Reviewed-by: Hannes Reinecke <hare@suse.de>
>
> [...]
>
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 0a8b4b426e..e3efba6db7 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
>
> [...]
>
> > @@ -3752,6 +4025,54 @@ static BlockDriver bdrv_host_device = {
> >  #endif
> >  };
> >
> > +#if defined(CONFIG_BLKZONED)
> > +static BlockDriver bdrv_zoned_host_device = {
> > +        .format_name = "zoned_host_device",
>
> Indentation should be 4, not 8.
>
> > +        .protocol_name = "zoned_host_device",
> > +        .instance_size = sizeof(BDRVRawState),
> > +        .bdrv_needs_filename = true,
> > +        .bdrv_probe_device  = hdev_probe_device,
> > +        .bdrv_file_open     = hdev_open,
> > +        .bdrv_close         = raw_close,
> > +        .bdrv_reopen_prepare = raw_reopen_prepare,
> > +        .bdrv_reopen_commit  = raw_reopen_commit,
> > +        .bdrv_reopen_abort   = raw_reopen_abort,
> > +        .bdrv_co_create_opts = bdrv_co_create_opts_simple,
> > +        .create_opts         = &bdrv_create_opts_simple,
> > +        .mutable_opts        = mutable_opts,
> > +        .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
> > +        .bdrv_co_pwrite_zeroes = hdev_co_pwrite_zeroes,
> > +
> > +        .bdrv_co_preadv         = raw_co_preadv,
> > +        .bdrv_co_pwritev        = raw_co_pwritev,
> > +        .bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
> > +        .bdrv_co_pdiscard       = hdev_co_pdiscard,
> > +        .bdrv_co_copy_range_from = raw_co_copy_range_from,
> > +        .bdrv_co_copy_range_to  = raw_co_copy_range_to,
> > +        .bdrv_refresh_limits = raw_refresh_limits,
> > +        .bdrv_io_plug = raw_aio_plug,
> > +        .bdrv_io_unplug = raw_aio_unplug,
> > +        .bdrv_attach_aio_context = raw_aio_attach_aio_context,
> > +
> > +        .bdrv_co_truncate       = raw_co_truncate,
> > +        .bdrv_getlength = raw_getlength,
> > +        .bdrv_get_info = raw_get_info,
> > +        .bdrv_get_allocated_file_size
> > +                            = raw_get_allocated_file_size,
> > +        .bdrv_get_specific_stats = hdev_get_specific_stats,
> > +        .bdrv_check_perm = raw_check_perm,
> > +        .bdrv_set_perm   = raw_set_perm,
> > +        .bdrv_abort_perm_update = raw_abort_perm_update,
> > +        .bdrv_probe_blocksizes = hdev_probe_blocksizes,
> > +        .bdrv_probe_geometry = hdev_probe_geometry,
> > +        .bdrv_co_ioctl = hdev_co_ioctl,
> > +
> > +        /* zone management operations */
> > +        .bdrv_co_zone_report = raw_co_zone_report,
> > +        .bdrv_co_zone_mgmt = raw_co_zone_mgmt,
> > +};
>
> Differences to bdrv_host_device:
>
> * .bdrv_parse_filename is not set
>
> * .bdrv_co_ioctl is not set
>
> * .bdrv_co_zone_report and .bdrv_co_zone_mgmt are set

As Stefan mentioned, zoned_host_device is a new driver that doesn't
work with string filenames. .bdrv_parse_filename() helps legacy
drivers strip the optional protocol prefix off the filename and no use
here. Therefore it can be dropped.

.bdrv_co_ioctl is set actually.

Zoned_host_device is basically host_device + zone operations. It
serves for a simple purpose: if the host device is zoned, register
zoned_host_device driver; else, register host_device.

>
> Notably common is .bdrv_file_open = hdev_open.  What happens when you
> try to create a zoned_host_device where the @filename argument is not in
> fact a zoned device?

If the device is a regular block device, QEMU will still open the
device. For instance, I use a loopback device to test zone_report in
qemu-io. It returns ENOTTY which indicates Inappropriate ioctl for the
device. Meanwhile, if using a regular block device when emulation a
zoned device on a guest os, the best case is that the guest can boot
but has no emulated block device. In some cases, QEMU just terminates
because the block device has not met the alignment requirements.

>
> Do we really need a separate, but almost identical BlockDriver?  Could
> the existing one provide zoned functionality exactly when the underlying
> host device does?

I did use the existing one host device to add zoned commands at first.
But then, we decided to change that and use a separate BlockDriver.
Though the existing one can provide zoned functionality, a new
BlockDriver makes it clear when mixing block drivers, adding more
configurations/constraints, etc. For example, zoned devices must
enforce direct I/O instead of using page cache to ensure the order of
writes. It would be good to print a message for users when using
zoned_host_device without setting direct I/O.

However, it's still a simple version I was thinking about and can be
improved/changed afterward. Using host_device only is possible I think
but needs more carefully thinking.

Maybe Damien and Stefan can talk more about this?

>
> Forgive me if these are ignorant questions, or have been discussed
> before.

Always a pleasure.

>
> > +#endif
> > +
> >  #if defined(__linux__) || defined(__FreeBSD__) || 
> > defined(__FreeBSD_kernel__)
> >  static void cdrom_parse_filename(const char *filename, QDict *options,
> >                                   Error **errp)
> > @@ -4012,6 +4333,9 @@ static void bdrv_file_init(void)
> >      bdrv_register(&bdrv_file);
> >  #if defined(HAVE_HOST_BLOCK_DEVICE)
> >      bdrv_register(&bdrv_host_device);
> > +#if defined(CONFIG_BLKZONED)
> > +    bdrv_register(&bdrv_zoned_host_device);
> > +#endif
> >  #ifdef __linux__
> >      bdrv_register(&bdrv_host_cdrom);
> >  #endif
>
> [...]
>
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 2173e7734a..c6bbb7a037 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -2942,6 +2942,7 @@
> >  # @compress: Since 5.0
> >  # @copy-before-write: Since 6.2
> >  # @snapshot-access: Since 7.0
> > +# @zoned_host_device: Since 7.2
> >  #
> >  # Since: 2.9
> >  ##
> > @@ -2955,7 +2956,8 @@
> >              'luks', 'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 
> > 'parallels',
> >              'preallocate', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'rbd',
> >              { 'name': 'replication', 'if': 'CONFIG_REPLICATION' },
> > -            'ssh', 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> > +            'ssh', 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat',
> > +            { 'name': 'zoned_host_device', 'if': 'CONFIG_BLKZONED' } ] }
>
> QAPI naming conventions ask for 'zoned-host-device'.  We may choose to
> ignore them to stay closer to existing 'host_device'.

I am not sure why should ignore zoned_host_device. Can you be more specific?

>
> >
> >  ##
> >  # @BlockdevOptionsFile:
> > @@ -4329,7 +4331,9 @@
> >        'vhdx':       'BlockdevOptionsGenericFormat',
> >        'vmdk':       'BlockdevOptionsGenericCOWFormat',
> >        'vpc':        'BlockdevOptionsGenericFormat',
> > -      'vvfat':      'BlockdevOptionsVVFAT'
> > +      'vvfat':      'BlockdevOptionsVVFAT',
> > +      'zoned_host_device': { 'type': 'BlockdevOptionsFile',
> > +                             'if': 'CONFIG_BLKZONED' }
> >    } }
> >
> >  ##
>
> [...]
>