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 09:55:53PM +0530, Ritesh Harjani wrote:
> Matthew Wilcox <willy@infradead.org> writes:
> 
> > 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.
> >
> 
> We represent each FS block as a bit in the bitmap. So first_blkp or
> first_bitp or first_blkbitp essentially means the same. 
> I went with first_blk, first_blkp in the first place based on your
> suggestion itself [1].

No, it's not the same!  If you have 1kB blocks in a 64kB page, they're
numbered 0-63.  If you 'calc_range' for any of the dirty bits, you get
back a number in the range 64-127.  That's not a block number!  It's
the number of the bit you want to refer to.  Calling it blkp is going
to lead to confusion -- as you yourself seem to be confused.

> [1]: https://lore.kernel.org/linux-xfs/Y%2FvxlVUJ31PZYaRa@casper.infradead.org/

Those _were_ block numbers!  off >> inode->i_blkbits calculates a block
number.  (off >> inode->i_blkbits) + blocks_per_folio() does not calculate
a block number, it calculates a bit number.

> >> -	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.
> 
> Actually I never wanted to use the the enum this way. That's why I was
> not fond of the idea behind using enum in all the bitmap state
> manipulation APIs (test/set/).
> 
> It was only intended to be passed as a state argument to ifs_calc_range()
> function to keep all the first_blkp and nr_blksp calculation at one
> place. And just use it's IOMAP_ST_MAX value while allocating state bitmap.
> It was never intended to be used like this.
> 
> We can even now go back to this original idea and keep the use of the
> enum limited to what I just mentioned above i.e. for ifs_calc_range().
> 
> And maybe just use this in ifs_alloc()?
> BUILD_BUG_ON(IOMAP_ST_UPTODATE == 0);
> BUILD_BUG_ON(IOMAP_ST_DIRTY == 1);
> 
> > Define the uptodate bits to be the first ones in the bitmap,
> > document it (and why), and leave it at that.
> 
> Do you think we can go with above suggestion, or do you still think we
> need to drop it?
> 
> In case if we drop it, then should we open code the calculations for
> first_blk, last_blk? These calculations are done in exact same fashion
> at 3 places ifs_set_range_uptodate(), ifs_clear_range_dirty() and
> ifs_set_range_dirty().
> Thoughts?

I disliked the enum from the moment I saw it, but didn't care enough to
say so.

Look, an abstraction should have a _purpose_.  The enum doesn't.  I'd
ditch this calc_range function entirely; it's just not worth it.