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

[Joanne Koong <joannelkoong@gmail.com>]

On Tue, Aug 12, 2025 at 1:15 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> > 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;
>
> This adds more sizse to the folio state.  Shouldn't this be the same
> as
>
>     DIV_ROUND_UP(write_bytes_pending, PAGE_SIZE)
>
> anyway?

I don't think we can use write_bytes_pending because writeback for a
folio may be split into multiple requests (eg for fuse, if the ranges
are not contiguous) and each request when it finishes will call
iomap_finish_folio_write() which will decrement write_bytes_pending,
but when the last folio writeback request has finished and we call
folio_end_writeback_pages(), we would need the original value of
write_bytes_pending before any of the decrements. write_bytes_pending
gets decremented since it gets used as a refcount.

I need to look more into whether readahead/read_folio and writeback
run concurrently or not but if not, maybe read_bytes_pending and
write_bytes_pending could be consolidated together.

>
> > +     unsigned end_blk = min((unsigned)(i_size_read(inode) >> inode->i_blkbits),
> > +                             i_blocks_per_folio(inode, folio));
>
> Overly long line.  Also not sure why the cast is needed to start with?

The cast is needed to avoid the compiler error of comparing a loff_t
with an unsigned int. I see there's a min_t helper, I'll use that
instead then.

>
> > +     unsigned nblks = 0;
> > +
> > +     while (start_blk < end_blk) {
> > +             if (ifs_block_is_dirty(folio, ifs, start_blk))
> > +                     nblks++;
> > +             start_blk++;
> > +     }
>
> We have this pattern open coded in a few places.  Maybe factor it into a
> helper first?  And then maybe someone smart can actually make it use
> find_first_bit/find_next_bit.
>
> > +static bool iomap_granular_dirty_pages(struct folio *folio)
> > +{
> > +     struct iomap_folio_state *ifs = folio->private;
> > +     struct inode *inode;
> > +     unsigned block_size;
> > +
> > +     if (!ifs)
> > +             return false;
> > +
> > +     inode = folio->mapping->host;
> > +     block_size = 1 << inode->i_blkbits;
> > +
> > +     if (block_size >= PAGE_SIZE) {
> > +             WARN_ON(block_size & (PAGE_SIZE - 1));
> > +             return true;
> > +     }
> > +     return false;
>
> Do we need the WARN_ON?  Both the block and page size must be powers
> of two, so I can't see how it would trigger.  Also this can use the
> i_blocksize helper.

I'll get rid of the WARN_ON and will incorporate your i_blocksize
helper suggestion.

>
> I.e. just turn this into:
>
>         return i_blocksize(folio->mapping->host) >= PAGE_SIZE;
>
>
> > +static bool iomap_dirty_folio_range(struct address_space *mapping, struct folio *folio,
>
> Overly long line.

I'll fix up the long lines in the patchset, sorry.

>
> > +     wpc->wbc->no_stats_accounting = true;
>
> Who does the writeback accounting now?  Maybe throw in a comment if
> iomap is now doing something different than all the other writeback
> code.

iomap does the writeback accounting now, which happens in
iomap_update_dirty_stats(). I'll add a comment about that.


Thanks,
Joanne
>