* Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
2023-09-07 21:53 [PATCH 1/1] block: improve alignment detection and fix 271 test Denis V. Lunev
@ 2023-09-19 11:53 ` Denis V. Lunev
2023-09-26 20:02 ` Denis V. Lunev
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Denis V. Lunev @ 2023-09-19 11:53 UTC (permalink / raw)
To: Denis V. Lunev, qemu-devel, qemu-block
Cc: Nir Soffer, Kevin Wolf, Hanna Reitz, Alberto Garcia
On 9/7/23 23:53, Denis V. Lunev wrote:
> Unfortunately 271 IO test is broken if started in non-cached mode.
>
> 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) {
> + break;
> + }
> + }
> + bs->bl.request_alignment = align;
> break;
> }
> }
ping
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
2023-09-07 21:53 [PATCH 1/1] block: improve alignment detection and fix 271 test Denis V. Lunev
2023-09-19 11:53 ` Denis V. Lunev
@ 2023-09-26 20:02 ` Denis V. Lunev
2023-10-02 7:29 ` Denis V. Lunev
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Denis V. Lunev @ 2023-09-26 20:02 UTC (permalink / raw)
To: Denis V. Lunev, qemu-devel, qemu-block
Cc: Nir Soffer, Kevin Wolf, Hanna Reitz, Alberto Garcia
On 9/7/23 23:53, Denis V. Lunev wrote:
> Unfortunately 271 IO test is broken if started in non-cached mode.
>
> 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) {
> + break;
> + }
> + }
> + bs->bl.request_alignment = align;
> break;
> }
> }
ping v2
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
2023-09-07 21:53 [PATCH 1/1] block: improve alignment detection and fix 271 test Denis V. Lunev
2023-09-19 11:53 ` Denis V. Lunev
2023-09-26 20:02 ` Denis V. Lunev
@ 2023-10-02 7:29 ` Denis V. Lunev
2023-10-13 9:54 ` Denis V. Lunev
2023-10-14 11:23 ` Nir Soffer
4 siblings, 0 replies; 6+ messages in thread
From: Denis V. Lunev @ 2023-10-02 7:29 UTC (permalink / raw)
To: Denis V. Lunev, qemu-devel, qemu-block
Cc: Nir Soffer, Kevin Wolf, Hanna Reitz, Alberto Garcia
On 9/7/23 23:53, Denis V. Lunev wrote:
> Unfortunately 271 IO test is broken if started in non-cached mode.
>
> 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) {
> + break;
> + }
> + }
> + bs->bl.request_alignment = align;
> break;
> }
> }
ping v3
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
2023-09-07 21:53 [PATCH 1/1] block: improve alignment detection and fix 271 test Denis V. Lunev
` (2 preceding siblings ...)
2023-10-02 7:29 ` Denis V. Lunev
@ 2023-10-13 9:54 ` Denis V. Lunev
2023-10-14 11:23 ` Nir Soffer
4 siblings, 0 replies; 6+ messages in thread
From: Denis V. Lunev @ 2023-10-13 9:54 UTC (permalink / raw)
To: Denis V. Lunev, qemu-devel, qemu-block
Cc: Nir Soffer, Kevin Wolf, Hanna Reitz, Alberto Garcia
On 9/7/23 23:53, Denis V. Lunev wrote:
> Unfortunately 271 IO test is broken if started in non-cached mode.
>
> 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) {
> + break;
> + }
> + }
> + bs->bl.request_alignment = align;
> break;
> }
> }
ping
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
2023-09-07 21:53 [PATCH 1/1] block: improve alignment detection and fix 271 test Denis V. Lunev
` (3 preceding siblings ...)
2023-10-13 9:54 ` Denis V. Lunev
@ 2023-10-14 11:23 ` Nir Soffer
4 siblings, 0 replies; 6+ messages in thread
From: Nir Soffer @ 2023-10-14 11:23 UTC (permalink / raw)
To: Denis V. Lunev
Cc: qemu-devel, qemu-block, Nir Soffer, Kevin Wolf, Hanna Reitz,
Alberto Garcia
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread