Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
On Fri, Sep 8, 2023 at 12:54 AM Denis V. Lunev 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
> Date: Tue Aug 13 21:21:03 2019 +0300
> file-posix: Handle undetectable alignment
> and
> commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
> Author: Kevin Wolf
> 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
> CC: Nir Soffer
> CC: Kevin Wolf
> CC: Hanna Reitz
> CC: Alberto Garcia
> ---
> 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
Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
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
Date: Tue Aug 13 21:21:03 2019 +0300
file-posix: Handle undetectable alignment
and
commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
Author: Kevin Wolf
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
CC: Nir Soffer
CC: Kevin Wolf
CC: Hanna Reitz
CC: Alberto Garcia
---
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
Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
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
Date: Tue Aug 13 21:21:03 2019 +0300
file-posix: Handle undetectable alignment
and
commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
Author: Kevin Wolf
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
CC: Nir Soffer
CC: Kevin Wolf
CC: Hanna Reitz
CC: Alberto Garcia
---
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
Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
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
Date: Tue Aug 13 21:21:03 2019 +0300
file-posix: Handle undetectable alignment
and
commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
Author: Kevin Wolf
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
CC: Nir Soffer
CC: Kevin Wolf
CC: Hanna Reitz
CC: Alberto Garcia
---
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
Re: [PATCH 1/1] block: improve alignment detection and fix 271 test
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
Date: Tue Aug 13 21:21:03 2019 +0300
file-posix: Handle undetectable alignment
and
commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
Author: Kevin Wolf
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
CC: Nir Soffer
CC: Kevin Wolf
CC: Hanna Reitz
CC: Alberto Garcia
---
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
[PATCH 1/1] block: improve alignment detection and fix 271 test
Unfortunately 271 IO test is broken if started in non-cached mode.
Commits
commit a6b257a08e3d72219f03e461a52152672fec0612
Author: Nir Soffer
Date: Tue Aug 13 21:21:03 2019 +0300
file-posix: Handle undetectable alignment
and
commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
Author: Kevin Wolf
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
CC: Nir Soffer
CC: Kevin Wolf
CC: Hanna Reitz
CC: Alberto Garcia
---
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;
}
}
--
2.34.1
