At the end of a reopen, we already call bdrv_refresh_limits(), which
should update bs->request_alignment according to the new file
descriptor. However, raw_probe_alignment() relies on s->needs_alignment
and just uses 1 if it isn't set. We neglected to update this field, so
starting with cache=writeback and then reopening with cache=none means
that we get an incorrect bs->request_alignment == 1 and unaligned
requests fail instead of being automatically aligned.

Fix this by recalculating s->needs_alignment in raw_refresh_limits()
before calling raw_probe_alignment().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c         | 20 ++++++++++++++++----
 tests/qemu-iotests/142     | 22 ++++++++++++++++++++++
 tests/qemu-iotests/142.out | 15 +++++++++++++++
 3 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 7a27c83060..3f14e47096 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -167,6 +167,7 @@ typedef struct BDRVRawState {
     int page_cache_inconsistent; /* errno from fdatasync failure */
     bool has_fallocate;
     bool needs_alignment;
+    bool force_alignment;
     bool drop_cache;
     bool check_cache_dropped;
     struct {
@@ -351,6 +352,17 @@ static bool dio_byte_aligned(int fd)
     return false;
 }
 
+static int raw_needs_alignment(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+
+    if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) {
+        return true;
+    }
+
+    return s->force_alignment;
+}
+
 /* Check if read is allowed with given memory buffer and length.
  *
  * This function is used to check O_DIRECT memory buffer and request alignment.
@@ -728,9 +740,6 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
 
     s->has_discard = true;
     s->has_write_zeroes = true;
-    if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) {
-        s->needs_alignment = true;
-    }
 
     if (fstat(s->fd, &st) < 0) {
         ret = -errno;
@@ -784,9 +793,10 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
          * so QEMU makes sure all IO operations on the device are aligned
          * to sector size, or else FreeBSD will reject them with EINVAL.
          */
-        s->needs_alignment = true;
+        s->force_alignment = true;
     }
 #endif
+    s->needs_alignment = raw_needs_alignment(bs);
 
 #ifdef CONFIG_XFS
     if (platform_test_xfs_fd(s->fd)) {
@@ -1251,7 +1261,9 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
     BDRVRawState *s = bs->opaque;
     struct stat st;
 
+    s->needs_alignment = raw_needs_alignment(bs);
     raw_probe_alignment(bs, s->fd, errp);
+
     bs->bl.min_mem_alignment = s->buf_align;
     bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size);
 
diff --git a/tests/qemu-iotests/142 b/tests/qemu-iotests/142
index 69fd10ef51..07003c597a 100755
--- a/tests/qemu-iotests/142
+++ b/tests/qemu-iotests/142
@@ -350,6 +350,28 @@ info block backing-file"
 
 echo "$hmp_cmds" | run_qemu -drive "$files","$ids" | grep "Cache"
 
+echo
+echo "--- Alignment after changing O_DIRECT ---"
+echo
+
+# Directly test the protocol level: Can unaligned requests succeed even if
+# O_DIRECT was only enabled through a reopen and vice versa?
+
+$QEMU_IO --cache=writeback -f file $TEST_IMG <<EOF | _filter_qemu_io
+read 42 42
+reopen -o cache.direct=on
+read 42 42
+reopen -o cache.direct=off
+read 42 42
+EOF
+$QEMU_IO --cache=none -f file $TEST_IMG <<EOF | _filter_qemu_io
+read 42 42
+reopen -o cache.direct=off
+read 42 42
+reopen -o cache.direct=on
+read 42 42
+EOF
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/142.out b/tests/qemu-iotests/142.out
index a92b948edd..f167038230 100644
--- a/tests/qemu-iotests/142.out
+++ b/tests/qemu-iotests/142.out
@@ -747,4 +747,19 @@ cache.no-flush=on on backing-file
     Cache mode:       writeback
     Cache mode:       writeback, direct
     Cache mode:       writeback, ignore flushes
+
+--- Alignment after changing O_DIRECT ---
+
+read 42/42 bytes at offset 42
+42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 42/42 bytes at offset 42
+42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 42/42 bytes at offset 42
+42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 42/42 bytes at offset 42
+42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 42/42 bytes at offset 42
+42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 42/42 bytes at offset 42
+42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
2.31.1

On Thu, Nov 04, 2021 at 12:31:09PM +0100, Kevin Wolf wrote:
>At the end of a reopen, we already call bdrv_refresh_limits(), which
>should update bs->request_alignment according to the new file
>descriptor. However, raw_probe_alignment() relies on s->needs_alignment
>and just uses 1 if it isn't set. We neglected to update this field, so
>starting with cache=writeback and then reopening with cache=none means
>that we get an incorrect bs->request_alignment == 1 and unaligned
>requests fail instead of being automatically aligned.
>
>Fix this by recalculating s->needs_alignment in raw_refresh_limits()
>before calling raw_probe_alignment().
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> block/file-posix.c         | 20 ++++++++++++++++----
> tests/qemu-iotests/142     | 22 ++++++++++++++++++++++
> tests/qemu-iotests/142.out | 15 +++++++++++++++
> 3 files changed, 53 insertions(+), 4 deletions(-)
>
>diff --git a/block/file-posix.c b/block/file-posix.c
>index 7a27c83060..3f14e47096 100644
>--- a/block/file-posix.c
>+++ b/block/file-posix.c
>@@ -167,6 +167,7 @@ typedef struct BDRVRawState {
>     int page_cache_inconsistent; /* errno from fdatasync failure */
>     bool has_fallocate;
>     bool needs_alignment;
>+    bool force_alignment;
>     bool drop_cache;
>     bool check_cache_dropped;
>     struct {
>@@ -351,6 +352,17 @@ static bool dio_byte_aligned(int fd)
>     return false;
> }
>
>+static int raw_needs_alignment(BlockDriverState *bs)

If you need to respin, maybe it's better to use `bool` as return type.

In both cases:
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Stefano

Am 04.11.2021 um 15:20 hat Stefano Garzarella geschrieben:
> On Thu, Nov 04, 2021 at 12:31:09PM +0100, Kevin Wolf wrote:
> > At the end of a reopen, we already call bdrv_refresh_limits(), which
> > should update bs->request_alignment according to the new file
> > descriptor. However, raw_probe_alignment() relies on s->needs_alignment
> > and just uses 1 if it isn't set. We neglected to update this field, so
> > starting with cache=writeback and then reopening with cache=none means
> > that we get an incorrect bs->request_alignment == 1 and unaligned
> > requests fail instead of being automatically aligned.
> > 
> > Fix this by recalculating s->needs_alignment in raw_refresh_limits()
> > before calling raw_probe_alignment().
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > block/file-posix.c         | 20 ++++++++++++++++----
> > tests/qemu-iotests/142     | 22 ++++++++++++++++++++++
> > tests/qemu-iotests/142.out | 15 +++++++++++++++
> > 3 files changed, 53 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 7a27c83060..3f14e47096 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -167,6 +167,7 @@ typedef struct BDRVRawState {
> >     int page_cache_inconsistent; /* errno from fdatasync failure */
> >     bool has_fallocate;
> >     bool needs_alignment;
> > +    bool force_alignment;
> >     bool drop_cache;
> >     bool check_cache_dropped;
> >     struct {
> > @@ -351,6 +352,17 @@ static bool dio_byte_aligned(int fd)
> >     return false;
> > }
> > 
> > +static int raw_needs_alignment(BlockDriverState *bs)
> 
> If you need to respin, maybe it's better to use `bool` as return type.

Yes, that's what it should be. Can be fixed up while applying. I had a
0/1/-errno return value in an intermediate version, which is how it
became 'int', but it's not necessary any more in this version.

> In both cases:
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks!

Kevin

On 04.11.21 12:31, Kevin Wolf wrote:
> At the end of a reopen, we already call bdrv_refresh_limits(), which
> should update bs->request_alignment according to the new file
> descriptor. However, raw_probe_alignment() relies on s->needs_alignment
> and just uses 1 if it isn't set. We neglected to update this field, so
> starting with cache=writeback and then reopening with cache=none means
> that we get an incorrect bs->request_alignment == 1 and unaligned
> requests fail instead of being automatically aligned.
>
> Fix this by recalculating s->needs_alignment in raw_refresh_limits()
> before calling raw_probe_alignment().
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/file-posix.c         | 20 ++++++++++++++++----
>   tests/qemu-iotests/142     | 22 ++++++++++++++++++++++
>   tests/qemu-iotests/142.out | 15 +++++++++++++++
>   3 files changed, 53 insertions(+), 4 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>