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

Stefan Hajnoczi posted 2 patches 8 months, 1 week ago
There is a newer version of this series
[PATCH v3 2/2] block/io: skip head/tail requests on EINVAL
Posted by Stefan Hajnoczi 8 months, 1 week ago
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
Posted by Kevin Wolf 8 months ago
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
Posted by Eric Blake 8 months ago
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 2/2] block/io: skip head/tail requests on EINVAL
Posted by Stefan Hajnoczi 8 months ago
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