nilfs2: fix block-offset handling in iomap reads

[PATCH] nilfs2: fix block-offset handling in iomap reads

Posted by Linmao Li 2 months ago

nilfs_iomap_begin() looks up the filesystem block containing the requested
offset and returns the physical address of that block.  However, it sets
iomap->offset to the original, possibly sub-block, offset while leaving
iomap->addr at the start of the physical block.

iomap_sector() adds the difference between the I/O position and
iomap->offset to iomap->addr.  For an O_DIRECT read at offset 512 on a
filesystem with 4 KiB blocks, the two file offsets are equal and the I/O is
therefore submitted at the start of the physical block instead of 512 bytes
into it.  iomap direct I/O permits this alignment when the device logical
block size is 512 bytes.

Describe mapped and hole extents from the filesystem-block-aligned file
offset.  This makes the physical and file offsets refer to the same byte.
It also prevents a one-block hole mapping from extending into the following
block.

Fixes: b924d8d4e54f ("nilfs2: switch O_DIRECT reads to iomap")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 fs/nilfs2/iomap.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/nilfs2/iomap.c b/fs/nilfs2/iomap.c
index 3ae3bf6ed3686..e130ed63abd9a 100644
--- a/fs/nilfs2/iomap.c
+++ b/fs/nilfs2/iomap.c
@@ -18,6 +18,7 @@ static int nilfs_iomap_begin(struct inode *inode, loff_t offset,
 	struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
 	struct nilfs_inode_info *ii = NILFS_I(inode);
 	sector_t blkoff = offset >> inode->i_blkbits;
+	loff_t iomap_offset = (loff_t)blkoff << inode->i_blkbits;
 	unsigned int maxblocks;
 	__u64 blknum = 0;
 	int ret;
@@ -50,15 +51,15 @@ static int nilfs_iomap_begin(struct inode *inode, loff_t offset,
 	if (ret == -ENOENT) {
 		iomap->type = IOMAP_HOLE;
 		iomap->addr = IOMAP_NULL_ADDR;
-		iomap->offset = offset;
-		iomap->length = min_t(loff_t, length, i_blocksize(inode));
+		iomap->offset = iomap_offset;
+		iomap->length = i_blocksize(inode);
 		return 0;
 	} else if (ret < 0)
 		return ret;
 
 	iomap->bdev = inode->i_sb->s_bdev;
-	iomap->offset = offset;
-	iomap->length = min_t(loff_t, length, (loff_t)ret << inode->i_blkbits);
+	iomap->offset = iomap_offset;
+	iomap->length = (loff_t)ret << inode->i_blkbits;
 	iomap->addr = (loff_t)blknum << inode->i_blkbits;
 	iomap->type = IOMAP_MAPPED;
 	iomap->flags = IOMAP_F_MERGED;

base-commit: d5c57d9bf03516d625521cd6cf5acfd93e3e63c7
-- 
2.25.1

Re: [PATCH] nilfs2: fix block-offset handling in iomap reads

Posted by Ryusuke Konishi 2 months ago

On Fri, Jul 31, 2026 at 5:38 PM Linmao Li wrote:
>
> nilfs_iomap_begin() looks up the filesystem block containing the requested
> offset and returns the physical address of that block.  However, it sets
> iomap->offset to the original, possibly sub-block, offset while leaving
> iomap->addr at the start of the physical block.
>
> iomap_sector() adds the difference between the I/O position and
> iomap->offset to iomap->addr.  For an O_DIRECT read at offset 512 on a
> filesystem with 4 KiB blocks, the two file offsets are equal and the I/O is
> therefore submitted at the start of the physical block instead of 512 bytes
> into it.  iomap direct I/O permits this alignment when the device logical
> block size is 512 bytes.
>
> Describe mapped and hole extents from the filesystem-block-aligned file
> offset.  This makes the physical and file offsets refer to the same byte.
> It also prevents a one-block hole mapping from extending into the following
> block.
>
> Fixes: b924d8d4e54f ("nilfs2: switch O_DIRECT reads to iomap")
> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> ---
>  fs/nilfs2/iomap.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nilfs2/iomap.c b/fs/nilfs2/iomap.c
> index 3ae3bf6ed3686..e130ed63abd9a 100644
> --- a/fs/nilfs2/iomap.c
> +++ b/fs/nilfs2/iomap.c
> @@ -18,6 +18,7 @@ static int nilfs_iomap_begin(struct inode *inode, loff_t offset,
>         struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
>         struct nilfs_inode_info *ii = NILFS_I(inode);
>         sector_t blkoff = offset >> inode->i_blkbits;
> +       loff_t iomap_offset = (loff_t)blkoff << inode->i_blkbits;
>         unsigned int maxblocks;
>         __u64 blknum = 0;
>         int ret;
> @@ -50,15 +51,15 @@ static int nilfs_iomap_begin(struct inode *inode, loff_t offset,
>         if (ret == -ENOENT) {
>                 iomap->type = IOMAP_HOLE;
>                 iomap->addr = IOMAP_NULL_ADDR;
> -               iomap->offset = offset;
> -               iomap->length = min_t(loff_t, length, i_blocksize(inode));
> +               iomap->offset = iomap_offset;
> +               iomap->length = i_blocksize(inode);
>                 return 0;
>         } else if (ret < 0)
>                 return ret;
>
>         iomap->bdev = inode->i_sb->s_bdev;
> -       iomap->offset = offset;
> -       iomap->length = min_t(loff_t, length, (loff_t)ret << inode->i_blkbits);
> +       iomap->offset = iomap_offset;
> +       iomap->length = (loff_t)ret << inode->i_blkbits;
>         iomap->addr = (loff_t)blknum << inode->i_blkbits;
>         iomap->type = IOMAP_MAPPED;
>         iomap->flags = IOMAP_F_MERGED;
>
> base-commit: d5c57d9bf03516d625521cd6cf5acfd93e3e63c7
> --
> 2.25.1

Hi Linmao,

Since the O_DIRECT iomap conversion patch series is currently under
review and subject to revision, please post your finding as comments
on the original patch thread in the future, rather than sending a
standalone patch with a premature Fixes tag.

Hi Viacheslav,
I believe the point being made is that a misalignment occurs because
iomap->addr is adjusted to the block boundary, whereas iomap->offset
is not.  Could you take a look at his points ?

Thanks,
Ryusuke Konishi

Re: [PATCH] nilfs2: fix block-offset handling in iomap reads

Posted by Viacheslav Dubeyko 2 months ago

On Fri, 2026-07-31 at 21:05 +0900, Ryusuke Konishi wrote:
> On Fri, Jul 31, 2026 at 5:38 PM Linmao Li wrote:
> > 
> > nilfs_iomap_begin() looks up the filesystem block containing the
> > requested
> > offset and returns the physical address of that block.  However, it
> > sets
> > iomap->offset to the original, possibly sub-block, offset while
> > leaving
> > iomap->addr at the start of the physical block.
> > 
> > iomap_sector() adds the difference between the I/O position and
> > iomap->offset to iomap->addr.  For an O_DIRECT read at offset 512
> > on a
> > filesystem with 4 KiB blocks, the two file offsets are equal and
> > the I/O is
> > therefore submitted at the start of the physical block instead of
> > 512 bytes
> > into it.  iomap direct I/O permits this alignment when the device
> > logical
> > block size is 512 bytes.
> > 
> > Describe mapped and hole extents from the filesystem-block-aligned
> > file
> > offset.  This makes the physical and file offsets refer to the same
> > byte.
> > It also prevents a one-block hole mapping from extending into the
> > following
> > block.
> > 
> > Fixes: b924d8d4e54f ("nilfs2: switch O_DIRECT reads to iomap")
> > Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> > ---
> >  fs/nilfs2/iomap.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nilfs2/iomap.c b/fs/nilfs2/iomap.c
> > index 3ae3bf6ed3686..e130ed63abd9a 100644
> > --- a/fs/nilfs2/iomap.c
> > +++ b/fs/nilfs2/iomap.c
> > @@ -18,6 +18,7 @@ static int nilfs_iomap_begin(struct inode *inode,
> > loff_t offset,
> >         struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
> >         struct nilfs_inode_info *ii = NILFS_I(inode);
> >         sector_t blkoff = offset >> inode->i_blkbits;
> > +       loff_t iomap_offset = (loff_t)blkoff << inode->i_blkbits;
> >         unsigned int maxblocks;
> >         __u64 blknum = 0;
> >         int ret;
> > @@ -50,15 +51,15 @@ static int nilfs_iomap_begin(struct inode
> > *inode, loff_t offset,
> >         if (ret == -ENOENT) {
> >                 iomap->type = IOMAP_HOLE;
> >                 iomap->addr = IOMAP_NULL_ADDR;
> > -               iomap->offset = offset;
> > -               iomap->length = min_t(loff_t, length,
> > i_blocksize(inode));
> > +               iomap->offset = iomap_offset;
> > +               iomap->length = i_blocksize(inode);
> >                 return 0;
> >         } else if (ret < 0)
> >                 return ret;
> > 
> >         iomap->bdev = inode->i_sb->s_bdev;
> > -       iomap->offset = offset;
> > -       iomap->length = min_t(loff_t, length, (loff_t)ret << inode-
> > >i_blkbits);
> > +       iomap->offset = iomap_offset;
> > +       iomap->length = (loff_t)ret << inode->i_blkbits;
> >         iomap->addr = (loff_t)blknum << inode->i_blkbits;
> >         iomap->type = IOMAP_MAPPED;
> >         iomap->flags = IOMAP_F_MERGED;
> > 
> > base-commit: d5c57d9bf03516d625521cd6cf5acfd93e3e63c7
> > --
> > 2.25.1
> 
> Hi Linmao,
> 
> Since the O_DIRECT iomap conversion patch series is currently under
> review and subject to revision, please post your finding as comments
> on the original patch thread in the future, rather than sending a
> standalone patch with a premature Fixes tag.
> 
> Hi Viacheslav,
> I believe the point being made is that a misalignment occurs because
> iomap->addr is adjusted to the block boundary, whereas iomap->offset
> is not.  Could you take a look at his points ?
> 

Hi Ryusuke,

The fix makes sense to me. I am attending FMS 2026 (August 4th - 6th).
So, I don't have time for preparing v2 of the patchset with adding
Linmao's fix during this week. Let me try to find the time for pathset
reworking around the end of this week.

Thanks,
Slava.