| Message ID | 20260514062955.1183976-2-yi.zhang@huaweicloud.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iomap: trivial fixes for ext4 conversion | expand |
On Thu, May 14, 2026 at 02:29:52PM +0800, Zhang Yi wrote: > From: Zhang Yi <yi.zhang@huawei.com> > > The block range calculation in ifs_clear_range_dirty() is incorrect when > partially clearing a range in a folio. We cannot clear the dirty bit of > the first block or the last block if the start or end offset is not > blocksize-aligned. This has not yet caused any issues since we always > clear a whole folio in iomap_writeback_folio(). > > Fix this by rounding up the first block to blocksize alignment, and > calculate the last block by rounding down (using truncation). Correct > the nr_blks calculation accordingly. > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> Cc: <stable@vger.kernel.org> # v6.6 Fixes: 4ce02c67972211 ("iomap: Add per-block dirty state tracking to improve performance") > --- > This is modified from: > https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-2-yi.zhang@huaweicloud.com/ > Changes: > - Use round_up() instead of DIV_ROUND_UP() to prevent wasted integer > division. > > fs/iomap/buffered-io.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index d7b648421a70..64351a448a8b 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -176,13 +176,17 @@ static void ifs_clear_range_dirty(struct folio *folio, > { > struct inode *inode = folio->mapping->host; > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio); > - unsigned int first_blk = (off >> inode->i_blkbits); > - unsigned int last_blk = (off + len - 1) >> inode->i_blkbits; > - unsigned int nr_blks = last_blk - first_blk + 1; > + unsigned int first_blk = round_up(off, i_blocksize(inode)) >> > + inode->i_blkbits; Ok, so now we round off up to the next fsblock to compute first_blk... > + unsigned int last_blk = (off + len) >> inode->i_blkbits; ...and last_blk (which is really the next block number after the range that we're undirtying) is now rounded down. Presumably off/len have to be aligned to fsblock granularity so we'll never have to deal with unaligned situations like (off=324,len=1), right? > unsigned long flags; > > + if (first_blk >= last_blk) Do we need this check? When would the test actually be true? --D > + return; > + > spin_lock_irqsave(&ifs->state_lock, flags); > - bitmap_clear(ifs->state, first_blk + blks_per_folio, nr_blks); > + bitmap_clear(ifs->state, first_blk + blks_per_folio, > + last_blk - first_blk); > spin_unlock_irqrestore(&ifs->state_lock, flags); > } > > -- > 2.52.0 > >
On 5/15/2026 2:03 AM, Darrick J. Wong wrote: > On Thu, May 14, 2026 at 02:29:52PM +0800, Zhang Yi wrote: >> From: Zhang Yi <yi.zhang@huawei.com> >> >> The block range calculation in ifs_clear_range_dirty() is incorrect when >> partially clearing a range in a folio. We cannot clear the dirty bit of >> the first block or the last block if the start or end offset is not >> blocksize-aligned. This has not yet caused any issues since we always >> clear a whole folio in iomap_writeback_folio(). >> >> Fix this by rounding up the first block to blocksize alignment, and >> calculate the last block by rounding down (using truncation). Correct >> the nr_blks calculation accordingly. >> >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > > Cc: <stable@vger.kernel.org> # v6.6 > Fixes: 4ce02c67972211 ("iomap: Add per-block dirty state tracking to improve performance") > >> --- >> This is modified from: >> https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-2-yi.zhang@huaweicloud.com/ >> Changes: >> - Use round_up() instead of DIV_ROUND_UP() to prevent wasted integer >> division. >> >> fs/iomap/buffered-io.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c >> index d7b648421a70..64351a448a8b 100644 >> --- a/fs/iomap/buffered-io.c >> +++ b/fs/iomap/buffered-io.c >> @@ -176,13 +176,17 @@ static void ifs_clear_range_dirty(struct folio *folio, >> { >> struct inode *inode = folio->mapping->host; >> unsigned int blks_per_folio = i_blocks_per_folio(inode, folio); >> - unsigned int first_blk = (off >> inode->i_blkbits); >> - unsigned int last_blk = (off + len - 1) >> inode->i_blkbits; >> - unsigned int nr_blks = last_blk - first_blk + 1; >> + unsigned int first_blk = round_up(off, i_blocksize(inode)) >> >> + inode->i_blkbits; > > Ok, so now we round off up to the next fsblock to compute first_blk... > >> + unsigned int last_blk = (off + len) >> inode->i_blkbits; > > ...and last_blk (which is really the next block number after the range > that we're undirtying) is now rounded down. Presumably off/len have to > be aligned to fsblock granularity so we'll never have to deal with > unaligned situations like (off=324,len=1), right? Yeah, that's right. > >> unsigned long flags; >> >> + if (first_blk >= last_blk) > > Do we need this check? When would the test actually be true? Yes, we do. Following your example, when off = 324 and len = 1, we have first_block = 1 but last_block = 0. In this case, we must add this check to prevent an inversion when calculating the length to be zeroed out. Thanks, Yi. > > --D > >> + return; >> + >> spin_lock_irqsave(&ifs->state_lock, flags); >> - bitmap_clear(ifs->state, first_blk + blks_per_folio, nr_blks); >> + bitmap_clear(ifs->state, first_blk + blks_per_folio, >> + last_blk - first_blk); >> spin_unlock_irqrestore(&ifs->state_lock, flags); >> } >> >> -- >> 2.52.0 >> >> >
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index d7b648421a70..64351a448a8b 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -176,13 +176,17 @@ static void ifs_clear_range_dirty(struct folio *folio, { struct inode *inode = folio->mapping->host; unsigned int blks_per_folio = i_blocks_per_folio(inode, folio); - unsigned int first_blk = (off >> inode->i_blkbits); - unsigned int last_blk = (off + len - 1) >> inode->i_blkbits; - unsigned int nr_blks = last_blk - first_blk + 1; + unsigned int first_blk = round_up(off, i_blocksize(inode)) >> + inode->i_blkbits; + unsigned int last_blk = (off + len) >> inode->i_blkbits; unsigned long flags; + if (first_blk >= last_blk) + return; + spin_lock_irqsave(&ifs->state_lock, flags); - bitmap_clear(ifs->state, first_blk + blks_per_folio, nr_blks); + bitmap_clear(ifs->state, first_blk + blks_per_folio, + last_blk - first_blk); spin_unlock_irqrestore(&ifs->state_lock, flags); }