Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 8 Oct 2020 16:43:41 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Jerome Glisse <jglisse@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        linux-fsdevel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Tejun Heo <tj@kernel.org>, Jan Kara <jack@suse.cz>,
        Josef Bacik <jbacik@fb.com>
Subject: Re: [PATCH 00/14] Small step toward KSM for file back page.
Message-ID: <20201008154341.GJ20115@casper.infradead.org>
References: <20201007010603.3452458-1-jglisse@redhat.com>
 <20201007032013.GS20115@casper.infradead.org>
 <20201007144835.GA3471400@redhat.com>
 <20201007170558.GU20115@casper.infradead.org>
 <20201007175419.GA3478056@redhat.com>
 <20201007220916.GX20115@casper.infradead.org>
 <20201008153028.GA3508856@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201008153028.GA3508856@redhat.com>
Precedence: bulk

On Thu, Oct 08, 2020 at 11:30:28AM -0400, Jerome Glisse wrote:
> On Wed, Oct 07, 2020 at 11:09:16PM +0100, Matthew Wilcox wrote:
> > So ... why don't you put a PageKsm page in the page cache?  That way you
> > can share code with the current KSM implementation.  You'd need
> > something like this:
> 
> I do just that but there is no need to change anything in page cache.

That's clearly untrue.  If you just put a PageKsm page in the page
cache today, here's what will happen on a truncate:

void truncate_inode_pages_range(struct address_space *mapping,
                                loff_t lstart, loff_t lend)
{
...
                struct page *page = find_lock_page(mapping, start - 1);

find_lock_page() does this:
        return pagecache_get_page(mapping, offset, FGP_LOCK, 0);

pagecache_get_page():

repeat:
        page = find_get_entry(mapping, index);
...
        if (fgp_flags & FGP_LOCK) {
...
                if (unlikely(compound_head(page)->mapping != mapping)) {
                        unlock_page(page);
                        put_page(page);
                        goto repeat;

so it's just going to spin.  There are plenty of other codepaths that
would need to be checked.  If you haven't found them, that shows you
don't understand the problem deeply enough yet.

I believe we should solve this problem, but I don't think you're going
about it the right way.

> So flow is:
> 
>   Same as before:
>     1 - write fault (address, vma)
>     2 - regular write fault handler -> find page in page cache
> 
>   New to common page fault code:
>     3 - ksm check in write fault common code (same as ksm today
>         for anonymous page fault code path).
>     4 - break ksm (address, vma) -> (file offset, mapping)
>         4.a - use mapping and file offset to lookup the proper
>               fs specific information that were save when the
>               page was made ksm.
>         4.b - allocate new page and initialize it with that
>               information (and page content), update page cache
>               and mappings ie all the pte who where pointing to
>               the ksm for that mapping at that offset to now use
>               the new page (like KSM for anonymous page today).

But by putting that logic in the page fault path, you've missed
the truncate path.  And maybe other places.  Putting the logic
down in pagecache_get_page() means you _don't_ need to find
all the places that call pagecache_get_page().