On Sat, Sep 19, 2020 at 05:16:57PM -0700, Hugh Dickins wrote:
On Sat, 19 Sep 2020, Matthew Wilcox wrote:
How about this for a band-aid until we sort that out properly? Just mark the page as dirty before splitting it so subsequent iterations see the subpages as dirty.
This is certainly much better than my original, and I've had no problem in running with it (briefly); and it's what I'll now keep in my testing tree - thanks. But it is more obviously a hack, or as you put it, a band-aid: fit for Linus's tree?
This issue has only come up with i915 and shmem_enabled "force", nobody has been bleeding except me: but if it's something that impedes or may impede your own testing, or that you feel should go in anyway, say so and I'll push your new improved version to akpm (adding your signoff to mine).
No, it's not impeding my testing; I don't have i915 even compiled into my kernel.
Arguably, we should use set_page_dirty() instead of SetPageDirty,
My position on that has changed down the years: I went through a phase of replacing shmem.c's SetPageDirtys by set_page_dirtys, with the idea that its "if (!PageDirty)" is good to avoid setting cacheline dirty. Then Spectre changed the balance, so now I'd rather avoid the indirect function call, and go with your SetPageDirty. But the bdi flags are such that they do the same thing, and if they ever diverge, then we make the necessary changes at that time.
That makes sense.
but I don't think i915 cares. In particular, it uses an untagged iteration instead of searching for PAGECACHE_TAG_DIRTY.
PAGECACHE_TAG_DIRTY comes with bdi flags that shmem does not use: with one exception, every shmem page is always dirty (since its swap is freed as soon as the page is moved from swap cache to file cache); unless I'm forgetting something (like the tails in my patch!), the only exception is a page allocated to a hole on fault (after which a write fault will soon set pte dirty).
Ah, of course. I keep forgetting that shmem doesn't use the dirty/writeback bits.
So I didn't quite get what i915 was doing with its set_page_dirty()s: but very probably being a good citizen, marking when it exposed the page to changes itself, making no assumption of what shmem.c does.
It would be good to have a conversation with i915 guys about huge pages sometime (they forked off their own mount point in i915_gemfs_init(), precisely in order to make use of huge pages, but couldn't get them working, so removed the option to ask for them, but kept the separate mount point. But not a conversation I'll be responsive on at present.)
Yes, I have a strong feeling the i915 shmem code is cargo-culted. There are some very questionable constructs in there. It's a little unfair to expect graphics developers to suddenly become experts on the page cache, so I've taken this as impetus to clean up our APIs a little (eg moving find_lock_entry() to mm/internal.h)