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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3308343.1628801274@warthog.procyon.org.uk>
Date:   Thu, 12 Aug 2021 21:47:54 +0100
From:   David Howells <dhowells@...hat.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     dhowells@...hat.com, Jeff Layton <jlayton@...nel.org>,
        Marc Dionne <marc.dionne@...istor.com>,
        Ilya Dryomov <idryomov@...il.com>,
        linux-afs@...ts.infradead.org, ceph-devel@...r.kernel.org,
        linux-cachefs@...hat.com, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, linux-fsdevel@...r.kernel.org
Subject: Re: [RFC][PATCH] netfs, afs, ceph: Use folios

Matthew Wilcox <willy@...radead.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