Re: [RFCv4 3/3] iomap: Support subpage size dirty tracking to improve write performance

[Matthew Wilcox <willy@infradead.org>]

On Fri, May 05, 2023 at 08:57:53AM +0530, Ritesh Harjani wrote:
> Matthew Wilcox <willy@infradead.org> writes:
> > "per-filesystem"?  I think you mean "per-block (uptodate|block) state".

(s/|block/|dirty/, sorry)

> > Using "per-block" naming throughout this patchset might help readability.
> > It's currently an awkward mix of "subpage", "sub-page" and "sub-folio".
> and subfolio to add.
> 
> Yes, I agree it got all mixed up in the comments.
> Let me stick to sub-folio (which was what we were using earlier [1])

I think per-block is better?  sub-folio might be at almost any
granularity, but per-block is specific.

> >> +static void iomap_iop_set_range(struct folio *folio, struct iomap_page *iop,
> >> +		size_t off, size_t len, enum iop_state state)
> >
> > I can't believe this is what Dave wanted you to do.  iomap_iop_set_range()
> > should be the low-level helper called by iop_set_range_uptodate() and
> > iop_set_range_dirty(), not the other way around.
> 
> Ok, I see the confusion, I think if we make
> iomap_iop_set_range() to iomap_set_range(), then that should be good.
> Then it becomes iomap_set_range() calling
> iop_set_range_update() & iop_set_range_dirty() as the lower level helper routines.
> 
> Based on the the existing code, I believe this ^^^^ is how the heirarchy
> should look like. Does it look good then? If yes, I will simply drop the
> "_iop" part in the next rev.

I don't think that's what I'm saying.  The next version
should not have enum iop_state in it.

ie it looks something like:

iomap_set_range()
{
	bitmap_set(...);
}

iomap_set_range_uptodate()
{
	iomap_set_range(...);
}

iomap_set_range_dirty()
{
	iomap_set_range(...);
}

> <Relevant function list>
>     iop_set_range_uptodate
>     iop_clear_range_uptodate
>     iop_test_uptodate
>     iop_uptodate_full
>     iop_set_range_dirty
>     iop_clear_range_dirty
>     iop_test_dirty
>     iomap_page_create
>     iomap_page_release
>     iomap_iop_set_range -> iomap_set_range()
>     iomap_iop_clear_range  -> iomap_clear_range()
> 
> -ritesh