1. Patchew
  2. QEMU
  3. [PATCH v2 0/2] block: discard alignment fixes
  4. View patch

[PATCH v2 1/2] file-posix: probe discard alignment on Linux block devices

Stefan Hajnoczi posted 2 patches 7 months, 1 week ago
There is a newer version of this series
[PATCH v2 1/2] file-posix: probe discard alignment on Linux block devices
Posted by Stefan Hajnoczi 7 months, 1 week ago 
Populate the pdiscard_alignment block limit so the block layer is able
align discard requests correctly.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/file-posix.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 56d1972d15..2a1e1f48c0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1276,10 +1276,10 @@ static int get_sysfs_zoned_model(struct stat *st, BlockZoneModel *zoned)
 }
 #endif /* defined(CONFIG_BLKZONED) */
 
+#ifdef CONFIG_LINUX
 /*
  * Get a sysfs attribute value as a long integer.
  */
-#ifdef CONFIG_LINUX
 static long get_sysfs_long_val(struct stat *st, const char *attribute)
 {
     g_autofree char *str = NULL;
@@ -1299,6 +1299,30 @@ static long get_sysfs_long_val(struct stat *st, const char *attribute)
     }
     return ret;
 }
+
+/*
+ * Get a sysfs attribute value as a uint32_t.
+ */
+static int get_sysfs_u32_val(struct stat *st, const char *attribute,
+                             uint32_t *u32)
+{
+    g_autofree char *str = NULL;
+    const char *end;
+    unsigned int val;
+    int ret;
+
+    ret = get_sysfs_str_val(st, attribute, &str);
+    if (ret < 0) {
+        return ret;
+    }
+
+    /* The file is ended with '\n', pass 'end' to accept that. */
+    ret = qemu_strtoui(str, &end, 10, &val);
+    if (ret == 0 && end && *end == '\0') {
+        *u32 = val;
+    }
+    return ret;
+}
 #endif
 
 static int hdev_get_max_segments(int fd, struct stat *st)
@@ -1318,6 +1342,23 @@ static int hdev_get_max_segments(int fd, struct stat *st)
 #endif
 }
 
+/*
+ * Fills in *dalign with the discard alignment and returns 0 on success,
+ * -errno otherwise.
+ */
+static int hdev_get_pdiscard_alignment(struct stat *st, uint32_t *dalign)
+{
+#ifdef CONFIG_LINUX
+    /*
+     * Note that Linux "discard_granularity" is QEMU "discard_alignment". Linux
+     * "discard_alignment" is something else.
+     */
+    return get_sysfs_u32_val(st, "discard_granularity", dalign);
+#else
+    return -ENOTSUP;
+#endif
+}
+
 #if defined(CONFIG_BLKZONED)
 /*
  * If the reset_all flag is true, then the wps of zone whose state is
@@ -1527,6 +1568,19 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
         }
     }
 
+    if (S_ISBLK(st.st_mode)) {
+        uint32_t dalign = 0;
+        int ret;
+
+        ret = hdev_get_pdiscard_alignment(&st, &dalign);
+        if (ret == 0) {
+            /* Must be a multiple of request_alignment */
+            assert(dalign % bs->bl.request_alignment == 0);
+
+            bs->bl.pdiscard_alignment = dalign;
+        }
+    }
+
     raw_refresh_zoned_limits(bs, &st, errp);
 }
 
-- 
2.49.0
Re: [PATCH v2 1/2] file-posix: probe discard alignment on Linux block devices
Posted by Hanna Czenczek 7 months, 1 week ago 
On 10.04.25 20:41, Stefan Hajnoczi wrote:
> Populate the pdiscard_alignment block limit so the block layer is able
> align discard requests correctly.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/file-posix.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 55 insertions(+), 1 deletion(-)

Ah, I didn’t know sysfs is actually fair game.  Should we not also get 
the maximum discard length then, too?

> diff --git a/block/file-posix.c b/block/file-posix.c
> index 56d1972d15..2a1e1f48c0 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1276,10 +1276,10 @@ static int get_sysfs_zoned_model(struct stat *st, BlockZoneModel *zoned)
>   }
>   #endif /* defined(CONFIG_BLKZONED) */
>   
> +#ifdef CONFIG_LINUX
>   /*
>    * Get a sysfs attribute value as a long integer.
>    */
> -#ifdef CONFIG_LINUX
>   static long get_sysfs_long_val(struct stat *st, const char *attribute)
>   {
>       g_autofree char *str = NULL;
> @@ -1299,6 +1299,30 @@ static long get_sysfs_long_val(struct stat *st, const char *attribute)
>       }
>       return ret;
>   }
> +
> +/*
> + * Get a sysfs attribute value as a uint32_t.
> + */
> +static int get_sysfs_u32_val(struct stat *st, const char *attribute,
> +                             uint32_t *u32)
> +{
> +    g_autofree char *str = NULL;
> +    const char *end;
> +    unsigned int val;
> +    int ret;
> +
> +    ret = get_sysfs_str_val(st, attribute, &str);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    /* The file is ended with '\n', pass 'end' to accept that. */
> +    ret = qemu_strtoui(str, &end, 10, &val);
> +    if (ret == 0 && end && *end == '\0') {
> +        *u32 = val;
> +    }
> +    return ret;
> +}
>   #endif
>   
>   static int hdev_get_max_segments(int fd, struct stat *st)
> @@ -1318,6 +1342,23 @@ static int hdev_get_max_segments(int fd, struct stat *st)
>   #endif
>   }
>   
> +/*
> + * Fills in *dalign with the discard alignment and returns 0 on success,
> + * -errno otherwise.
> + */
> +static int hdev_get_pdiscard_alignment(struct stat *st, uint32_t *dalign)
> +{
> +#ifdef CONFIG_LINUX
> +    /*
> +     * Note that Linux "discard_granularity" is QEMU "discard_alignment". Linux
> +     * "discard_alignment" is something else.
> +     */
> +    return get_sysfs_u32_val(st, "discard_granularity", dalign);
> +#else
> +    return -ENOTSUP;
> +#endif
> +}
> +
>   #if defined(CONFIG_BLKZONED)
>   /*
>    * If the reset_all flag is true, then the wps of zone whose state is
> @@ -1527,6 +1568,19 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>           }
>       }
>   
> +    if (S_ISBLK(st.st_mode)) {
> +        uint32_t dalign = 0;
> +        int ret;
> +
> +        ret = hdev_get_pdiscard_alignment(&st, &dalign);
> +        if (ret == 0) {
> +            /* Must be a multiple of request_alignment */
> +            assert(dalign % bs->bl.request_alignment == 0);

Is it fair to crash qemu if the kernel reports a value that is not a 
multiple of request_alignment?  Wouldn’t it make more sense to take the 
maximum, and if that still isn’t a multiple, return an error here?

Hanna

> +
> +            bs->bl.pdiscard_alignment = dalign;
> +        }
> +    }
> +
>       raw_refresh_zoned_limits(bs, &st, errp);
>   }
>
Re: [PATCH v2 1/2] file-posix: probe discard alignment on Linux block devices
Posted by Stefan Hajnoczi 7 months ago 
On Fri, Apr 11, 2025 at 10:15:13AM +0200, Hanna Czenczek wrote:
> On 10.04.25 20:41, Stefan Hajnoczi wrote:
> > Populate the pdiscard_alignment block limit so the block layer is able
> > align discard requests correctly.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   block/file-posix.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 55 insertions(+), 1 deletion(-)
> 
> Ah, I didn’t know sysfs is actually fair game.  Should we not also get the
> maximum discard length then, too?

The maximum discard length behaves differently: the Linux block layer
splits requests according to the maximum discard length. If the guest
submits a discard request that is too large for the host, the host block
layer will split it and the request succeeds. That is why I didn't make
any changes to the maximum discard length in this series.

> 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 56d1972d15..2a1e1f48c0 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1276,10 +1276,10 @@ static int get_sysfs_zoned_model(struct stat *st, BlockZoneModel *zoned)
> >   }
> >   #endif /* defined(CONFIG_BLKZONED) */
> > +#ifdef CONFIG_LINUX
> >   /*
> >    * Get a sysfs attribute value as a long integer.
> >    */
> > -#ifdef CONFIG_LINUX
> >   static long get_sysfs_long_val(struct stat *st, const char *attribute)
> >   {
> >       g_autofree char *str = NULL;
> > @@ -1299,6 +1299,30 @@ static long get_sysfs_long_val(struct stat *st, const char *attribute)
> >       }
> >       return ret;
> >   }
> > +
> > +/*
> > + * Get a sysfs attribute value as a uint32_t.
> > + */
> > +static int get_sysfs_u32_val(struct stat *st, const char *attribute,
> > +                             uint32_t *u32)
> > +{
> > +    g_autofree char *str = NULL;
> > +    const char *end;
> > +    unsigned int val;
> > +    int ret;
> > +
> > +    ret = get_sysfs_str_val(st, attribute, &str);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +    /* The file is ended with '\n', pass 'end' to accept that. */
> > +    ret = qemu_strtoui(str, &end, 10, &val);
> > +    if (ret == 0 && end && *end == '\0') {
> > +        *u32 = val;
> > +    }
> > +    return ret;
> > +}
> >   #endif
> >   static int hdev_get_max_segments(int fd, struct stat *st)
> > @@ -1318,6 +1342,23 @@ static int hdev_get_max_segments(int fd, struct stat *st)
> >   #endif
> >   }
> > +/*
> > + * Fills in *dalign with the discard alignment and returns 0 on success,
> > + * -errno otherwise.
> > + */
> > +static int hdev_get_pdiscard_alignment(struct stat *st, uint32_t *dalign)
> > +{
> > +#ifdef CONFIG_LINUX
> > +    /*
> > +     * Note that Linux "discard_granularity" is QEMU "discard_alignment". Linux
> > +     * "discard_alignment" is something else.
> > +     */
> > +    return get_sysfs_u32_val(st, "discard_granularity", dalign);
> > +#else
> > +    return -ENOTSUP;
> > +#endif
> > +}
> > +
> >   #if defined(CONFIG_BLKZONED)
> >   /*
> >    * If the reset_all flag is true, then the wps of zone whose state is
> > @@ -1527,6 +1568,19 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
> >           }
> >       }
> > +    if (S_ISBLK(st.st_mode)) {
> > +        uint32_t dalign = 0;
> > +        int ret;
> > +
> > +        ret = hdev_get_pdiscard_alignment(&st, &dalign);
> > +        if (ret == 0) {
> > +            /* Must be a multiple of request_alignment */
> > +            assert(dalign % bs->bl.request_alignment == 0);
> 
> Is it fair to crash qemu if the kernel reports a value that is not a
> multiple of request_alignment?  Wouldn’t it make more sense to take the
> maximum, and if that still isn’t a multiple, return an error here?

I'll replace the assertion with an error.

The Linux block layer sysfs documentation says:

  [RO] Devices that support discard functionality may internally
  allocate space using units that are bigger than the logical
  block size.

I don't expect dalign to be smaller than request_alignment, but it
doesn't hurt the check if request_alignment would work.

> 
> Hanna
> 
> > +
> > +            bs->bl.pdiscard_alignment = dalign;
> > +        }
> > +    }
> > +
> >       raw_refresh_zoned_limits(bs, &st, errp);
> >   }
>
Re: [PATCH v2 1/2] file-posix: probe discard alignment on Linux block devices
Posted by Kevin Wolf 7 months ago 
Am 14.04.2025 um 17:34 hat Stefan Hajnoczi geschrieben:
> On Fri, Apr 11, 2025 at 10:15:13AM +0200, Hanna Czenczek wrote:
> > On 10.04.25 20:41, Stefan Hajnoczi wrote:
> > > Populate the pdiscard_alignment block limit so the block layer is able
> > > align discard requests correctly.
> > > 
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >   block/file-posix.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 55 insertions(+), 1 deletion(-)
> > 
> > Ah, I didn’t know sysfs is actually fair game.  Should we not also get the
> > maximum discard length then, too?
> 
> The maximum discard length behaves differently: the Linux block layer
> splits requests according to the maximum discard length. If the guest
> submits a discard request that is too large for the host, the host block
> layer will split it and the request succeeds. That is why I didn't make
> any changes to the maximum discard length in this series.

Do we need to do something with it for SCSI passthrough? Similar to how
we expose bs->bl.max_hw_transfer/iov in the block limits VPD page?

Kevin

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.