Re: [RFC][PATCH] netfs, afs, ceph: Use folios

[David Howells <dhowells@redhat.com>]

Matthew Wilcox <willy@infradead.org> wrote:

> >  (*) Can page_endio() be split into two separate functions, one for read
> >      and one for write?  If seems a waste of time to conditionally switch
> >      between two different branches.
> 
> At this point I'm thinking ...
> 
> static inline void folio_end_read(struct folio *folio, int err)
> {
> 	if (!err)
> 		folio_set_uptodate(folio);
> 	folio_unlock(folio);
> }
> 
> Clearly the page isn't uptodate at this point, or ->readpage wouldn't've
> been called.  So there's no need to clear it.  And PageError is
> completely useless.

Seems reasonable.

> > -	*_page = page;
> > +	*_page = &folio->page;
> 
> Can't do anything about this one; the write_begin API needs to be fixed.

That's fine.  I expected things like this at this stage.

> > @@ -174,40 +175,32 @@ static void afs_kill_pages(struct address_space *mapping,
> [...]
> > +		folio_clear_uptodate(folio);
> > +		folio_end_writeback(folio);
> > +		folio_lock(folio);
> > +		generic_error_remove_page(mapping, &folio->page);
> > +		folio_unlock(folio);
> > +		folio_put(folio);
> 
> This one I'm entirely missing.  It's awkward.  I'll work on it.

afs_kill_pages() is just a utility to end writeback, clear uptodate and do
generic_error_remove_page() over a range of pages and afs_redirty_pages() is a
utility that to end writeback and redirty a range of pages - hence why I was
thinking it might make sense to put them into common code.

> > -			index += thp_nr_pages(page);
> > -			if (!pagevec_add(&pvec, page))
> > +			index += folio_nr_pages(folio);
> > +			if (!pagevec_add(&pvec, &folio->page))
> 
> Pagevecs are also awkward.  I haven't quite figured out how to
> transition them to folios.

Maybe provide pagevec_add_folio(struct pagevec *, struct folio *)?

> >  zero_out:
> > -	zero_user_segments(page, 0, offset, offset + len, thp_size(page));
> > +	zero_user_segments(&folio->page, 0, offset, offset + len, folio_size(folio));
> 
> Yeah, that's ugly.

Maybe:

	folio_clear_around(folio, keep_from, keep_to);

clearing the bits of the folio outside the specified section?

David