On Fri, Sep 8, 2023 at 12:54 AM Denis V. Lunev <d...@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 <nir...@gmail.com>
> Date: Tue Aug 13 21:21:03 2019 +0300
> file-posix: Handle undetectable alignment
> and
> commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
> Author: Kevin Wolf <kw...@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 <d...@openvz.org>
> CC: Nir Soffer <nir...@gmail.com>
> CC: Kevin Wolf <kw...@redhat.com>
> CC: Hanna Reitz <hre...@redhat.com>
> CC: Alberto Garcia <be...@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
