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

[Markus Armbruster]

Sam Li <faithilikerun@gmail.com> writes:

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

Makes sense.

> .bdrv_co_ioctl is set actually.

You're right; I diffed the two and misread the result.

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

Why would I ever want to use host_device instead of zoned_host_device?

To answer this question, we need to understand how their behavior
differs.

We can ignore the legacy protocol prefix / string filename part.

All that's left seems to be "if the host device is zoned, then using the
zoned_host_device driver gets you the zoned features, whereas using the
host_device driver doesn't".  What am I missing?

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

I'm not sure I understand all of this.  I'm also not sure I have to :)

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

I'm not opposed to making this a separate driver.  But the case for it
should be made in the commit message.  Discussing it in review is a fine
way to get to a better commit message, of course.

> Maybe Damien and Stefan can talk more about this?
>
>>
>> Forgive me if these are ignorant questions, or have been discussed
>> before.
>
> Always a pleasure.

Thanks!

[...]