On Tue, Aug 12, 2025 at 1:15 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> 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
>
