Messages in this thread | | | From | David Howells <> | Subject | Re: [RFC][PATCH] netfs, afs, ceph: Use folios | Date | Thu, 12 Aug 2021 21:47:54 +0100 |
| |
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
| |