Re: [PATCH v2] virtio-blk: Respect discard granularity

[Stefano Garzarella]

On Tue, Feb 23, 2021 at 02:36:16PM +0900, Akihiko Odaki wrote:
> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
> ---
> hw/block/virtio-blk.c          | 8 +++++++-
> hw/core/machine.c              | 9 ++++++++-
> include/hw/virtio/virtio-blk.h | 1 +
> 3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index bac2d6fa2b2..f4378e61182 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -962,10 +962,14 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
> uint8_t *config)
>     blkcfg.wce = blk_enable_write_cache(s->blk);
>     virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues);
>     if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_DISCARD)) {
> +        uint32_t discard_granularity = conf->discard_granularity;
> +        if (discard_granularity == -1 || !s->conf.report_discard_granularity) {
> +            discard_granularity = blk_size;
> +        }
>         virtio_stl_p(vdev, &blkcfg.max_discard_sectors,
>                      s->conf.max_discard_sectors);
>         virtio_stl_p(vdev, &blkcfg.discard_sector_alignment,
> -                     blk_size >> BDRV_SECTOR_BITS);
> +                     discard_granularity >> BDRV_SECTOR_BITS);
>         /*
>          * We support only one segment per request since multiple segments
>          * are not widely used and there are no userspace APIs that allow
> @@ -1299,6 +1303,8 @@ static Property virtio_blk_properties[] = {
>                      IOThread *),
>     DEFINE_PROP_BIT64("discard", VirtIOBlock, host_features,
>                       VIRTIO_BLK_F_DISCARD, true),
> +    DEFINE_PROP_BOOL("report-discard-granularity", VirtIOBlock,
> +                     conf.report_discard_granularity, true),
>     DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock, host_features,
>                       VIRTIO_BLK_F_WRITE_ZEROES, true),
>     DEFINE_PROP_UINT32("max-discard-sectors", VirtIOBlock,
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index de3b8f1b318..3ba976e5bbc 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -33,7 +33,9 @@
> #include "migration/global_state.h"
> #include "migration/vmstate.h"
>
> -GlobalProperty hw_compat_5_2[] = {};
> +GlobalProperty hw_compat_5_2[] = {
> +    { "virtio-blk-device", "report-discard-granularity", "off" },

IIUC older machines inherit the properties set for newer machines, so I 
think only this one is enough.

Thanks,
Stefano

> +};
> const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
>
> GlobalProperty hw_compat_5_1[] = {
> @@ -41,6 +43,7 @@ GlobalProperty hw_compat_5_1[] = {
>     { "vhost-user-blk", "num-queues", "1"},
>     { "vhost-user-scsi", "num_queues", "1"},
>     { "virtio-blk-device", "num-queues", "1"},
> +    { "virtio-blk-device", "report-discard-granularity", "off" },
>     { "virtio-scsi-device", "num_queues", "1"},
>     { "nvme", "use-intel-id", "on"},
>     { "pvpanic", "events", "1"}, /* PVPANIC_PANICKED */
> @@ -50,6 +53,7 @@ const size_t hw_compat_5_1_len = G_N_ELEMENTS(hw_compat_5_1);
> GlobalProperty hw_compat_5_0[] = {
>     { "pci-host-bridge", "x-config-reg-migration-enabled", "off" },
>     { "virtio-balloon-device", "page-poison", "false" },
> +    { "virtio-blk-device", "report-discard-granularity", "off" },
>     { "vmport", "x-read-set-eax", "off" },
>     { "vmport", "x-signal-unsupported-cmd", "off" },
>     { "vmport", "x-report-vmx-type", "off" },
> @@ -59,6 +63,7 @@ GlobalProperty hw_compat_5_0[] = {
> const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
>
> GlobalProperty hw_compat_4_2[] = {
> +    { "virtio-blk-device", "report-discard-granularity", "off" },
>     { "virtio-blk-device", "queue-size", "128"},
>     { "virtio-scsi-device", "virtqueue_size", "128"},
>     { "virtio-blk-device", "x-enable-wce-if-config-wce", "off" },
> @@ -74,6 +79,7 @@ GlobalProperty hw_compat_4_2[] = {
> const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);
>
> GlobalProperty hw_compat_4_1[] = {
> +    { "virtio-blk-device", "report-discard-granularity", "off" },
>     { "virtio-pci", "x-pcie-flr-init", "off" },
>     { "virtio-device", "use-disabled-flag", "false" },
> };
> @@ -83,6 +89,7 @@ GlobalProperty hw_compat_4_0[] = {
>     { "VGA",            "edid", "false" },
>     { "secondary-vga",  "edid", "false" },
>     { "bochs-display",  "edid", "false" },
> +    { "virtio-blk-device", "report-discard-granularity", "off" },
>     { "virtio-vga",     "edid", "false" },
>     { "virtio-gpu-device", "edid", "false" },
>     { "virtio-device", "use-started", "false" },
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 214ab748229..29655a406dd 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -41,6 +41,7 @@ struct VirtIOBlkConf
>     uint16_t num_queues;
>     uint16_t queue_size;
>     bool seg_max_adjust;
> +    bool report_discard_granularity;
>     uint32_t max_discard_sectors;
>     uint32_t max_write_zeroes_sectors;
>     bool x_enable_wce_if_config_wce;
> --
> 2.24.3 (Apple Git-128)