[RFC PATCH] iomap: Enable alignment check for iomap DIO in holes

[RFC PATCH] iomap: Enable alignment check for iomap DIO in holes

[Carlos Maiolino]

Commit 09230435dffdb13de507e5e40b524b0069fc5c7b refactor iomap_dio_actor
into two different helpers for bio based IO and hole IO, although, the
latter doesn't check for IO alignment anymore.

Move the DIO alignment check up into the caller, so all iomap DIO has
alignment checked.

Fixes: 09230435d iomap: refactor iomap_dio_actor
Reported-by: Ondrej Kozina <okozina@redhat.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---

Christoph, I tagged this patch as RFC, because I couldn't really identify if you
intentionally dropped the alignment check for holes and inline actors, if you
didn't drop the alignment check intentionally, please consider it as a normal
patch.

 fs/iomap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index 90c2febc93ac..347e31512268 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -1574,9 +1574,7 @@ static loff_t
 iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 		struct iomap_dio *dio, struct iomap *iomap)
 {
-	unsigned int blkbits = blksize_bits(bdev_logical_block_size(iomap->bdev));
 	unsigned int fs_block_size = i_blocksize(inode), pad;
-	unsigned int align = iov_iter_alignment(dio->submit.iter);
 	struct iov_iter iter;
 	struct bio *bio;
 	bool need_zeroout = false;
@@ -1584,9 +1582,6 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 	int nr_pages, ret;
 	size_t copied = 0;
 
-	if ((pos | length | align) & ((1 << blkbits) - 1))
-		return -EINVAL;
-
 	if (iomap->type == IOMAP_UNWRITTEN) {
 		dio->flags |= IOMAP_DIO_UNWRITTEN;
 		need_zeroout = true;
@@ -1726,6 +1721,11 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
 		void *data, struct iomap *iomap)
 {
 	struct iomap_dio *dio = data;
+	unsigned int blkbits = blksize_bits(bdev_logical_block_size(iomap->bdev));
+	unsigned int align = iov_iter_alignment(dio->submit.iter);
+
+	if ((pos | length | align) & ((1 << blkbits) - 1))
+		return -EINVAL;
 
 	switch (iomap->type) {
 	case IOMAP_HOLE:
-- 
2.17.1

Re: [RFC PATCH] iomap: Enable alignment check for iomap DIO in holes

[Christoph Hellwig]

On Fri, Oct 05, 2018 at 03:03:16PM +0200, Carlos Maiolino wrote:
> Commit 09230435dffdb13de507e5e40b524b0069fc5c7b refactor iomap_dio_actor
> into two different helpers for bio based IO and hole IO, although, the
> latter doesn't check for IO alignment anymore.
> 
> Move the DIO alignment check up into the caller, so all iomap DIO has
> alignment checked.

Why?  The only reason to check alignmnet for actual I/O is because we
have to.  There is no requirement to align holes.

> Christoph, I tagged this patch as RFC, because I couldn't really identify if you
> intentionally dropped the alignment check for holes and inline actors, if you
> didn't drop the alignment check intentionally, please consider it as a normal
> patch.

The primary intent was to simplify the code and not enforce alignment
for inline data support.

Re: [RFC PATCH] iomap: Enable alignment check for iomap DIO in holes

[Carlos Maiolino]

On Sat, Oct 06, 2018 at 11:59:51AM +0200, Christoph Hellwig wrote:
> On Fri, Oct 05, 2018 at 03:03:16PM +0200, Carlos Maiolino wrote:
> > Commit 09230435dffdb13de507e5e40b524b0069fc5c7b refactor iomap_dio_actor
> > into two different helpers for bio based IO and hole IO, although, the
> > latter doesn't check for IO alignment anymore.
> > 
> > Move the DIO alignment check up into the caller, so all iomap DIO has
> > alignment checked.
> 
> Why?  The only reason to check alignmnet for actual I/O is because we
> have to.  There is no requirement to align holes.
> 
> > Christoph, I tagged this patch as RFC, because I couldn't really identify if you
> > intentionally dropped the alignment check for holes and inline actors, if you
> > didn't drop the alignment check intentionally, please consider it as a normal
> > patch.
> 
> The primary intent was to simplify the code and not enforce alignment
> for inline data support.

Ok, thanks, so please disregard this patch.

Cheers.


-- 
Carlos