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

[Hanna Czenczek <hreitz@redhat.com>]

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);
>   }
>