On Fri, May 05, 2023 at 08:57:53AM +0530, Ritesh Harjani wrote:
> Matthew Wilcox <willy@xxxxxxxxxxxxx> 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
