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

[Christoph Hellwig <hch@infradead.org>]

> 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?

> +	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?

> +	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.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.

> +	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.