Re: [PATCH v8 06/14] xfs: wire up MAP_DIRECT

On Tue, Oct 10, 2017 at 6:09 PM, Dave Chinner [off-list ref] wrote:

On Tue, Oct 10, 2017 at 07:49:30AM -0700, Dan Williams wrote:

quoted

@@ -1009,6 +1019,22 @@ xfs_file_llseek(
 }

 /*
+ * MAP_DIRECT faults can only be serviced while the FL_LAYOUT lease is
+ * valid. See map_direct_invalidate.
+ */
+static int
+xfs_can_fault_direct(
+     struct vm_area_struct   *vma)
+{
+     if (!xfs_vma_is_direct(vma))
+             return 0;
+
+     if (!test_map_direct_valid(vma->vm_private_data))
+             return VM_FAULT_SIGBUS;
+     return 0;
+}

Better, but I'm going to be an annoying pedant here: a "can
<something>" check should return a boolean true/false.

Also, it's a bit jarring to see that a non-direct VMA that /can't/
do direct faults returns the same thing as a direct-vma that /can/
do direct faults, so a couple of extra comments for people who will
quickly forget how this code works (i.e. me) will be helpful. Say
something like this:

/*
 * MAP_DIRECT faults can only be serviced while the FL_LAYOUT lease is
 * valid. See map_direct_invalidate.
 */
static bool
xfs_vma_has_direct_lease(
        struct vm_area_struct   *vma)
{
        /* Non MAP_DIRECT vmas do not require layout leases */
        if (!xfs_vma_is_direct(vma))
                return true;

        if (!test_map_direct_valid(vma->vm_private_data))
                return false;

        /* We have a valid lease */
        return true;
}

.....
        if (!xfs_vma_has_direct_lease(vma)) {
                ret = VM_FAULT_SIGBUS;
                goto out_unlock;
        }
....

Looks good to me.