Re: [PATCH 1/1] block: improve alignment detection and fix 271 test

[Nir Soffer <nsoffer@redhat.com>]

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

On Fri, Sep 8, 2023 at 12:54 AM Denis V. Lunev <den@openvz.org> wrote:

> Unfortunately 271 IO test is broken if started in non-cached mode.
>

Is this a real world issue? For example in oVirt you cannot create a disk
with
size < 4k so there is no way that 4k is not a good alignment.

Should we fix the test to reflect real world usage?

    _reset_img 2083k

I guess it works with:

    _reset_img 2084k

Commits
>     commit a6b257a08e3d72219f03e461a52152672fec0612
>     Author: Nir Soffer <nirsof@gmail.com>
>     Date:   Tue Aug 13 21:21:03 2019 +0300
>     file-posix: Handle undetectable alignment
> and
>     commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
>     Author: Kevin Wolf <kwolf@redhat.com>
>     Date:   Thu Jul 16 16:26:00 2020 +0200
>     block: Require aligned image size to avoid assertion failure
> have interesting side effect if used togather.
>
> If the image size is not multiple of 4k and that image falls under
> original constraints of Nil's patch, the image can not be opened
> due to the check in the bdrv_check_perm().
>
> The patch tries to satisfy the requirements of bdrv_check_perm()
> inside raw_probe_alignment(). This is at my opinion better that just
> disallowing to run that test in non-cached mode. The operation is legal
> by itself.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Nir Soffer <nirsof@gmail.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Alberto Garcia <berto@igalia.com>
> ---
>  block/file-posix.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index b16e9c21a1..988cfdc76c 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -447,8 +447,21 @@ static void raw_probe_alignment(BlockDriverState *bs,
> int fd, Error **errp)
>          for (i = 0; i < ARRAY_SIZE(alignments); i++) {
>              align = alignments[i];
>              if (raw_is_io_aligned(fd, buf, align)) {
> -                /* Fallback to safe value. */
> -                bs->bl.request_alignment = (align != 1) ? align :
> max_align;
> +                if (align != 1) {
> +                    bs->bl.request_alignment = align;
> +                    break;
> +                }
> +                /*
> +                 * Fallback to safe value. max_align is perfect, but the
> size of the device must be multiple of
> +                 * the virtual length of the device. In the other case we
> will get a error in
> +                 * bdrv_node_refresh_perm().
> +                 */
> +                for (align = max_align; align > 1; align /= 2) {
> +                    if ((bs->total_sectors * BDRV_SECTOR_SIZE) % align ==
> 0) {
>

Moving image size calculation out of the loop would make the intent of the
code
more clear:

    if (image_size % align == 0) {

Since qemu does not enforce image size alignment, I can see how you create
a 512 bytes
aligned image and in the case when qemu cannot detect the alignment, we end
with
align = 4k. In this case this loop would select align = 512, but with the
image aligned to
some strange value, this loop may select align = 2 or some other value that
does not
make sense.

So I can see using 4k or 512 bytes as a good fallback value, but anything
else should not
be possible, so maybe we should fix this in bdrv_check_perm()?

Nir

[-- Attachment #2: Type: text/html, Size: 4893 bytes --]