Re: [PATCHv10 8/8] iomap: Add per-block dirty state tracking to improve performance

[Matthew Wilcox <willy@infradead.org>]

On Mon, Jun 19, 2023 at 07:58:51AM +0530, Ritesh Harjani (IBM) wrote:
> +static void ifs_calc_range(struct folio *folio, size_t off, size_t len,
> +		enum iomap_block_state state, unsigned int *first_blkp,
> +		unsigned int *nr_blksp)
> +{
> +	struct inode *inode = folio->mapping->host;
> +	unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> +	unsigned int first = off >> inode->i_blkbits;
> +	unsigned int last = (off + len - 1) >> inode->i_blkbits;
> +
> +	*first_blkp = first + (state * blks_per_folio);
> +	*nr_blksp = last - first + 1;
> +}

As I said, this is not 'first_blkp'.  It's first_bitp.  I think this
misunderstanding is related to Andreas' complaint, but it's not quite
the same.

>  static inline bool ifs_is_fully_uptodate(struct folio *folio,
>  					       struct iomap_folio_state *ifs)
>  {
>  	struct inode *inode = folio->mapping->host;
> +	unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> +	unsigned int nr_blks = (IOMAP_ST_UPTODATE + 1) * blks_per_folio;
>  
> -	return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
> +	return bitmap_full(ifs->state, nr_blks);

I think we have a gap in our bitmap APIs.  We don't have a
'bitmap_range_full(src, pos, nbits)'.  We could use find_next_zero_bit(),
but that's going to do more work than necessary.

Given this lack, perhaps it's time to say that you're making all of
this too hard by using an enum, and pretending that we can switch the
positions of 'uptodate' and 'dirty' in the bitmap just by changing
the enum.  Define the uptodate bits to be the first ones in the bitmap,
document it (and why), and leave it at that.