Re: [RFC PATCH v1 10/10] iomap: add granular dirty and writeback accounting

[Joanne Koong <joannelkoong@gmail.com>]

On Thu, Aug 14, 2025 at 9:38 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Thu, Jul 31, 2025 at 05:21:31PM -0700, Joanne Koong wrote:
> > Add granular dirty and writeback accounting for large folios. These
> > stats are used by the mm layer for dirty balancing and throttling.
> > Having granular dirty and writeback accounting helps prevent
> > over-aggressive balancing and throttling.
> >
> > There are 4 places in iomap this commit affects:
> > a) filemap dirtying, which now calls filemap_dirty_folio_pages()
> > b) writeback_iter with setting the wbc->no_stats_accounting bit and
> > calling clear_dirty_for_io_stats()
> > c) starting writeback, which now calls __folio_start_writeback()
> > d) ending writeback, which now calls folio_end_writeback_pages()
> >
> > This relies on using the ifs->state dirty bitmap to track dirty pages in
> > the folio. As such, this can only be utilized on filesystems where the
> > block size >= PAGE_SIZE.
>
> Apologies for my slow responses this month. :)

No worries at all, thanks for looking at this.
>
> I wonder, does this cause an observable change in the writeback
> accounting and throttling behavior for non-fuse filesystems like XFS
> that use large folios?  I *think* this does actually reduce throttling
> for XFS, but it might not be so noticeable because the limits are much
> more generous outside of fuse?

I haven't run any benchmarks on non-fuse filesystems yet but that's
what I would expect too. Will run some benchmarks to see!

>
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  fs/iomap/buffered-io.c | 136 ++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 128 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index bcc6e0e5334e..626c3c8399cc 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -20,6 +20,8 @@ struct iomap_folio_state {
> >       spinlock_t              state_lock;
> >       unsigned int            read_bytes_pending;
> >       atomic_t                write_bytes_pending;
> > +     /* number of pages being currently written back */
> > +     unsigned                nr_pages_writeback;
> >
> >       /*
> >        * Each block has two bits in this bitmap:
> > @@ -81,6 +83,25 @@ static inline bool ifs_block_is_dirty(struct folio *folio,
> >       return test_bit(block + blks_per_folio, ifs->state);
> >  }
> >
> > +static unsigned ifs_count_dirty_pages(struct folio *folio)
> > +{
> > +     struct iomap_folio_state *ifs = folio->private;
> > +     struct inode *inode = folio->mapping->host;
> > +     unsigned block_size = 1 << inode->i_blkbits;
> > +     unsigned start_blk = 0;
> > +     unsigned end_blk = min((unsigned)(i_size_read(inode) >> inode->i_blkbits),
> > +                             i_blocks_per_folio(inode, folio));
> > +     unsigned nblks = 0;
> > +
> > +     while (start_blk < end_blk) {
> > +             if (ifs_block_is_dirty(folio, ifs, start_blk))
> > +                     nblks++;
> > +             start_blk++;
> > +     }
>
> Hmm, isn't this bitmap_weight(ifs->state, blks_per_folio) ?
>
> Ohh wait no, the dirty bitmap doesn't start on a byte boundary because
> the format of the bitmap is [uptodate bits][dirty bits].
>
> Maybe those two should be reversed, because I bet the dirty state gets
> changed a lot more over the lifetime of a folio than the uptodate bits.

I think there's the find_next_bit() helper (which Christoph also
pointed out) that could probably be used here instead. Or at least
that's how I see a lot of the driver code doing it for their bitmaps.