Re: [PATCHv2] block: use the request length for iov alignment

[Keith Busch]

On Thu, Sep 29, 2022 at 07:59:50PM +0200, Kevin Wolf wrote:
> Am 29.09.2022 um 18:09 hat Keith Busch geschrieben:
> > On Fri, Sep 23, 2022 at 08:34:51AM -0700, Keith Busch wrote:
> > > 
> > > An iov length needs to be aligned to the logical block size, which may
> > > be larger than the memory alignment. And since this is only used with
> > > file-posix backing storage, move the alignment function to there, where
> > > the value of the request_alignment is known to be the file's logical
> > > block size.
> > 
> > Any objections to this version? This is fixing real bug reports that
> > may become more frequent without this patch.
> 
> I think it is okay. Splitting it in two patches would have been nicer
> (one for moving code, one for making the change), but it's small enough
> that I can ignore that. I'll probably merge it tomorrow.

I agree that splitting makes the functional change stand out, otherwise a
casual look may mistake the patch as a simple function move. I'll send you a
new version.

Re: [PATCHv2] block: use the request length for iov alignment

[Kevin Wolf]

Am 29.09.2022 um 18:09 hat Keith Busch geschrieben:
> On Fri, Sep 23, 2022 at 08:34:51AM -0700, Keith Busch wrote:
> > 
> > An iov length needs to be aligned to the logical block size, which may
> > be larger than the memory alignment. And since this is only used with
> > file-posix backing storage, move the alignment function to there, where
> > the value of the request_alignment is known to be the file's logical
> > block size.
> 
> Any objections to this version? This is fixing real bug reports that
> may become more frequent without this patch.

I think it is okay. Splitting it in two patches would have been nicer
(one for moving code, one for making the change), but it's small enough
that I can ignore that. I'll probably merge it tomorrow.

Kevin

Re: [PATCHv2] block: use the request length for iov alignment

[Keith Busch]

On Fri, Sep 23, 2022 at 08:34:51AM -0700, Keith Busch wrote:
> 
> An iov length needs to be aligned to the logical block size, which may
> be larger than the memory alignment. And since this is only used with
> file-posix backing storage, move the alignment function to there, where
> the value of the request_alignment is known to be the file's logical
> block size.

Any objections to this version? This is fixing real bug reports that may become
more frequent without this patch.

[PATCHv2] block: use the request length for iov alignment

[Keith Busch]

From: Keith Busch 

An iov length needs to be aligned to the logical block size, which may
be larger than the memory alignment. And since this is only used with
file-posix backing storage, move the alignment function to there, where
the value of the request_alignment is known to be the file's logical
block size.

Signed-off-by: Keith Busch 
---
v1->v2:

  Relocate the function to the only caller so that irrelavant
  constraints don't need to be considered.

 block/file-posix.c   | 22 ++
 block/io.c   | 21 -
 include/block/block-io.h |  1 -
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 48cd096624..af994aba2b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2061,6 +2061,28 @@ static int coroutine_fn 
raw_thread_pool_submit(BlockDriverState *bs,
 return thread_pool_submit_co(pool, func, arg);
 }
 
+/*
+ * Check if all memory in this vector is sector aligned.
+ */
+static bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
+{
+int i;
+size_t alignment = bdrv_min_mem_align(bs);
+size_t len = bs->bl.request_alignment;
+IO_CODE();
+
+for (i = 0; i < qiov->niov; i++) {
+if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
+return false;
+}
+if (qiov->iov[i].iov_len % len) {
+return false;
+}
+}
+
+return true;
+}
+
 static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov, int 
type)
 {
diff --git a/block/io.c b/block/io.c
index 0a8cbefe86..96edc7f7cb 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3236,27 +3236,6 @@ void *qemu_try_blockalign0(BlockDriverState *bs, size_t 
size)
 return mem;
 }
 
-/*
- * Check if all memory in this vector is sector aligned.
- */
-bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
-{
-int i;
-size_t alignment = bdrv_min_mem_align(bs);
-IO_CODE();
-
-for (i = 0; i < qiov->niov; i++) {
-if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
-return false;
-}
-if (qiov->iov[i].iov_len % alignment) {
-return false;
-}
-}
-
-return true;
-}
-
 void bdrv_io_plug(BlockDriverState *bs)
 {
 BdrvChild *child;
diff --git a/include/block/block-io.h b/include/block/block-io.h
index fd25ffa9be..492f95fc05 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -150,7 +150,6 @@ void *qemu_blockalign(BlockDriverState *bs, size_t size);
 void *qemu_blockalign0(BlockDriverState *bs, size_t size);
 void *qemu_try_blockalign(BlockDriverState *bs, size_t size);
 void *qemu_try_blockalign0(BlockDriverState *bs, size_t size);
-bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
 
 void bdrv_enable_copy_on_read(BlockDriverState *bs);
 void bdrv_disable_copy_on_read(BlockDriverState *bs);
-- 
2.30.2