[PATCH v3 0/2] block: discard alignment fixes

[PATCH v3 0/2] block: discard alignment fixes

[Stefan Hajnoczi]

v3:
- Refine error handling when pdiscard_alignment is not a multiple of request_alignment [Hanna]
v2:
- Fix inverted logic in alignment check [Qing Wang]

Two discard alignment issues were identified in
https://issues.redhat.com/browse/RHEL-86032:
1. pdiscard_alignment is not populated for host_device in file-posix.c.
2. Misaligned head/tail discard requests are not skipped when file-posix.c
   returns -EINVAL. This causes an undesired pause when guests are configured
   with werror=stop.

Stefan Hajnoczi (2):
  file-posix: probe discard alignment on Linux block devices
  block/io: skip head/tail requests on EINVAL

 block/file-posix.c | 67 +++++++++++++++++++++++++++++++++++++++++++++-
 block/io.c         |  6 ++++-
 2 files changed, 71 insertions(+), 2 deletions(-)

-- 
2.49.0

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

[Stefan Hajnoczi]

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 | 67 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 56d1972d15..0d6e12f880 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,30 @@ 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) {
+            uint32_t ralign = bs->bl.request_alignment;
+
+            /* Probably never happens, but handle it just in case */
+            if (dalign < ralign && (ralign % dalign == 0)) {
+                dalign = ralign;
+            }
+
+            /* The block layer requires a multiple of request_alignment */
+            if (dalign % ralign != 0) {
+                error_setg(errp, "Invalid pdiscard_alignment limit %u is not a "
+                        "multiple of request_alignment %u", dalign, ralign);
+                return;
+            }
+
+            bs->bl.pdiscard_alignment = dalign;
+        }
+    }
+
     raw_refresh_zoned_limits(bs, &st, errp);
 }
 
-- 
2.49.0

[PATCH v3 2/2] block/io: skip head/tail requests on EINVAL

[Stefan Hajnoczi]

When guests send misaligned discard requests, the block layer breaks
them up into a misaligned head, an aligned main body, and a misaligned
tail.

The file-posix block driver on Linux returns -EINVAL on misaligned
discard requests. This causes bdrv_co_pdiscard() to fail and guests
configured with werror=stop will pause.

Add a special case for misaligned head/tail requests. Simply continue
when EINVAL is encountered so that the aligned main body of the request
can be completed and the guest is not paused. This is the best we can do
when guest discard limits do not match the host discard limits.

Fixes: https://issues.redhat.com/browse/RHEL-86032
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
---
 block/io.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 1ba8d1aeea..a0d0b31a3e 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3180,7 +3180,11 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset,
             }
         }
         if (ret && ret != -ENOTSUP) {
-            goto out;
+            if (ret == -EINVAL && (offset % align != 0 || num % align != 0)) {
+                /* Silently skip rejected unaligned head/tail requests */
+            } else {
+                goto out; /* bail out */
+            }
         }
 
         offset += num;
-- 
2.49.0

Re: [PATCH v3 2/2] block/io: skip head/tail requests on EINVAL

[Kevin Wolf]

Am 14.04.2025 um 22:12 hat Stefan Hajnoczi geschrieben:
> When guests send misaligned discard requests, the block layer breaks
> them up into a misaligned head, an aligned main body, and a misaligned
> tail.
> 
> The file-posix block driver on Linux returns -EINVAL on misaligned
> discard requests. This causes bdrv_co_pdiscard() to fail and guests
> configured with werror=stop will pause.
> 
> Add a special case for misaligned head/tail requests. Simply continue
> when EINVAL is encountered so that the aligned main body of the request
> can be completed and the guest is not paused. This is the best we can do
> when guest discard limits do not match the host discard limits.
> 
> Fixes: https://issues.redhat.com/browse/RHEL-86032
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

It would be good to also update the comment a bit further up:

    /* Discard is advisory, but some devices track and coalesce
     * unaligned requests, so we must pass everything down rather than
     * round here.  Still, most devices will just silently ignore
     * unaligned requests (by returning -ENOTSUP), so we must fragment
     * the request accordingly.  */

I'm not sure where the -ENOTSUP came from (Eric, do you remember?), but
we should at least mention this -EINVAL case separately.

Kevin

Re: [PATCH v3 2/2] block/io: skip head/tail requests on EINVAL

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1516 bytes --]

On Thu, Apr 17, 2025 at 10:49:55AM +0200, Kevin Wolf wrote:
> Am 14.04.2025 um 22:12 hat Stefan Hajnoczi geschrieben:
> > When guests send misaligned discard requests, the block layer breaks
> > them up into a misaligned head, an aligned main body, and a misaligned
> > tail.
> > 
> > The file-posix block driver on Linux returns -EINVAL on misaligned
> > discard requests. This causes bdrv_co_pdiscard() to fail and guests
> > configured with werror=stop will pause.
> > 
> > Add a special case for misaligned head/tail requests. Simply continue
> > when EINVAL is encountered so that the aligned main body of the request
> > can be completed and the guest is not paused. This is the best we can do
> > when guest discard limits do not match the host discard limits.
> > 
> > Fixes: https://issues.redhat.com/browse/RHEL-86032
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
> 
> It would be good to also update the comment a bit further up:
> 
>     /* Discard is advisory, but some devices track and coalesce
>      * unaligned requests, so we must pass everything down rather than
>      * round here.  Still, most devices will just silently ignore
>      * unaligned requests (by returning -ENOTSUP), so we must fragment
>      * the request accordingly.  */
> 
> I'm not sure where the -ENOTSUP came from (Eric, do you remember?), but
> we should at least mention this -EINVAL case separately.

Sounds good.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/2] block/io: skip head/tail requests on EINVAL

[Eric Blake]

On Thu, Apr 17, 2025 at 10:49:55AM +0200, Kevin Wolf wrote:
> Am 14.04.2025 um 22:12 hat Stefan Hajnoczi geschrieben:
> > When guests send misaligned discard requests, the block layer breaks
> > them up into a misaligned head, an aligned main body, and a misaligned
> > tail.
> > 
> > The file-posix block driver on Linux returns -EINVAL on misaligned
> > discard requests. This causes bdrv_co_pdiscard() to fail and guests
> > configured with werror=stop will pause.
> > 
> > Add a special case for misaligned head/tail requests. Simply continue
> > when EINVAL is encountered so that the aligned main body of the request
> > can be completed and the guest is not paused. This is the best we can do
> > when guest discard limits do not match the host discard limits.
> > 
> > Fixes: https://issues.redhat.com/browse/RHEL-86032
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
> 
> It would be good to also update the comment a bit further up:
> 
>     /* Discard is advisory, but some devices track and coalesce
>      * unaligned requests, so we must pass everything down rather than
>      * round here.  Still, most devices will just silently ignore
>      * unaligned requests (by returning -ENOTSUP), so we must fragment
>      * the request accordingly.  */
> 
> I'm not sure where the -ENOTSUP came from (Eric, do you remember?), but
> we should at least mention this -EINVAL case separately.

I don't remember if -ENOTSUP came from individual drivers, or from
actual hardware; but I agree that we are now at a point where there is
more than one errno value for obviously indicating that an unaligned
attempt was rejected as useless, and that we are best off ignoring
those errors.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

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

[Eric Blake]

On Mon, Apr 14, 2025 at 04:12:13PM -0400, 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 | 67 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 

> +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') {

This doesn't match the comment.  If we expect the file contents to end
in \n, then this should be checking *end == '\n', not '\0'.

> +        *u32 = val;
> +    }
> +    return ret;
> +}
>  #endif
>  

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

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

[Eric Blake]

On Thu, Apr 17, 2025 at 11:27:42AM -0500, Eric Blake wrote:
> On Mon, Apr 14, 2025 at 04:12:13PM -0400, 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 | 67 +++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 66 insertions(+), 1 deletion(-)
> > 
> 
> > +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') {
> 
> This doesn't match the comment.  If we expect the file contents to end
> in \n, then this should be checking *end == '\n', not '\0'.

Then again, get_sysfs_str_val() strips the trailing \n, so the code is
correct, and the comment is fishy.

But now, if we expect there to be no trailing garbage (after we've
stripped the \n when getting the string), we could simplify by passing
NULL instead of &end to qemu_strtoui, and merely rely on "if (ret == 0)".

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org