On 64-bit platforms, assigning SIZE_MAX to the int64_t max_pdiscard
results in a negative value, and the following assertion would trigger
down the line (it's not the same max_pdiscard, but computed from the
other one):
qemu-system-x86_64: ../block/io.c:3166: bdrv_co_pdiscard: Assertion
`max_pdiscard >= bs->bl.request_alignment' failed.

On 32-bit platforms, it's fine to keep using SIZE_MAX.

The assertion in qemu_gluster_co_pdiscard() is checking that the value
of 'bytes' can safely be passed to glfs_discard_async(), which takes a
size_t for the argument in question, so it is kept as is. And since
max_pdiscard is still <= SIZE_MAX, relying on max_pdiscard is still
fine.

Fixes: 0c8022876f ("block: use int64_t instead of int in driver discard handlers")
Cc: qemu-stable@nongnu.org
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

v2 -> v3:
    * Keep assertion in qemu_gluster_co_pdiscard() as is.
    * Improve commit message.

v1 -> v2:
    * Use an expression that works for both 64-bit and 32-bit platforms.

 block/gluster.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/gluster.c b/block/gluster.c
index 398976bc66..b60213ab80 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -891,7 +891,7 @@ out:
 static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
-    bs->bl.max_pdiscard = SIZE_MAX;
+    bs->bl.max_pdiscard = MIN(SIZE_MAX, INT64_MAX);
 }
 
 static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
-- 
2.30.2

Am 20.05.2022 um 09:59 hat Fabian Ebner geschrieben:
> On 64-bit platforms, assigning SIZE_MAX to the int64_t max_pdiscard
> results in a negative value, and the following assertion would trigger
> down the line (it's not the same max_pdiscard, but computed from the
> other one):
> qemu-system-x86_64: ../block/io.c:3166: bdrv_co_pdiscard: Assertion
> `max_pdiscard >= bs->bl.request_alignment' failed.
> 
> On 32-bit platforms, it's fine to keep using SIZE_MAX.
> 
> The assertion in qemu_gluster_co_pdiscard() is checking that the value
> of 'bytes' can safely be passed to glfs_discard_async(), which takes a
> size_t for the argument in question, so it is kept as is. And since
> max_pdiscard is still <= SIZE_MAX, relying on max_pdiscard is still
> fine.
> 
> Fixes: 0c8022876f ("block: use int64_t instead of int in driver discard handlers")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>

Thanks, applied to the block branch.

Kevin

On 5/20/22 10:59, Fabian Ebner wrote:
> On 64-bit platforms, assigning SIZE_MAX to the int64_t max_pdiscard
> results in a negative value, and the following assertion would trigger

Oops. Good catch!

> down the line (it's not the same max_pdiscard, but computed from the
> other one):
> qemu-system-x86_64: ../block/io.c:3166: bdrv_co_pdiscard: Assertion
> `max_pdiscard >= bs->bl.request_alignment' failed.
> 
> On 32-bit platforms, it's fine to keep using SIZE_MAX.
> 
> The assertion in qemu_gluster_co_pdiscard() is checking that the value
> of 'bytes' can safely be passed to glfs_discard_async(), which takes a
> size_t for the argument in question, so it is kept as is. And since
> max_pdiscard is still <= SIZE_MAX, relying on max_pdiscard is still
> fine.
> 
> Fixes: 0c8022876f ("block: use int64_t instead of int in driver discard handlers")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>


-- 
Best regards,
Vladimir

On Fri, May 20, 2022 at 09:59:22AM +0200, Fabian Ebner wrote:
>On 64-bit platforms, assigning SIZE_MAX to the int64_t max_pdiscard
>results in a negative value, and the following assertion would trigger
>down the line (it's not the same max_pdiscard, but computed from the
>other one):
>qemu-system-x86_64: ../block/io.c:3166: bdrv_co_pdiscard: Assertion
>`max_pdiscard >= bs->bl.request_alignment' failed.
>
>On 32-bit platforms, it's fine to keep using SIZE_MAX.
>
>The assertion in qemu_gluster_co_pdiscard() is checking that the value
>of 'bytes' can safely be passed to glfs_discard_async(), which takes a
>size_t for the argument in question, so it is kept as is. And since
>max_pdiscard is still <= SIZE_MAX, relying on max_pdiscard is still
>fine.
>
>Fixes: 0c8022876f ("block: use int64_t instead of int in driver discard handlers")
>Cc: qemu-stable@nongnu.org
>Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>---
>
>v2 -> v3:
>    * Keep assertion in qemu_gluster_co_pdiscard() as is.
>    * Improve commit message.
>
>v1 -> v2:
>    * Use an expression that works for both 64-bit and 32-bit platforms.
>
> block/gluster.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/block/gluster.c b/block/gluster.c
>index 398976bc66..b60213ab80 100644
>--- a/block/gluster.c
>+++ b/block/gluster.c
>@@ -891,7 +891,7 @@ out:
> static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
> {
>     bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
>-    bs->bl.max_pdiscard = SIZE_MAX;
>+    bs->bl.max_pdiscard = MIN(SIZE_MAX, INT64_MAX);
> }
>
> static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
>-- 
>2.30.2
>
>

On Fri, May 20, 2022 at 09:59:22AM +0200, Fabian Ebner wrote:
> On 64-bit platforms, assigning SIZE_MAX to the int64_t max_pdiscard
> results in a negative value, and the following assertion would trigger
> down the line (it's not the same max_pdiscard, but computed from the
> other one):
> qemu-system-x86_64: ../block/io.c:3166: bdrv_co_pdiscard: Assertion
> `max_pdiscard >= bs->bl.request_alignment' failed.
> 
> On 32-bit platforms, it's fine to keep using SIZE_MAX.
> 
> The assertion in qemu_gluster_co_pdiscard() is checking that the value
> of 'bytes' can safely be passed to glfs_discard_async(), which takes a
> size_t for the argument in question, so it is kept as is. And since
> max_pdiscard is still <= SIZE_MAX, relying on max_pdiscard is still
> fine.
> 
> Fixes: 0c8022876f ("block: use int64_t instead of int in driver discard handlers")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org