On Wed, Apr 14, 2021 at 12:36:13AM -0700, Hugh Dickins wrote:

On Mon, 12 Apr 2021, Axel Rasmussen wrote:

quoted

This patch allows shmem-backed VMAs to be registered for minor faults.
Minor faults are appropriately relayed to userspace in the fault path,
for VMAs with the relevant flag.

This commit doesn't hook up the UFFDIO_CONTINUE ioctl for shmem-backed
minor faults, though, so userspace doesn't yet have a way to resolve
such faults.

This is a very odd way to divide up the series: an "Intermission"
half way through the implementation of MINOR/CONTINUE: this 3/9
makes little sense without the 4/9 to mm/userfaultfd.c which follows.

But, having said that, I won't object and Peter did not object, and
I don't know of anyone else looking here: it will only give each of
us more trouble to insist on repartitioning the series, and it's the
end state that's far more important to me and to all of us.

Agreed, ideally it should be after patch 4 since this patch enables the
feature already.

And I'll even seize on it, to give myself an intermission after
this one, until tomorrow (when I'll look at 4/9 and 9/9 - but
shall not look at the selftests ones at all).

Most of this is okay, except the mm/shmem.c part; and I've just now
realized that somewhere (whether in this patch or separately) there
needs to be an update to Documentation/admin-guide/mm/userfaultfd.rst
(admin-guide? how weird, but not this series' business to correct).

(maybe some dir "devel" would suite better?  But I do also see soft-dirty.rst,
 idle_page_tracking.rst,..)

[...]

quoted

 static int shmem_getpage_gfp(struct inode *inode, pgoff_t index,

@@ -1820,6 +1820,14 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index,
 
 	page = pagecache_get_page(mapping, index,
 					FGP_ENTRY | FGP_HEAD | FGP_LOCK, 0);
+
+	if (page && vma && userfaultfd_minor(vma)) {
+		unlock_page(page);
+		put_page(page);
+		*fault_type = handle_userfault(vmf, VM_UFFD_MINOR);
+		return 0;
+	}
+

Okay, Peter persuaded you to move that up here: where indeed it
does look better than the earlier "swapped" version.

But will crash on swap as it's currently written: it needs to say
		if (!xa_is_value(page)) {
			unlock_page(page);
			put_page(page);
		}

And this is definitely true...  Thanks,

I did say before that it's more robust to return from the swap
case after doing the shmem_swapin_page(). But I might be slowly
realizing that the ioctl to add the pte (in 4/9) will do its
shmem_getpage_gfp(), and that will bring in the swap if user
did not already do so: so I was wrong to claim more robustness
the other way, this placement should be fine. I think.

quoted

 	if (xa_is_value(page)) {
 		error = shmem_swapin_page(inode, index, &page,
 					  sgp, gfp, vma, fault_type);
-- 
2.31.1.295.g9ea45b61b8-goog

-- 
Peter Xu