Re: [RFC PATCH 0/4] SGX shmem backing store issue

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, May 02, 2022 at 02:33:06PM -0700, Dave Hansen wrote:
> On 5/2/22 10:11, Reinette Chatre wrote:
> > My goal was to prevent the page fault handler from doing a "is the PCMD
> > page empty" if the reclaimer has a reference to it. Even if the PCMD page
> > is empty when the page fault handler checks it the page is expected to
> > get data right when reclaimer can get the mutex back since the reclaimer
> > already has a reference to the page.
> 
> I think shmem_truncate_range() might be the wrong operation.  It
> destroys data and, in the end, we don't want to destroy data.
> Filesystems and the page cache already have nice ways to keep from
> destroying data, we just need to use them.
> 
> First, I think set_page_dirty(pcmd_page) before the EWB is a good start.
>  That's what filesystems do before important data that needs to be saved
> goes into pages.
> 
> Second, I think we need behavior like POSIX_FADV_DONTNEED, not
> FALLOC_FL_PUNCH_HOLE.  The DONTNEED operations end up in
> mapping_evict_folio(), which has both page refcount *and* dirty page
> checks.  That means that either elevating a refcount or set_page_dirty()
> will thwart DONTNEED-like behavior.
> 
> There are two basic things we need to do:
> 
> 1. Prevent page from being truncated around EWB time
> 2. Prevent unreferenced page with all zeros from staying in shmem
>    forever or taking up swap space
> 
> On the EWB (write to PCMD side) I think something like this works:
> 
> 	sgx_encl_get_backing()
> 		get_page(pcmd_page)
> 
> 	...
> 	lock_page(pcmd_page);
> 	// check for truncation since sgx_encl_get_backing()
> 	if (pcmd_page->mapping != shmem)
> 		goto retry;
> 	 // double check this is OK under lock_page():
> 	set_page_dirty(pcmd_page);
> 	__sgx_encl_ewb();
> 	unlock_page(pcmd_page);
> 
> That's basically what filesystems do.  Get the page from the page cache,
> lock it, then make sure it is consistent.  If not, retry.
> 
> On the "free" / read in (ELDU) side:
> 
> 	// get pcmd_page ref
> 	lock_page(pcmd_page);
> 	// truncation is not a concern because that's only done
> 	// on the read-in side, here, where we hold encl->lock
> 
> 	memset();
> 	if (!memchr_inv())
> 		// clear the way for DONTNEED:
> 		ClearPageDirty(pcmd_page);
> 	unlock_page(pcmd_page);
> 	// drop pcmd_page ref
> 	...
> 	POSIX_FADV_DONTNEED
> 
> There's one downside to this: it's _possible_ that an transient
> get_page() would block POSIX_FADV_DONTNEED.  Then the zeroed page would
> stick around forever, or at least until the next ELDU operation did
> another memchr_inv().
> 
> I went looking around for some of those and could not find any that I
> *know* apply to shmem.
> 
> This doesn't feel like a great solution; it's more complicated than I
> would like.  Any other ideas?

If we could do both truncation and swapping in one side, i.e. in ksgxd,
that would simplify this process a lot. Then the whole synchronization
problem would not exist.

E.g. perhaps #PF handler could just zero PCMD and collect zeroed pages
indices to a list and ksgxd would truncate them.

BR, Jarkko