Re: [rfc][patch] unlock_page speedup

[Hugh Dickins <hugh@veritas.com>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2074 bytes --]

On Fri, 19 Dec 2008, Linus Torvalds wrote:
> 
> That said, I did notice a problem. Namely that while the VM code is good 
> about looking at ->mapping (because it doesn't know whether the page is 
> anonymous or a true mapping), much of the filesystem code is _not_ careful 
> about page->mapping, since the filesystem code knows a-priori that the 
> mapping pointer must be an inode mapping (or we'd not have called it).
> 
> So filesystems do tend to do things like
> 
> 	struct inode *inode = page->mapping->host;
> 
> and while the low bit of mapping is magic, those code-paths don't care 
> because they depend on it being zero.
> 
> So hiding the lock bit there would involve a lot more work than I naïvely 
> expected before looking closer. We'd have to change the name (to 
> "_mapping", presumably), and make all users use an accessor function to 
> make code like the above do
> 
> 	struct inode *inode = page_mapping(page)->host;
> 
> or something (we migth want to have a "page_host_inode()" helper to do it, 
> it seems to be the most common reason for accessing "->mapping" that 
> there is.
> 
> So it could be done pretty mechanically, but it's still a _big_ change. 
> Maybe not worth it, unless we can really translate it into some other 
> advantage (ie real simplification of page flag access)

Yes, it's messy, particularly given out-of-tree filesystems.

Perhaps there's somewhere else in the struct page you could keep the
lock bit and get the advantage you're looking for: playing with the
low bits of page->mapping is best left to anon pages.

I did have a patch to keep PG_swapcache there: that got stalled on
dreaming up enough memorable names for variants of page_mapping()
that I turned out to need, then it got buried under other work.

I'll resurrect it in the next month or so; it would be nice to
keep PG_swapbacked there too, but it's not quite as easy since
Rik's split LRU categorizes shmem/tmpfs pages as swapbacked i.e.
that flag applies to anon and also to one particular filesystem.

Hugh