1. Patchew
  2. linux
  3. [PATCH v4 0/4] allow partial folio write with iomap_folio_state
  4. View patch

[PATCH 1/4] iomap: make sure iomap_adjust_read_range() are aligned with block_size

alexjlzheng@gmail.com posted 4 patches 2 weeks, 5 days ago
There is a newer version of this series
[PATCH 1/4] iomap: make sure iomap_adjust_read_range() are aligned with block_size
Posted by alexjlzheng@gmail.com 2 weeks, 5 days ago 
From: Jinliang Zheng <alexjlzheng@tencent.com>

iomap_folio_state marks the uptodate state in units of block_size, so
it is better to check that pos and length are aligned with block_size.

Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
---
 fs/iomap/buffered-io.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index fd827398afd2..0c38333933c6 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -234,6 +234,9 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
 	unsigned first = poff >> block_bits;
 	unsigned last = (poff + plen - 1) >> block_bits;
 
+	WARN_ON(*pos & (block_size - 1));
+	WARN_ON(length & (block_size - 1));
+
 	/*
 	 * If the block size is smaller than the page size, we need to check the
 	 * per-block uptodate status and adjust the offset and length if needed
-- 
2.49.0
Re: [PATCH 1/4] iomap: make sure iomap_adjust_read_range() are aligned with block_size
Posted by Pankaj Raghav (Samsung) 2 weeks, 4 days ago 
On Sat, Sep 13, 2025 at 11:37:15AM +0800, alexjlzheng@gmail.com wrote:
> From: Jinliang Zheng <alexjlzheng@tencent.com>
> 
> iomap_folio_state marks the uptodate state in units of block_size, so
> it is better to check that pos and length are aligned with block_size.
> 
> Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> ---
>  fs/iomap/buffered-io.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index fd827398afd2..0c38333933c6 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -234,6 +234,9 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
>  	unsigned first = poff >> block_bits;
>  	unsigned last = (poff + plen - 1) >> block_bits;
>  
> +	WARN_ON(*pos & (block_size - 1));
> +	WARN_ON(length & (block_size - 1));
Any reason you chose WARN_ON instead of WARN_ON_ONCE?

I don't see WARN_ON being used in iomap/buffered-io.c.
--
Pankaj
Re: [PATCH 1/4] iomap: make sure iomap_adjust_read_range() are aligned with block_size
Posted by Jinliang Zheng 2 weeks, 4 days ago 
On Sun, 14 Sep 2025 13:45:16 +0200, kernel@pankajraghav.com wrote:
> On Sat, Sep 14, 2025 at 11:37:15AM +0800, alexjlzheng@gmail.com wrote:
> > From: Jinliang Zheng <alexjlzheng@tencent.com>
> > 
> > iomap_folio_state marks the uptodate state in units of block_size, so
> > it is better to check that pos and length are aligned with block_size.
> > 
> > Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> > ---
> >  fs/iomap/buffered-io.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index fd827398afd2..0c38333933c6 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -234,6 +234,9 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
> >  	unsigned first = poff >> block_bits;
> >  	unsigned last = (poff + plen - 1) >> block_bits;
> >  
> > +	WARN_ON(*pos & (block_size - 1));
> > +	WARN_ON(length & (block_size - 1));
> Any reason you chose WARN_ON instead of WARN_ON_ONCE?

I just think it's a fatal error that deserves attention every time
it's triggered.

> 
> I don't see WARN_ON being used in iomap/buffered-io.c.

I'm not sure if there are any community guidelines for using these
two macros. If there are, please let me know and I'll be happy to
follow them as a guide.

thanks,
Jinliang Zheng. :)

> --
> Pankaj
Re: [PATCH 1/4] iomap: make sure iomap_adjust_read_range() are aligned with block_size
Posted by Pankaj Raghav (Samsung) 2 weeks, 3 days ago 
On Sun, Sep 14, 2025 at 08:40:06PM +0800, Jinliang Zheng wrote:
> On Sun, 14 Sep 2025 13:45:16 +0200, kernel@pankajraghav.com wrote:
> > On Sat, Sep 14, 2025 at 11:37:15AM +0800, alexjlzheng@gmail.com wrote:
> > > From: Jinliang Zheng <alexjlzheng@tencent.com>
> > > 
> > > iomap_folio_state marks the uptodate state in units of block_size, so
> > > it is better to check that pos and length are aligned with block_size.
> > > 
> > > Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> > > ---
> > >  fs/iomap/buffered-io.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > > index fd827398afd2..0c38333933c6 100644
> > > --- a/fs/iomap/buffered-io.c
> > > +++ b/fs/iomap/buffered-io.c
> > > @@ -234,6 +234,9 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
> > >  	unsigned first = poff >> block_bits;
> > >  	unsigned last = (poff + plen - 1) >> block_bits;
> > >  
> > > +	WARN_ON(*pos & (block_size - 1));
> > > +	WARN_ON(length & (block_size - 1));
> > Any reason you chose WARN_ON instead of WARN_ON_ONCE?
> 
> I just think it's a fatal error that deserves attention every time
> it's triggered.
> 

Is this a general change or does your later changes depend on these on
warning to work correctly?

> > 
> > I don't see WARN_ON being used in iomap/buffered-io.c.
> 
> I'm not sure if there are any community guidelines for using these
> two macros. If there are, please let me know and I'll be happy to
> follow them as a guide.

We typically use WARN_ON_ONCE to prevent spamming.

--
Pankaj
Re: [PATCH 1/4] iomap: make sure iomap_adjust_read_range() are aligned with block_size
Posted by Jinliang Zheng 2 weeks, 3 days ago 
On Mon, 15 Sep 2025 10:54:00 +0200, kernel@pankajraghav.com wrote:
> On Sun, Sep 14, 2025 at 08:40:06PM +0800, Jinliang Zheng wrote:
> > On Sun, 14 Sep 2025 13:45:16 +0200, kernel@pankajraghav.com wrote:
> > > On Sat, Sep 14, 2025 at 11:37:15AM +0800, alexjlzheng@gmail.com wrote:
> > > > From: Jinliang Zheng <alexjlzheng@tencent.com>
> > > > 
> > > > iomap_folio_state marks the uptodate state in units of block_size, so
> > > > it is better to check that pos and length are aligned with block_size.
> > > > 
> > > > Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> > > > ---
> > > >  fs/iomap/buffered-io.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > > > index fd827398afd2..0c38333933c6 100644
> > > > --- a/fs/iomap/buffered-io.c
> > > > +++ b/fs/iomap/buffered-io.c
> > > > @@ -234,6 +234,9 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
> > > >  	unsigned first = poff >> block_bits;
> > > >  	unsigned last = (poff + plen - 1) >> block_bits;
> > > >  
> > > > +	WARN_ON(*pos & (block_size - 1));
> > > > +	WARN_ON(length & (block_size - 1));
> > > Any reason you chose WARN_ON instead of WARN_ON_ONCE?
> > 
> > I just think it's a fatal error that deserves attention every time
> > it's triggered.
> > 
> 
> Is this a general change or does your later changes depend on these on
> warning to work correctly?

No, there is no functional change.

I added it only because the correctness of iomap_adjust_read_range() depends on
it, so it's better to hightlight it now.

```
	/* move forward for each leading block marked uptodate */
	for (i = first; i <= last; i++) {
		if (!ifs_block_is_uptodate(ifs, i))
			break;
		*pos += block_size; <-------------------- if not aligned, ...
		poff += block_size;
		plen -= block_size;
		first++;
	}
```

> 
> > > 
> > > I don't see WARN_ON being used in iomap/buffered-io.c.
> > 
> > I'm not sure if there are any community guidelines for using these
> > two macros. If there are, please let me know and I'll be happy to
> > follow them as a guide.
> 
> We typically use WARN_ON_ONCE to prevent spamming.

If you think it's better, I will send a new version.

thanks,
Jinliang Zheng. :)

> 
> --
> Pankaj

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.