1. Patchew
  2. QEMU
  3. View series

[PATCH v3] virtio-blk: Respect discard granularity

Akihiko Odaki posted 1 patch 4 years, 1 month ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210223120940.89227-1-akihiko.odaki@gmail.com
Maintainers: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>
There is a newer version of this series
hw/block/virtio-blk.c          | 8 +++++++-
hw/core/machine.c              | 4 +++-
include/hw/virtio/virtio-blk.h | 1 +
3 files changed, 11 insertions(+), 2 deletions(-)
[PATCH v3] virtio-blk: Respect discard granularity
Posted by Akihiko Odaki 4 years, 1 month ago 
Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 hw/block/virtio-blk.c          | 8 +++++++-
 hw/core/machine.c              | 4 +++-
 include/hw/virtio/virtio-blk.h | 1 +
 3 files changed, 11 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..e4df5797e72 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" },
+};
 const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
 
 GlobalProperty hw_compat_5_1[] = {
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)
Re: [PATCH v3] virtio-blk: Respect discard granularity
Posted by Michael S. Tsirkin 4 years, 1 month ago 
On Tue, Feb 23, 2021 at 09:09:40PM +0900, Akihiko Odaki wrote:
> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>


Acked-by: Michael S. Tsirkin <mst@redhat.com>


whoever knows more about the detail here, feel free to merge.

> ---
>  hw/block/virtio-blk.c          | 8 +++++++-
>  hw/core/machine.c              | 4 +++-
>  include/hw/virtio/virtio-blk.h | 1 +
>  3 files changed, 11 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..e4df5797e72 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" },
> +};
>  const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
>  
>  GlobalProperty hw_compat_5_1[] = {
> 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)
Re: [PATCH v3] virtio-blk: Respect discard granularity
Posted by Stefano Garzarella 4 years, 1 month ago 
+Cc stefanha@redhat.com

Please explain a bit the changes in the commit message, for example that 
you added 'report-discard-granularity', disabled it for older machines, 
that we use blk_size as default granularity, etc.

Something like this:

     Report the configured granularity for discard operation to the 
     guest. If this is not set use the block size.

     Since until now we have ignored the configured discard granularity 
     and always reported the block size, let's add 
     'report-discard-granularity' property and disable it for older 
     machine types to avoid migration issues.

And use ./scripts/get_maintainer.pl to CC all the maintainers (e.g.  
Stefan Hajnoczi was missing)

Other than that, the patch LGTM.

Thanks,
Stefano

On Tue, Feb 23, 2021 at 09:09:40PM +0900, Akihiko Odaki wrote:
>Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
>---
> hw/block/virtio-blk.c          | 8 +++++++-
> hw/core/machine.c              | 4 +++-
> include/hw/virtio/virtio-blk.h | 1 +
> 3 files changed, 11 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..e4df5797e72 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" },
>+};
> const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
>
> GlobalProperty hw_compat_5_1[] = {
>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)
>
>
[PATCH v4] virtio-blk: Respect discard granularity
Posted by Akihiko Odaki 4 years, 1 month ago 
Report the configured granularity for discard operation to the
guest. If this is not set use the block size.

Since until now we have ignored the configured discard granularity
and always reported the block size, let's add
'report-discard-granularity' property and disable it for older
machine types to avoid migration issues.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 hw/block/virtio-blk.c          | 8 +++++++-
 hw/core/machine.c              | 4 +++-
 include/hw/virtio/virtio-blk.h | 1 +
 3 files changed, 11 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..e4df5797e72 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" },
+};
 const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
 
 GlobalProperty hw_compat_5_1[] = {
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)
Re: [PATCH v4] virtio-blk: Respect discard granularity
Posted by Stefano Garzarella 4 years, 1 month ago 
On Thu, Feb 25, 2021 at 09:12:39AM +0900, Akihiko Odaki wrote:
>Report the configured granularity for discard operation to the
>guest. If this is not set use the block size.
>
>Since until now we have ignored the configured discard granularity
>and always reported the block size, let's add
>'report-discard-granularity' property and disable it for older
>machine types to avoid migration issues.
>
>Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
>---
> hw/block/virtio-blk.c          | 8 +++++++-
> hw/core/machine.c              | 4 +++-
> include/hw/virtio/virtio-blk.h | 1 +
> 3 files changed, 11 insertions(+), 2 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Re: [PATCH v4] virtio-blk: Respect discard granularity
Posted by Stefan Hajnoczi 4 years ago 
On Thu, Feb 25, 2021 at 09:12:39AM +0900, Akihiko Odaki wrote:
> Report the configured granularity for discard operation to the
> guest. If this is not set use the block size.
> 
> Since until now we have ignored the configured discard granularity
> and always reported the block size, let's add
> 'report-discard-granularity' property and disable it for older
> machine types to avoid migration issues.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
> ---
>  hw/block/virtio-blk.c          | 8 +++++++-
>  hw/core/machine.c              | 4 +++-
>  include/hw/virtio/virtio-blk.h | 1 +
>  3 files changed, 11 insertions(+), 2 deletions(-)

Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block

Stefan

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.