| Message ID | 20250408172924.9349-1-gouhao@uniontech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iomap: skip unnecessary ifs_block_is_uptodate check | expand |
On Wed, Apr 09, 2025 at 01:29:24AM +0800, Gou Hao wrote: > After the first 'for' loop, the first call to > ifs_block_is_uptodate always evaluates to 0. > > Signed-off-by: Gou Hao <gouhao@uniontech.com> > --- > fs/iomap/buffered-io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 31553372b33a..2f52e8e61240 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -259,7 +259,7 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio, > } > > /* truncate len if we find any trailing uptodate block(s) */ > - for ( ; i <= last; i++) { > + for (i++; i <= last; i++) { Hmmm... prior to the loop, $i is either the first !uptodate block, or it's past $last. Assuming there's no overflow (there's no combination of huge folios and tiny blksize that I can think of) then yeah, there's no point in retesting that the same block $i is uptodate since we hold the folio lock so nobody else could have set uptodate. Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> --D > if (ifs_block_is_uptodate(ifs, i)) { > plen -= (last - i + 1) * block_size; > last = i - 1; > -- > 2.20.1 > >
On 2025/4/9 23:30, Darrick J. Wong wrote: > On Wed, Apr 09, 2025 at 01:29:24AM +0800, Gou Hao wrote: >> After the first 'for' loop, the first call to >> ifs_block_is_uptodate always evaluates to 0. >> >> Signed-off-by: Gou Hao <gouhao@uniontech.com> >> --- >> fs/iomap/buffered-io.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c >> index 31553372b33a..2f52e8e61240 100644 >> --- a/fs/iomap/buffered-io.c >> +++ b/fs/iomap/buffered-io.c >> @@ -259,7 +259,7 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio, >> } >> >> /* truncate len if we find any trailing uptodate block(s) */ >> - for ( ; i <= last; i++) { >> + for (i++; i <= last; i++) { > Hmmm... prior to the loop, $i is either the first !uptodate block, or > it's past $last. Assuming there's no overflow (there's no combination > of huge folios and tiny blksize that I can think of) then yeah, there's > no point in retesting that the same block $i is uptodate since we hold > the folio lock so nobody else could have set uptodate. > > Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> > > --D Thank you for your review. The explanation is very clear. I will revise the commit message and send the V2 patch. -- thanks, Gou Hao > >> if (ifs_block_is_uptodate(ifs, i)) { >> plen -= (last - i + 1) * block_size; >> last = i - 1; >> -- >> 2.20.1 >> >>
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 31553372b33a..2f52e8e61240 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -259,7 +259,7 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio, } /* truncate len if we find any trailing uptodate block(s) */ - for ( ; i <= last; i++) { + for (i++; i <= last; i++) { if (ifs_block_is_uptodate(ifs, i)) { plen -= (last - i + 1) * block_size; last = i - 1;
After the first 'for' loop, the first call to ifs_block_is_uptodate always evaluates to 0. Signed-off-by: Gou Hao <gouhao@uniontech.com> --- fs/iomap/buffered-io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)