Re: [rfc][patch] unlock_page speedup

Hugh Dickins
2008-12-23T00:45:45Z

[-- 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